Gas optimizations from the Code4rena audit (#18)

* Check that withdrawAmt > maxCost before updating mapping. Per code-423n4/2024-03-coinbase-findings#195, G04 * Switch validation checks for gas optimization. Per code-423n4/2024-03-coinbase-findings#195 G06 * Reorder validations in withdraw to check signature before validating the full request. Per code-423n4/2024-03-coinbase-findings#137 G02 * Added gas snapshot * Formatted * Remove balance check before updating withdrawable mapping, rerun snapshot
coinbase · Apr 4, 2024 · 8592b5f · 8592b5f
1 parent 0041c9d
commit 8592b5f
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 18 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
@@ -4,35 +4,35 @@ IsValidWithdrawalSignature:test_returnsTrueWithValidSignature() (gas: 51016)
 OwnerWithdrawTest:test_revertsIfNotOwner() (gas: 15305)
 OwnerWithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 97271, ~: 99711)
 OwnerWithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 51418, ~: 53415)
-PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97259, ~: 99371)
+PostOpTest:test_RevertsIfPostOpFailed(uint256,uint256,uint256) (runs: 256, μ: 97243, ~: 99371)
 PostOpTest:test_entryPointAddStake(uint112,uint32) (runs: 256, μ: 56233, ~: 56233)
 PostOpTest:test_entryPointDeposit(uint112) (runs: 256, μ: 32479, ~: 32962)
 PostOpTest:test_entryPointUnlockStake() (gas: 54888)
-PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63050, ~: 65300)
+PostOpTest:test_entryPointWithdraw(uint112) (runs: 256, μ: 63200, ~: 65300)
 PostOpTest:test_entryPointWithdrawStake() (gas: 71803)
 PostOpTest:test_paymasterPaysForOp() (gas: 210663)
-PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112442, ~: 113477)
+PostOpTest:test_transfersExcess(uint256,uint256,uint256,uint256) (runs: 256, μ: 112479, ~: 113488)
 SetMaxWithdrawDenominator:test_emitsCorrectly(uint256) (runs: 256, μ: 19851, ~: 20191)
 SetMaxWithdrawDenominator:test_reverts_whenNotCalledByOwner() (gas: 12257)
 SetMaxWithdrawDenominator:test_setsMaxWithdrawPercent(uint256) (runs: 256, μ: 19209, ~: 19549)
 Simulate:test() (gas: 12114)
-ValidatePaymasterUserOpTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 104847, ~: 106080)
+ValidatePaymasterUserOpTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 104697, ~: 106080)
 ValidatePaymasterUserOpTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 112158, ~: 113247)
 ValidatePaymasterUserOpTest:test_returns1sIfWrongSignature() (gas: 96298)
 ValidatePaymasterUserOpTest:test_returnsCorrectly() (gas: 91185)
 ValidatePaymasterUserOpTest:test_revertsIfMaxCostMoreThanRequested() (gas: 39141)
 ValidatePaymasterUserOpTest:test_revertsIfNonceUsed() (gas: 103502)
 ValidatePaymasterUserOpTest:test_revertsIfWithdrawAssetNotZero() (gas: 55952)
-ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100806, ~: 100992)
+ValidatePaymasterUserOpTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 100830, ~: 100992)
 ValidateTest:test_receive() (gas: 14687)
 WithdrawGasExcess:test_RevertsIfNoExcess(uint256) (runs: 256, μ: 71133, ~: 71396)
 WithdrawGasExcess:test_transferExcess(uint256,uint256,uint256) (runs: 256, μ: 104290, ~: 104290)
-WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118270, ~: 121368)
-WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 162427, ~: 164773)
-WithdrawTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 111372, ~: 112461)
-WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 61664, ~: 61785)
-WithdrawTest:test_revertsIfNonceUsed() (gas: 95844)
-WithdrawTest:test_revertsIfWrongSignature() (gas: 63830)
-WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 85119, ~: 85096)
-WithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 140626, ~: 143066)
-WithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 91692, ~: 93689)
+WithdrawTest:test_emitsCorrectly(address,uint256,uint256) (runs: 256, μ: 118018, ~: 121375)
+WithdrawTest:test_emitsCorrectlyERC20Withdraw(address,uint256,uint256) (runs: 256, μ: 161748, ~: 164780)
+WithdrawTest:test_recordsNonceUsed(uint256) (runs: 256, μ: 111379, ~: 112468)
+WithdrawTest:test_revertsIfExpired(uint48,uint256) (runs: 256, μ: 28345, ~: 28455)
+WithdrawTest:test_revertsIfNonceUsed() (gas: 100227)
+WithdrawTest:test_revertsIfWrongSignature() (gas: 36875)
+WithdrawTest:test_reverts_whenWithdrawExceedsMaxAllowed(uint256,uint256,uint256) (runs: 256, μ: 89538, ~: 89472)
+WithdrawTest:test_transfersERC20Successfully(uint256) (runs: 256, μ: 140633, ~: 143073)
+WithdrawTest:test_transfersETHSuccessfully(uint256) (runs: 256, μ: 91699, ~: 93696)
diff --git a/src/MagicSpend.sol b/src/MagicSpend.sol
@@ -188,15 +188,15 @@ contract MagicSpend is Ownable, IPaymaster {
     ///
     /// @param withdrawRequest The withdraw request.
     function withdraw(WithdrawRequest memory withdrawRequest) external {
-        _validateRequest(msg.sender, withdrawRequest);
+        if (block.timestamp > withdrawRequest.expiry) {
+            revert Expired();
+        }
 
         if (!isValidWithdrawSignature(msg.sender, withdrawRequest)) {
             revert InvalidSignature();
         }
 
-        if (block.timestamp > withdrawRequest.expiry) {
-            revert Expired();
-        }
+        _validateRequest(msg.sender, withdrawRequest);
 
         // reserve funds for gas, will credit user with difference in post op
         _withdraw(withdrawRequest.asset, msg.sender, withdrawRequest.amount);