Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #8

Open
code423n4 opened this issue Apr 21, 2022 · 0 comments
Open

Gas Optimizations #8

code423n4 opened this issue Apr 21, 2022 · 0 comments
Labels
bug Something isn't working G (Gas Optimization) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

general considerations

  • there are multiple instances of block.timestamp.safeCastTo32(), which could be changed to uint32(block.timestamp) with no practical risk, and saves the require() check
  • uint256 should be used in many scenarios inside of the functions or as function parameters instead, and only cast it into a lower size uint, when storing it instead. reference. for example timestamp is being casted way too early, and then being used, resulting in higher gas.
  • snapshots below were run with forge snapshot --optimize --optimize-runs 1000000

use storage pointers where possible [ERC20MultiVotes]

diff --git a/.gas-snapshot b/.gas-snapshot
index c5e3f62..c439cf6 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -53,13 +53,13 @@ ERC20MultiVotesTest:testCanContractExceedMax() (gas: 35031)
 ERC20MultiVotesTest:testCanContractExceedMaxNonContractFails() (gas: 13535)
 ERC20MultiVotesTest:testCanContractExceedMaxNonOwnerFails() (gas: 13760)
 ERC20MultiVotesTest:testDecrementOverWeightFails() (gas: 247398)
-ERC20MultiVotesTest:testDecrementUntilFreeDouble() (gas: 350624)
-ERC20MultiVotesTest:testDecrementUntilFreeSingle() (gas: 327524)
+ERC20MultiVotesTest:testDecrementUntilFreeDouble() (gas: 350352)
+ERC20MultiVotesTest:testDecrementUntilFreeSingle() (gas: 327354)
 ERC20MultiVotesTest:testDecrementUntilFreeWhenFree() (gas: 375129)
 ERC20MultiVotesTest:testDelegateOverMaxDelegatesApproved() (gas: 509277)
 ERC20MultiVotesTest:testDelegateOverMaxDelegatesFails() (gas: 413581)
 ERC20MultiVotesTest:testDelegateOverVotesFails() (gas: 247599)
 ERC20MultiVotesTest:testDelegateToAddressZeroFails() (gas: 87370)
-ERC20MultiVotesTest:testPastVotes() (gas: 351022)
+ERC20MultiVotesTest:testPastVotes() (gas: 350952)
 ERC20MultiVotesTest:testSetMaxDelegatesNonOwnerFails() (gas: 13647)
