Fix EntryPoint revert if postOp reverts with short revert reason (#308)

* Makes comments and formatting consistent * @inheritdoc IEntryPoint * Reverts contracts/test/TestHelpers.sol * fix: entrypoint does not revert even first postOp reverts with short revert reason * lint * gas-report * Update IEntryPoint.sol - add missing 'INonceManager' * Update EntryPoint.sol - fix lint errors * Update gas-checker.txt manually * Update gas-checker.txt - add newline for the 'diff' * Update gas-checker.txt manually --------- Co-authored-by: Matt Masurka <[email protected]> Co-authored-by: leekt <[email protected]>
eth-infinitism · Aug 15, 2023 · 7f6c103 · 7f6c103
1 parent f2b09e6
commit 7f6c103
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 15 deletions.
diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol
@@ -72,8 +72,11 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard
         } catch {
             bytes32 innerRevertCode;
             assembly {
-                returndatacopy(0, 0, 32)
-                innerRevertCode := mload(0)
+                let len := returndatasize()
+                if eq(32,len) {
+                    returndatacopy(0, 0, 32)
+                    innerRevertCode := mload(0)
+                }
             }
             // handleOps was called with gas limit too low. abort entire bundle.
             if (innerRevertCode == INNER_OUT_OF_GAS) {

diff --git a/contracts/test/TestPaymasterRevertCustomError.sol b/contracts/test/TestPaymasterRevertCustomError.sol
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-3.0-only
+pragma solidity ^0.8.12;
+
+import "../core/BasePaymaster.sol";
+
+/**
+ * test postOp revert with custom error
+ */
+error CustomError();
+
+contract TestPaymasterRevertCustomError is BasePaymaster {
+    // solhint-disable no-empty-blocks
+    constructor(IEntryPoint _entryPoint) BasePaymaster(_entryPoint)
+    {}
+
+    function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32, uint256)
+    internal virtual override view
+    returns (bytes memory context, uint256 validationData) {
+        validationData = 0;
+        context = abi.encodePacked(userOp.sender);
+    }
+
+    function _postOp(PostOpMode mode, bytes calldata, uint256) internal override {
+        if(mode == PostOpMode.postOpReverted) {
+            return;
+        }
+
+        revert CustomError();
+    }
+}
diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt
@@ -12,28 +12,28 @@
 ║                                │       │               │     (delta for │        (compared to ║
 ║                                │       │               │    one UserOp) │     account.exec()) ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │     1 │         81891 │                │                     ║
+║ simple                         │     1 │         81921 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │     2 │               │          44154 │               15140 ║
+║ simple - diff from previous    │     2 │               │          44208 │               15194 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │    10 │        479586 │                │                     ║
+║ simple                         │    10 │        479922 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │    11 │               │          44250 │               15236 ║
+║ simple - diff from previous    │    11 │               │          44244 │               15230 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │     1 │         88162 │                │                     ║
+║ simple paymaster               │     1 │         88192 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │     2 │               │          43143 │               14129 ║
+║ simple paymaster with diff     │     2 │               │          43197 │               14183 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │    10 │        476798 │                │                     ║
+║ simple paymaster               │    10 │        477170 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │    11 │               │          43214 │               14200 ║
+║ simple paymaster with diff     │    11 │               │          43220 │               14206 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │     1 │        182948 │                │                     ║
+║ big tx 5k                      │     1 │        182978 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │     2 │               │         144689 │               19429 ║
+║ big tx - diff from previous    │     2 │               │         144683 │               19423 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │    10 │       1485278 │                │                     ║
+║ big tx 5k                      │    10 │       1485506 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │    11 │               │         144750 │               19490 ║
+║ big tx - diff from previous    │    11 │               │         144744 │               19484 ║
 ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝
 
diff --git a/test/entrypoint.test.ts b/test/entrypoint.test.ts
@@ -20,7 +20,8 @@ import {
   TestSignatureAggregator,
   TestSignatureAggregator__factory,
   MaliciousAccount__factory,
-  TestWarmColdAccount__factory
+  TestWarmColdAccount__factory,
+  TestPaymasterRevertCustomError__factory
 } from '../typechain'
 import {
   AddressZero,
@@ -1171,6 +1172,24 @@ describe('EntryPoint', function () {
         await expect(entryPoint.handleOps([op], beneficiaryAddress)).to.revertedWith('"AA31 paymaster deposit too low"')
       })
 
+      it('should not revert when paymaster reverts with custom error on postOp', async function () {
+        const account3Owner = createAccountOwner()
+        const errorPostOp = await new TestPaymasterRevertCustomError__factory(ethersSigner).deploy(entryPoint.address)
+        await errorPostOp.addStake(globalUnstakeDelaySec, { value: paymasterStake })
+        await errorPostOp.deposit({ value: ONE_ETH })
+
+        const op = await fillAndSign({
+          paymasterAndData: errorPostOp.address,
+          callData: accountExecFromEntryPoint.data,
+          initCode: getAccountInitCode(account3Owner.address, simpleAccountFactory),
+
+          verificationGasLimit: 3e6,
+          callGasLimit: 1e6
+        }, account3Owner, entryPoint)
+        const beneficiaryAddress = createAddress()
+        await entryPoint.handleOps([op], beneficiaryAddress)
+      })
+
       it('paymaster should pay for tx', async function () {
         await paymaster.deposit({ value: ONE_ETH })
         const op = await fillAndSign({