Gas optimization certora (ETH-1626) (#252)

* feat: foundry gas diff * fix: updated workflow file * fix: updated gas report workflow * fix: removed fuzz seed for forge gas diff * fix: added make file driven flow * chore: modified make * chore: testing without piping test output * fix: added fix for the gas-diff workflow * chore: changed the % to report * chore: changed the % to report * chore: changed the gas report name * chore: changed the base to compare to * chore: removed report name * chore: removed report name * chore: added report name * fix: lint * fix: more changes * fix: more changes * fix: fixed removeValidators bug * chore: added more tests * fix: added tests for active operators count * fix: github action correction * fix: limit the fuzzing --------- Signed-off-by: Prafful <[email protected]> Co-authored-by: Mischa <[email protected]>
liquid-collective · May 28, 2024 · c187d39 · c187d39
1 parent a46a4e8
commit c187d39
Show file tree

Hide file tree

Showing 14 changed files with 150 additions and 66 deletions.
diff --git a/.github/workflows/foundry-gas-diff.yml b/.github/workflows/foundry-gas-diff.yml
@@ -0,0 +1,48 @@
+name: Report gas diff
+
+on:
+  push:
+    paths:
+      - "contracts/**"
+      - "lib/**"
+      - ".github/**"
+
+jobs:
+  compare_gas_reports:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          submodules: recursive
+
+      - name: Install Foundry
+        uses: onbjerg/foundry-toolchain@v1
+        with:
+          version: nightly
+
+      # Add any step generating a gas report to a temporary file named gasreport.ansi. For example:
+      - name: Run tests
+        run: forge test --gas-report > gasreport.ansi
+        env:
+          # make fuzzing semi-deterministic to avoid noisy gas cost estimation
+          # due to non-deterministic fuzzing (but still use pseudo-random fuzzing seeds)
+          FOUNDRY_FUZZ_SEED: 0x${{ github.event.pull_request.base.sha || github.sha }}
+          MAINNET_FORK_RPC_URL: ${{ secrets.MAINNET_FORK_RPC_URL }}
+
+      - name: Compare gas reports
+        uses: Rubilmax/[email protected]
+        with:
+          summaryQuantile: 0.01 # only display the 10% most significant gas diffs in the summary (defaults to 20%)
+          sortCriteria: avg,max # sort diff rows by criteria
+          sortOrders: desc,asc # and directions
+          ignore: test-foundry/**/*,test/**/* # filter out gas reports from specific paths (test/ is included by default)
+          base: main # branch to compare against (defaults to current branch)
+        id: gas_diff
+
+      - name: Add gas diff to sticky comment
+        if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
+        uses: marocchino/sticky-pull-request-comment@v2
+        with:
+          # delete the comment in case changes no longer impact gas costs
+          delete: ${{ !steps.gas_diff.outputs.markdown }}
+          message: ${{ steps.gas_diff.outputs.markdown }}
diff --git a/Makefile b/Makefile
@@ -34,4 +34,3 @@ artifacts-mainnet:
 artifacts-holesky:
 	yarn hh run gen_root_artifacts.ts --network holesky
 	yarn hh run gen_meta_artifacts.ts --network holesky
-
diff --git a/contracts/src/Allowlist.1.sol b/contracts/src/Allowlist.1.sol
@@ -94,15 +94,17 @@ contract AllowlistV1 is IAllowlistV1, Initializable, Administrable {
             revert LibErrors.Unauthorized(msg.sender);
         }
 
-        if (_accounts.length == 0) {
+        uint256 accountsLength = _accounts.length;
+
+        if (accountsLength == 0) {
             revert InvalidCount();
         }
 
-        if (_accounts.length != _permissions.length) {
+        if (accountsLength != _permissions.length) {
             revert MismatchedArrayLengths();
         }
 
-        for (uint256 i = 0; i < _accounts.length;) {
+        for (uint256 i = 0; i < accountsLength;) {
             LibSanitize._notZeroAddress(_accounts[i]);
 
             // Check if account is already denied
@@ -130,15 +132,17 @@ contract AllowlistV1 is IAllowlistV1, Initializable, Administrable {
             revert LibErrors.Unauthorized(msg.sender);
         }
 
-        if (_accounts.length == 0) {
+        uint256 accountsLength = _accounts.length;
+
+        if (accountsLength == 0) {
             revert InvalidCount();
         }
 
-        if (_accounts.length != _permissions.length) {
+        if (accountsLength != _permissions.length) {
             revert MismatchedArrayLengths();
         }
 
-        for (uint256 i = 0; i < _accounts.length;) {
+        for (uint256 i = 0; i < accountsLength;) {
             LibSanitize._notZeroAddress(_accounts[i]);
             if (_permissions[i] & LibAllowlistMasks.DENY_MASK == LibAllowlistMasks.DENY_MASK) {
                 // Apply deny mask

diff --git a/contracts/src/Firewall.sol b/contracts/src/Firewall.sol
@@ -19,7 +19,7 @@ contract Firewall is IFirewall, Administrable {
     address public executor;
 
     /// @inheritdoc IFirewall
-    address public destination;
+    address public immutable destination;
 
     /// @inheritdoc IFirewall
     mapping(bytes4 => bool) public executorCanCall;

diff --git a/contracts/src/OperatorsRegistry.1.sol b/contracts/src/OperatorsRegistry.1.sol
@@ -91,7 +91,9 @@ contract OperatorsRegistryV1 is IOperatorsRegistryV1, Initializable, Administrab
                 if (operatorIndex == OperatorsV2.getCount() - 1) {
                     operatorIndex = type(uint256).max;
                 } else {
-                    ++operatorIndex;
+                    unchecked {
+                        ++operatorIndex;
+                    }
                 }
             } else {
                 keyIndex += publicKeys.length;
@@ -272,13 +274,14 @@ contract OperatorsRegistryV1 is IOperatorsRegistryV1, Initializable, Administrab
         uint32[] calldata _newLimits,
         uint256 _snapshotBlock
     ) external onlyAdmin {
-        if (_operatorIndexes.length != _newLimits.length) {
+        uint256 _operatorIndexesLength = _operatorIndexes.length;
+        if (_operatorIndexesLength != _newLimits.length) {
             revert InvalidArrayLengths();
         }
-        if (_operatorIndexes.length == 0) {
+        if (_operatorIndexesLength == 0) {
             revert InvalidEmptyArray();
         }
-        for (uint256 idx = 0; idx < _operatorIndexes.length;) {
+        for (uint256 idx = 0; idx < _operatorIndexesLength;) {
             uint256 operatorIndex = _operatorIndexes[idx];
             uint32 newLimit = _newLimits[idx];
 
@@ -347,19 +350,19 @@ contract OperatorsRegistryV1 is IOperatorsRegistryV1, Initializable, Administrab
         }
 
         OperatorsV2.Operator storage operator = OperatorsV2.get(_index);
-
+        uint256 totalKeys = uint256(operator.keys);
         for (uint256 idx = 0; idx < _keyCount;) {
             bytes memory publicKeyAndSignature = LibBytes.slice(
                 _publicKeysAndSignatures,
                 idx * (ValidatorKeys.PUBLIC_KEY_LENGTH + ValidatorKeys.SIGNATURE_LENGTH),
                 ValidatorKeys.PUBLIC_KEY_LENGTH + ValidatorKeys.SIGNATURE_LENGTH
             );
-            ValidatorKeys.set(_index, operator.keys + idx, publicKeyAndSignature);
+            ValidatorKeys.set(_index, totalKeys + idx, publicKeyAndSignature);
             unchecked {
                 ++idx;
             }
         }
-        OperatorsV2.setKeys(_index, operator.keys + _keyCount);
+        OperatorsV2.setKeys(_index, uint32(totalKeys) + _keyCount);
 
         emit AddedValidatorKeys(_index, _publicKeysAndSignatures);
     }
@@ -385,7 +388,7 @@ contract OperatorsRegistryV1 is IOperatorsRegistryV1, Initializable, Administrab
             revert InvalidFundedKeyDeletionAttempt();
         }
 
-        bool limitEqualsKeyCount = operator.keys == operator.limit;
+        bool limitEqualsKeyCount = totalKeys == operator.limit;
         OperatorsV2.setKeys(_index, totalKeys - uint32(indexesLength));
 
         uint256 idx;
@@ -682,7 +685,7 @@ contract OperatorsRegistryV1 is IOperatorsRegistryV1, Initializable, Administrab
         (OperatorsV2.CachedOperator[] memory operators, uint256 fundableOperatorCount) = OperatorsV2.getAllFundable();
 
         if (fundableOperatorCount == 0) {
-            return (new bytes[](0), new bytes[](0));
+            return (publicKeys, signatures);
         }
 
         while (_count > 0) {
@@ -783,7 +786,9 @@ contract OperatorsRegistryV1 is IOperatorsRegistryV1, Initializable, Administrab
                 uint32 activeCount = _getActiveValidatorCountForExitRequests(operators[idx]);
 
                 if (activeCount == highestActiveCount) {
-                    ++siblings;
+                    unchecked {
+                        ++siblings;
+                    }
                 } else if (activeCount > highestActiveCount) {
                     secondHighestActiveCount = highestActiveCount;
                     highestActiveCount = activeCount;
@@ -814,7 +819,9 @@ contract OperatorsRegistryV1 is IOperatorsRegistryV1, Initializable, Administrab
                     uint32 additionalRequestedExits = baseExitRequestAmount + (rest > 0 ? 1 : 0);
                     operators[idx].picked += additionalRequestedExits;
                     if (rest > 0) {
-                        --rest;
+                        unchecked {
+                            --rest;
+                        }
                     }
                 }
                 unchecked {

diff --git a/contracts/src/RedeemManager.1.sol b/contracts/src/RedeemManager.1.sol
@@ -117,7 +117,9 @@ contract RedeemManagerV1 is Initializable, IRedeemManagerV1 {
         WithdrawalStack.WithdrawalEvent[] storage withdrawalEvents = WithdrawalStack.get();
         uint256 withdrawalEventsLength = withdrawalEvents.length;
         if (withdrawalEventsLength > 0) {
-            lastWithdrawalEvent = withdrawalEvents[withdrawalEventsLength - 1];
+            unchecked {
+                lastWithdrawalEvent = withdrawalEvents[withdrawalEventsLength - 1];
+            }
         }
         for (uint256 idx = 0; idx < _redeemRequestIds.length; ++idx) {
             withdrawalEventIds[idx] = _resolveRedeemRequestId(_redeemRequestIds[idx], lastWithdrawalEvent);
@@ -173,7 +175,9 @@ contract RedeemManagerV1 is Initializable, IRedeemManagerV1 {
         withdrawalEvents.push(
             WithdrawalStack.WithdrawalEvent({height: height, amount: _lsETHWithdrawable, withdrawnEth: msgValue})
         );
-        _setRedeemDemand(redeemDemand - _lsETHWithdrawable);
+        unchecked {
+            _setRedeemDemand(redeemDemand - _lsETHWithdrawable);
+        }
         emit ReportedWithdrawal(height, _lsETHWithdrawable, msgValue, withdrawalEventId);
     }
 
@@ -318,14 +322,14 @@ contract RedeemManagerV1 is Initializable, IRedeemManagerV1 {
 
     /// @notice Internal structure used to optimize stack usage in _claimRedeemRequest
     struct ClaimRedeemRequestParameters {
-        /// @custom:attribute The id of the redeem request to claim
-        uint32 redeemRequestId;
         /// @custom:attribute The structure of the redeem request to claim
         RedeemQueue.RedeemRequest redeemRequest;
-        /// @custom:attribute The id of the withdrawal event to use to claim the redeem request
-        uint32 withdrawalEventId;
         /// @custom:attribute The structure of the withdrawal event to use to claim the redeem request
         WithdrawalStack.WithdrawalEvent withdrawalEvent;
+        /// @custom:attribute The id of the redeem request to claim
+        uint32 redeemRequestId;
+        /// @custom:attribute The id of the withdrawal event to use to claim the redeem request
+        uint32 withdrawalEventId;
         /// @custom:attribute The count of withdrawal events
         uint32 withdrawalEventCount;
         /// @custom:attribute The current depth of the recursive call
@@ -377,7 +381,9 @@ contract RedeemManagerV1 is Initializable, IRedeemManagerV1 {
                 (vars.matchingAmount * _params.redeemRequest.maxRedeemableEth) / _params.redeemRequest.amount;
 
             if (maxRedeemableEthAmount < vars.ethAmount) {
-                vars.exceedingEthAmount = vars.ethAmount - maxRedeemableEthAmount;
+                unchecked {
+                    vars.exceedingEthAmount = vars.ethAmount - maxRedeemableEthAmount;
+                }
                 BufferedExceedingEth.set(BufferedExceedingEth.get() + vars.exceedingEthAmount);
                 vars.ethAmount = maxRedeemableEthAmount;
             }
@@ -418,9 +424,13 @@ contract RedeemManagerV1 is Initializable, IRedeemManagerV1 {
         ) {
             WithdrawalStack.WithdrawalEvent[] storage withdrawalEvents = WithdrawalStack.get();
 
-            ++_params.withdrawalEventId;
+            unchecked {
+                ++_params.withdrawalEventId;
+            }
             _params.withdrawalEvent = withdrawalEvents[_params.withdrawalEventId];
-            --_params.depth;
+            unchecked {
+                --_params.depth;
+            }
 
             _claimRedeemRequest(_params);
         } else {

diff --git a/contracts/src/River.1.sol b/contracts/src/River.1.sol
@@ -117,8 +117,10 @@ contract RiverV1 is
         // force committed balance to a multiple of 32 ETH and
         // move extra funds back to the deposit buffer
         uint256 dustToUncommit = CommittedBalance.get() % DEPOSIT_SIZE;
-        _setCommittedBalance(CommittedBalance.get() - dustToUncommit);
-        _setBalanceToDeposit(BalanceToDeposit.get() + dustToUncommit);
+        unchecked {
+            _setCommittedBalance(CommittedBalance.get() - dustToUncommit);
+            _setBalanceToDeposit(BalanceToDeposit.get() + dustToUncommit);
+        }
     }
 
     /// @inheritdoc IRiverV1
@@ -304,10 +306,8 @@ contract RiverV1 is
     function _onDeposit(address _depositor, address _recipient, uint256 _amount) internal override {
         uint256 mintedShares = SharesManagerV1._mintShares(_depositor, _amount);
         IAllowlistV1 allowlist = IAllowlistV1(AllowlistAddress.get());
-        if (_depositor == _recipient) {
-            allowlist.onlyAllowed(_depositor, LibAllowlistMasks.DEPOSIT_MASK); // this call reverts if unauthorized or denied
-        } else {
-            allowlist.onlyAllowed(_depositor, LibAllowlistMasks.DEPOSIT_MASK); // this call reverts if unauthorized or denied
+        allowlist.onlyAllowed(_depositor, LibAllowlistMasks.DEPOSIT_MASK); // this call reverts if unauthorized or denied
+        if (_depositor != _recipient) {
             if (allowlist.isDenied(_recipient)) {
                 revert Denied(_recipient);
             }
@@ -484,7 +484,9 @@ contract RiverV1 is
 
             if (suppliedRedeemManagerDemandInEth > 0) {
                 // the available balance to redeem is updated
-                _setBalanceToRedeem(availableBalanceToRedeem - suppliedRedeemManagerDemandInEth);
+                unchecked {
+                    _setBalanceToRedeem(availableBalanceToRedeem - suppliedRedeemManagerDemandInEth);
+                }
 
                 // we burn the shares of the redeem manager associated with the amount of eth provided
                 _burnRawShares(address(redeemManager_), suppliedRedeemManagerDemand);

diff --git a/contracts/src/components/SharesManager.1.sol b/contracts/src/components/SharesManager.1.sol
@@ -157,7 +157,9 @@ abstract contract SharesManagerV1 is ISharesManagerV1 {
             revert AllowanceTooLow(_from, msg.sender, currentAllowance, _value);
         }
         if (currentAllowance != type(uint256).max) {
-            _approve(_from, msg.sender, currentAllowance - _value);
+            unchecked {
+                _approve(_from, msg.sender, currentAllowance - _value);
+            }
         }
     }
 

diff --git a/contracts/src/libraries/LibUint256.sol b/contracts/src/libraries/LibUint256.sol
@@ -8,7 +8,6 @@ library LibUint256 {
     /// @param _value The value to convert
     /// @return result The converted value
     function toLittleEndian64(uint256 _value) internal pure returns (uint256 result) {
-        result = 0;
         uint256 tempValue = _value;
         result = tempValue & 0xFF;
         tempValue >>= 8;

diff --git a/contracts/src/state/operatorsRegistry/Operators.2.sol b/contracts/src/state/operatorsRegistry/Operators.2.sol
@@ -132,9 +132,11 @@ library OperatorsV2 {
 
         uint256 activeCount = 0;
         uint256 operatorCount = r.value.length;
+        Operator[] memory activeOperators = new Operator[](operatorCount);
 
         for (uint256 idx = 0; idx < operatorCount;) {
             if (r.value[idx].active) {
+                activeOperators[activeCount] = r.value[idx];
                 unchecked {
                     ++activeCount;
                 }
@@ -143,20 +145,8 @@ library OperatorsV2 {
                 ++idx;
             }
         }
-
-        Operator[] memory activeOperators = new Operator[](activeCount);
-
-        uint256 activeIdx = 0;
-        for (uint256 idx = 0; idx < operatorCount;) {
-            if (r.value[idx].active) {
-                activeOperators[activeIdx] = r.value[idx];
-                unchecked {
-                    ++activeIdx;
-                }
-            }
-            unchecked {
-                ++idx;
-            }
+        assembly ("memory-safe") {
+            mstore(activeOperators, activeCount)
         }
 
         return activeOperators;

diff --git a/contracts/src/state/oracle/OracleMembers.sol b/contracts/src/state/oracle/OracleMembers.sol
@@ -77,7 +77,9 @@ library OracleMembers {
             r.slot := slot
         }
 
-        for (uint256 idx = 0; idx < r.value.length;) {
+        uint256 length = r.value.length;
+
+        for (uint256 idx = 0; idx < length;) {
             if (r.value[idx] == _memberAddress) {
                 return int256(idx);
             }

diff --git a/contracts/src/state/oracle/ReportsVariants.sol b/contracts/src/state/oracle/ReportsVariants.sol
@@ -77,7 +77,9 @@ library ReportsVariants {
             r.slot := slot
         }
 
-        for (uint256 idx = 0; idx < r.value.length;) {
+        uint256 length = r.value.length;
+
+        for (uint256 idx = 0; idx < length;) {
             if (r.value[idx].variant == _variant) {
                 return int256(idx);
             }