-ERC20MultiVotesTest:testUndelegate() (gas: 211796)
+ERC20MultiVotesTest:testUndelegate() (gas: 211684)
diff --git a/src/token/ERC20MultiVotes.sol b/src/token/ERC20MultiVotes.sol
index 08e2242..bb75374 100644
--- a/src/token/ERC20MultiVotes.sol
+++ b/src/token/ERC20MultiVotes.sol
@@ -260,13 +260,14 @@ abstract contract ERC20MultiVotes is ERC20, Auth {
         address delegatee,
         uint256 amount
     ) internal virtual {
-        uint256 newDelegates = _delegatesVotesCount[delegator][delegatee] - amount;
+        mapping(address => uint256) storage ptr = _delegatesVotesCount[delegator];
+        uint256 newDelegates = ptr[delegatee] - amount;
 
         if (newDelegates == 0) {
             require(_delegates[delegator].remove(delegatee));
         }
 
-        _delegatesVotesCount[delegator][delegatee] = newDelegates;
+        ptr[delegatee] = newDelegates;
         userDelegatedVotes[delegator] -= amount;
 
         emit Undelegation(delegator, delegatee, amount);
@@ -339,19 +340,22 @@ abstract contract ERC20MultiVotes is ERC20, Auth {
         uint256 totalFreed;
 
         // Loop through all delegates
-        address[] memory delegateList = _delegates[user].values();
+        EnumerableSet.AddressSet storage _delegatesUser = _delegates[user];
+        address[] memory delegateList = _delegatesUser.values();
 
         // Free delegates until through entire list or under votes amount
         uint256 size = delegateList.length;
         for (uint256 i = 0; i < size && (userFreeVotes + totalFreed) < votes; i++) {
             address delegatee = delegateList[i];
-            uint256 delegateVotes = _delegatesVotesCount[user][delegatee];
+
+            mapping(address => uint256) storage _delegatesVotesCountUser = _delegatesVotesCount[user];
+            uint256 delegateVotes = _delegatesVotesCountUser[delegatee];
             if (delegateVotes != 0) {
                 totalFreed += delegateVotes;
 
-                require(_delegates[user].remove(delegatee)); // Remove from set. Should never fail.
+                require(_delegatesUser.remove(delegatee)); // Remove from set. Should never fail.
 
-                _delegatesVotesCount[user][delegatee] = 0;
+                _delegatesVotesCountUser[delegatee] = 0;
 
                 _writeCheckpoint(delegatee, _subtract, delegateVotes);
                 emit Undelegation(user, delegatee, delegateVotes);

use storage pointers where possible [ERC20Gauges]

diff --git a/.gas-snapshot b/.gas-snapshot
index c439cf6..ddddaf1 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -4,17 +4,17 @@ FlywheelTest:testAccrueTwoUsersSeparately() (gas: 330979)
 FlywheelTest:testFailAddStrategy() (gas: 15092)
 FlywheelTest:testSetFlywheelBoosterUnauthorized() (gas: 13594)
 FlywheelTest:testSetFlywheelRewardsUnauthorized() (gas: 13615)
-FlywheelGaugeRewardsTest:testGetPriorRewards() (gas: 566359)
-FlywheelGaugeRewardsTest:testGetRewards() (gas: 546077)
+FlywheelGaugeRewardsTest:testGetPriorRewards() (gas: 566122)
+FlywheelGaugeRewardsTest:testGetRewards() (gas: 545907)
 FlywheelGaugeRewardsTest:testGetRewardsUninitialized() (gas: 12512)
-FlywheelGaugeRewardsTest:testIncompletePagination() (gas: 972359)
-FlywheelGaugeRewardsTest:testPagination() (gas: 900730)
-FlywheelGaugeRewardsTest:testQueue() (gas: 524891)
-FlywheelGaugeRewardsTest:testQueueFullPagination() (gas: 527970)
-FlywheelGaugeRewardsTest:testQueueSkipCycle() (gas: 345785)
-FlywheelGaugeRewardsTest:testQueueSkipCycleFullPagination() (gas: 348443)
-FlywheelGaugeRewardsTest:testQueueTwoCycles() (gas: 560780)
-FlywheelGaugeRewardsTest:testQueueTwoCyclesFullPagination() (gas: 566759)
+FlywheelGaugeRewardsTest:testIncompletePagination() (gas: 972019)
+FlywheelGaugeRewardsTest:testPagination() (gas: 900390)
+FlywheelGaugeRewardsTest:testQueue() (gas: 524721)
+FlywheelGaugeRewardsTest:testQueueFullPagination() (gas: 527800)
+FlywheelGaugeRewardsTest:testQueueSkipCycle() (gas: 345700)
+FlywheelGaugeRewardsTest:testQueueSkipCycleFullPagination() (gas: 348358)
+FlywheelGaugeRewardsTest:testQueueTwoCycles() (gas: 560543)
+FlywheelGaugeRewardsTest:testQueueTwoCyclesFullPagination() (gas: 566522)
 FlywheelGaugeRewardsTest:testQueueWithoutGaugesBeforeCycle() (gas: 13468)
 FlywheelGaugeRewardsTest:testQueueWithoutGaugesNoGauges() (gas: 69137)
 FlywheelStaticRewardsTest:testGetAccruedRewards() (gas: 97429)
@@ -23,27 +23,27 @@ FlywheelStaticRewardsTest:testGetAccruedRewardsCappedAfterEnd() (gas: 97873)
 FlywheelStaticRewardsTest:testSetRewardsInfo() (gas: 39879)
 ERC20GaugesTest:testAddGaugeNonOwner() (gas: 38452)
 ERC20GaugesTest:testAddGaugeTwice() (gas: 113090)
-ERC20GaugesTest:testCalculateGaugeAllocation() (gas: 488805)
+ERC20GaugesTest:testCalculateGaugeAllocation() (gas: 488640)
 ERC20GaugesTest:testCanContractExceedMax() (gas: 35078)
 ERC20GaugesTest:testCanContractExceedMaxNonContract() (gas: 13603)
 ERC20GaugesTest:testCanContractExceedMaxNonOwner() (gas: 13917)
-ERC20GaugesTest:testDecrement() (gas: 310594)
-ERC20GaugesTest:testDecrementAllRemovesGauge() (gas: 300684)
-ERC20GaugesTest:testDecrementGauges() (gas: 432470)
-ERC20GaugesTest:testDecrementGaugesOver() (gas: 415437)
-ERC20GaugesTest:testDecrementGaugesSizeMismatch() (gas: 466673)
-ERC20GaugesTest:testDecrementUntilFreeDeprecated() (gas: 447041)
-ERC20GaugesTest:testDecrementUntilFreeDouble() (gas: 431097)
-ERC20GaugesTest:testDecrementUntilFreeSingle() (gas: 449392)
-ERC20GaugesTest:testDecrementUntilFreeWhenFree() (gas: 477079)
-ERC20GaugesTest:testDecrementWithStorage() (gas: 324403)
-ERC20GaugesTest:testIncrementAtMax() (gas: 380777)
-ERC20GaugesTest:testIncrementGauges() (gas: 477454)
+ERC20GaugesTest:testDecrement() (gas: 310419)
+ERC20GaugesTest:testDecrementAllRemovesGauge() (gas: 300562)
+ERC20GaugesTest:testDecrementGauges() (gas: 432171)
+ERC20GaugesTest:testDecrementGaugesOver() (gas: 415206)
+ERC20GaugesTest:testDecrementGaugesSizeMismatch() (gas: 466503)
+ERC20GaugesTest:testDecrementUntilFreeDeprecated() (gas: 446756)
+ERC20GaugesTest:testDecrementUntilFreeDouble() (gas: 430812)
+ERC20GaugesTest:testDecrementUntilFreeSingle() (gas: 449178)
+ERC20GaugesTest:testDecrementUntilFreeWhenFree() (gas: 476909)
+ERC20GaugesTest:testDecrementWithStorage() (gas: 324228)
+ERC20GaugesTest:testIncrementAtMax() (gas: 380697)
+ERC20GaugesTest:testIncrementGauges() (gas: 477289)
 ERC20GaugesTest:testIncrementGaugesDeprecated() (gas: 282189)
-ERC20GaugesTest:testIncrementGaugesOver() (gas: 421785)
+ERC20GaugesTest:testIncrementGaugesOver() (gas: 421615)
 ERC20GaugesTest:testIncrementGaugesSizeMismatch() (gas: 281154)
-ERC20GaugesTest:testIncrementOverMax() (gas: 420342)
-ERC20GaugesTest:testIncrementWithStorage() (gas: 514952)
+ERC20GaugesTest:testIncrementOverMax() (gas: 420170)
+ERC20GaugesTest:testIncrementWithStorage() (gas: 514787)
 ERC20GaugesTest:testRemoveGauge() (gas: 179755)
 ERC20GaugesTest:testRemoveGaugeNonOwner() (gas: 112716)
 ERC20GaugesTest:testRemoveGaugeTwice() (gas: 180455)
diff --git a/src/token/ERC20Gauges.sol b/src/token/ERC20Gauges.sol
index ad8fabe..2b59338 100644
--- a/src/token/ERC20Gauges.sol
+++ b/src/token/ERC20Gauges.sol
@@ -259,9 +259,10 @@ abstract contract ERC20Gauges is ERC20, Auth {
             if (cycle - block.timestamp <= incrementFreezeWindow) revert IncrementFreezeError();
         }
 
-        bool added = _userGauges[user].add(gauge); // idempotent add
-        if (added && _userGauges[user].length() > maxGauges && !canContractExceedMaxGauges[user])
-            revert MaxGaugeError();
+        EnumerableSet.AddressSet storage pUserGauges = _userGauges[user];
+
+        bool added = pUserGauges.add(gauge); // idempotent add
+        if (added && pUserGauges.length() > maxGauges && !canContractExceedMaxGauges[user]) revert MaxGaugeError();
 
         getUserGaugeWeight[user][gauge] += weight;
 
@@ -337,9 +338,10 @@ abstract contract ERC20Gauges is ERC20, Auth {
         uint112 weight,
         uint32 cycle
     ) internal {
-        uint112 oldWeight = getUserGaugeWeight[user][gauge];
+        mapping(address => uint112) storage pUserGaugeWeight = getUserGaugeWeight[user];
+        uint112 oldWeight = pUserGaugeWeight[gauge];
 
-        getUserGaugeWeight[user][gauge] = oldWeight - weight;
+        pUserGaugeWeight[gauge] = oldWeight - weight;
         if (oldWeight == weight) {
             // If removing all weight, remove gauge from user list.
             require(_userGauges[user].remove(gauge));
@@ -561,9 +563,11 @@ abstract contract ERC20Gauges is ERC20, Auth {
 
         // Free gauges until through entire list or under weight
         uint256 size = gaugeList.length;
+        mapping(address => uint112) storage pUserGaugeWeight = getUserGaugeWeight[user];
+
         for (uint256 i = 0; i < size && (userFreeWeight + totalFreed) < weight; ) {
             address gauge = gaugeList[i];
-            uint112 userGaugeWeight = getUserGaugeWeight[user][gauge];
+            uint112 userGaugeWeight = pUserGaugeWeight[gauge];
             if (userGaugeWeight != 0) {
                 // If the gauge is live (not deprecated), include its weight in the total to remove
                 if (!_deprecatedGauges.contains(gauge)) {

unchecked i++ [FlywheelGaugeRewards]

diff --git a/.gas-snapshot b/.gas-snapshot
index ddddaf1..14733f7 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -4,17 +4,17 @@ FlywheelTest:testAccrueTwoUsersSeparately() (gas: 330979)
 FlywheelTest:testFailAddStrategy() (gas: 15092)
 FlywheelTest:testSetFlywheelBoosterUnauthorized() (gas: 13594)
 FlywheelTest:testSetFlywheelRewardsUnauthorized() (gas: 13615)
-FlywheelGaugeRewardsTest:testGetPriorRewards() (gas: 566122)
-FlywheelGaugeRewardsTest:testGetRewards() (gas: 545907)
+FlywheelGaugeRewardsTest:testGetPriorRewards() (gas: 565862)
+FlywheelGaugeRewardsTest:testGetRewards() (gas: 545777)
 FlywheelGaugeRewardsTest:testGetRewardsUninitialized() (gas: 12512)
-FlywheelGaugeRewardsTest:testIncompletePagination() (gas: 972019)
-FlywheelGaugeRewardsTest:testPagination() (gas: 900390)
-FlywheelGaugeRewardsTest:testQueue() (gas: 524721)
-FlywheelGaugeRewardsTest:testQueueFullPagination() (gas: 527800)
-FlywheelGaugeRewardsTest:testQueueSkipCycle() (gas: 345700)
-FlywheelGaugeRewardsTest:testQueueSkipCycleFullPagination() (gas: 348358)
-FlywheelGaugeRewardsTest:testQueueTwoCycles() (gas: 560543)
-FlywheelGaugeRewardsTest:testQueueTwoCyclesFullPagination() (gas: 566522)
+FlywheelGaugeRewardsTest:testIncompletePagination() (gas: 971499)
+FlywheelGaugeRewardsTest:testPagination() (gas: 900130)
+FlywheelGaugeRewardsTest:testQueue() (gas: 524591)
+FlywheelGaugeRewardsTest:testQueueFullPagination() (gas: 527670)
+FlywheelGaugeRewardsTest:testQueueSkipCycle() (gas: 345635)
+FlywheelGaugeRewardsTest:testQueueSkipCycleFullPagination() (gas: 348293)
+FlywheelGaugeRewardsTest:testQueueTwoCycles() (gas: 560283)
+FlywheelGaugeRewardsTest:testQueueTwoCyclesFullPagination() (gas: 566262)
 FlywheelGaugeRewardsTest:testQueueWithoutGaugesBeforeCycle() (gas: 13468)
 FlywheelGaugeRewardsTest:testQueueWithoutGaugesNoGauges() (gas: 69137)
 FlywheelStaticRewardsTest:testGetAccruedRewards() (gas: 97429)
diff --git a/src/rewards/FlywheelGaugeRewards.sol b/src/rewards/FlywheelGaugeRewards.sol
index 85a77c0..f099b07 100644
--- a/src/rewards/FlywheelGaugeRewards.sol
+++ b/src/rewards/FlywheelGaugeRewards.sol
@@ -186,7 +186,7 @@ contract FlywheelGaugeRewards is Auth, BaseFlywheelRewards {
 
         if (size == 0) revert EmptyGaugesError();
 
-        for (uint256 i = 0; i < size; i++) {
+        for (uint256 i = 0; i < size; ) {
             ERC20 gauge = ERC20(gauges[i]);
 
             QueuedRewards memory queuedRewards = gaugeQueuedRewards[gauge];
@@ -206,6 +206,10 @@ contract FlywheelGaugeRewards is Auth, BaseFlywheelRewards {
             });
 
             emit QueueRewards(address(gauge), currentCycle, nextRewards);
+
+            unchecked {
+                i++;
+            }
         }
     }
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Apr 21, 2022
code423n4 added a commit that referenced this issue Apr 21, 2022
@Joeysantoro Joeysantoro added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

2 participants