AA-184: Remove second postOp retry (#311)

* Remove "postOp" retry call mode and adjust tests

* Use up to two times 'verificationGasLimit' in the 'requiredGas' calculation

* Bump node version for Github Actions

* Disable new solhint rules

* Uncomment TestCounter deployment

.github/workflows/build.yml

@@ -19,7 +19,7 @@ jobs:
     steps:
       - uses: actions/setup-node@v1
         with:
-          node-version: '14'
+          node-version: '16'
       - uses: actions/checkout@v1
       - uses: actions/cache@v2
         with:
@@ -40,7 +40,7 @@ jobs:
     steps:
       - uses: actions/setup-node@v1
         with:
-          node-version: '14'
+          node-version: '16'
       - uses: actions/checkout@v1
       - uses: actions/cache@v2
         with:
@@ -55,7 +55,7 @@ jobs:
     steps:
       - uses: actions/setup-node@v1
         with:
-          node-version: '14'
+          node-version: '16'
       - uses: actions/checkout@v1
       - uses: actions/cache@v2
         with:
@@ -69,7 +69,7 @@ jobs:
     steps:
       - uses: actions/setup-node@v1
         with:
-          node-version: '14'
+          node-version: '16'
       - uses: actions/checkout@v1
       - uses: actions/cache@v2
         with:

.solhint.json

@@ -3,6 +3,10 @@
   "rules": {
   "compiler-version": ["error",">=0.7.5"],
     "func-visibility": ["off",{"ignoreConstructors":true}],
+    "custom-errors": ["off"],
+    "explicit-types": ["off"],
+    "no-global-import": ["off"],
+    "immutable-vars-naming": ["off"],
     "mark-callable-contracts": ["off"]
   }
 }

contracts/core/EntryPoint.sol

@@ -100,7 +100,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
             }
 
             uint256 actualGas = preGas - gasleft() + opInfo.preOpGas;
-            collected = _handlePostOp(
+            collected = _postExecution(
                 opIndex,
                 IPaymaster.PostOpMode.postOpReverted,
                 opInfo,
@@ -290,7 +290,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
         unchecked {
             uint256 actualGas = preGas - gasleft() + opInfo.preOpGas;
             // Note: opIndex is ignored (relevant only if mode==postOpReverted, which is only possible outside of innerHandleOp)
-            return _handlePostOp(0, mode, opInfo, context, actualGas);
+            return _postExecution(0, mode, opInfo, context, actualGas);
         }
     }
 
@@ -340,7 +340,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
         unchecked {
             // When using a Paymaster, the verificationGasLimit is used also to as a limit for the postOp call.
             // Our security model might call postOp eventually twice.
-            uint256 mul = mUserOp.paymaster != address(0) ? 3 : 1;
+            uint256 mul = mUserOp.paymaster != address(0) ? 2 : 1;
             uint256 requiredGas = mUserOp.callGasLimit +
                 mUserOp.verificationGasLimit *
                 mul +
@@ -640,7 +640,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
      * @param context   - The context returned in validatePaymasterUserOp.
      * @param actualGas - The gas used so far by this user operation.
      */
-    function _handlePostOp(
+    function _postExecution(
         uint256 opIndex,
         IPaymaster.PostOpMode mode,
         UserOpInfo memory opInfo,
@@ -664,20 +664,6 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
                         IPaymaster(paymaster).postOp{
                             gas: mUserOp.verificationGasLimit
                         }(mode, context, actualGasCost);
-                    } else {
-                        try
-                            IPaymaster(paymaster).postOp{
-                                gas: mUserOp.verificationGasLimit
-                            }(mode, context, actualGasCost)
-                        // solhint-disable-next-line no-empty-blocks
-                        {} catch Error(string memory reason) {
-                            revert FailedOp(
-                                opIndex,
-                                string.concat("AA50 postOp reverted: ", reason)
-                            );
-                        } catch {
-                            revert FailedOp(opIndex, "AA50 postOp revert");
-                        }
                     }
                 }
             }

contracts/interfaces/IPaymaster.sol

@@ -14,7 +14,7 @@ interface IPaymaster {
         // User op reverted. Still has to pay for gas.
         opReverted,
         // User op succeeded, but caused postOp to revert.
-        // Now it's a 2nd call, after user's op was deliberately reverted.
+        // Only used internally in the EntryPoint - Paymasters will not be called again.
         postOpReverted
     }
 

contracts/samples/DepositPaymaster.sol

@@ -151,18 +151,13 @@ contract DepositPaymaster is BasePaymaster {
      * this time in *postOpReverted* mode.
      * In this mode, we use the deposit to pay (which we validated to be large enough)
      */
-    function _postOp(PostOpMode mode, bytes calldata context, uint256 actualGasCost) internal override {
+    function _postOp(PostOpMode, bytes calldata context, uint256 actualGasCost) internal override {
 
         (address account, IERC20 token, uint256 gasPricePostOp, uint256 maxTokenCost, uint256 maxCost) = abi.decode(context, (address, IERC20, uint256, uint256, uint256));
         //use same conversion rate as used for validation.
         uint256 actualTokenCost = (actualGasCost + COST_OF_POST * gasPricePostOp) * maxTokenCost / maxCost;
-        if (mode != PostOpMode.postOpReverted) {
-            // attempt to pay with tokens:
-            token.safeTransferFrom(account, address(this), actualTokenCost);
-        } else {
-            //in case above transferFrom failed, pay with deposit:
-            balances[token][account] -= actualTokenCost;
-        }
+        // attempt to pay with tokens:
+        token.safeTransferFrom(account, address(this), actualTokenCost);
         balances[token][owner()] += actualTokenCost;
     }
 }

contracts/samples/TokenPaymaster.sol

@@ -150,10 +150,9 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper {
 
     /// @notice Performs post-operation tasks, such as updating the token price and refunding excess tokens.
     /// @dev This function is called after a user operation has been executed or reverted.
-    /// @param mode The post-operation mode (either successful or reverted).
     /// @param context The context containing the token amount and user sender address.
     /// @param actualGasCost The actual gas cost of the transaction.
-    function _postOp(PostOpMode mode, bytes calldata context, uint256 actualGasCost) internal override {
+    function _postOp(PostOpMode, bytes calldata context, uint256 actualGasCost) internal override {
         unchecked {
             uint256 priceMarkup = tokenPaymasterConfig.priceMarkup;
             (
@@ -163,11 +162,6 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper {
                 address userOpSender
             ) = abi.decode(context, (uint256, uint256, uint256, address));
             uint256 gasPrice = getGasPrice(maxFeePerGas, maxPriorityFeePerGas);
-            if (mode == PostOpMode.postOpReverted) {
-                emit PostOpReverted(userOpSender, preCharge);
-                // Do nothing here to not revert the whole bundle and harm reputation
-                return;
-            }
             uint256 _cachedPrice = updateCachedPrice(false);
         // note: as price is in ether-per-token and we want more tokens increasing it means dividing it by markup
             uint256 cachedPriceWithMarkup = _cachedPrice * PRICE_DENOMINATOR / priceMarkup;

contracts/test/BrokenBlsAccount.sol

@@ -1,4 +1,6 @@
 // SPDX-License-Identifier: GPL-3.0
+
+/* solhint-disable one-contract-per-file */
 pragma solidity ^0.8.12;
 
 import "@openzeppelin/contracts/utils/Create2.sol";

contracts/test/TestPaymasterRevertCustomError.sol

@@ -20,10 +20,7 @@ contract TestPaymasterRevertCustomError is BasePaymaster {
         context = abi.encodePacked(userOp.sender);
     }
 
-    function _postOp(PostOpMode mode, bytes calldata, uint256) internal pure override {
-        if(mode == PostOpMode.postOpReverted) {
-            return;
-        }
+    function _postOp(PostOpMode, bytes calldata, uint256) internal pure override {
 
         revert CustomError();
     }

deploy/1_deploy_entrypoint.ts

@@ -16,12 +16,12 @@ const deployEntryPoint: DeployFunction = async function (hre: HardhatRuntimeEnvi
       deterministicDeployment: true
     })
   console.log('==entrypoint addr=', ret.address)
-/*
+
   const entryPointAddress = ret.address
   const w = await hre.deployments.deploy(
     'SimpleAccount', {
       from,
-      args: [entryPointAddress, from],
+      args: [entryPointAddress],
       gasLimit: 2e6,
       deterministicDeployment: true
     })
@@ -33,7 +33,6 @@ const deployEntryPoint: DeployFunction = async function (hre: HardhatRuntimeEnvi
     deterministicDeployment: true
   })
   console.log('==testCounter=', t.address)
-  */
 }
 
 export default deployEntryPoint

hardhat.config.ts

@@ -59,7 +59,7 @@ const config: HardhatUserConfig = {
   mocha: {
     timeout: 10000
   },
-
+  // @ts-ignore
   etherscan: {
     apiKey: process.env.ETHERSCAN_API_KEY
   }

reports/gas-checker.txt

@@ -4,36 +4,36 @@
 ╔══════════════════════════╤════════╗
 ║ gas estimate "simple"    │  29014 ║
 ╟──────────────────────────┼────────╢
-║ gas estimate "big tx 5k" │ 125248 ║
+║ gas estimate "big tx 5k" │ 125260 ║
 ╚══════════════════════════╧════════╝
 
 ╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗
 ║ handleOps description          │ count │ total gasUsed │ per UserOp gas │ per UserOp overhead ║
 ║                                │       │               │     (delta for │        (compared to ║
 ║                                │       │               │    one UserOp) │     account.exec()) ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │     1 │         81954 │                │                     ║
+║ simple                         │     1 │         81838 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │     2 │               │          44187 │               15173 ║
+║ simple - diff from previous    │     2 │               │          44095 │               15081 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │    10 │        479898 │                │                     ║
+║ simple                         │    10 │        478810 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │    11 │               │          44271 │               15257 ║
+║ simple - diff from previous    │    11 │               │          44131 │               15117 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │     1 │         88237 │                │                     ║
+║ simple paymaster               │     1 │         88109 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │     2 │               │          43200 │               14186 ║
+║ simple paymaster with diff     │     2 │               │          43096 │               14082 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │    10 │        477146 │                │                     ║
+║ simple paymaster               │    10 │        476022 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │    11 │               │          43271 │               14257 ║
+║ simple paymaster with diff     │    11 │               │          43155 │               14141 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │     1 │        182975 │                │                     ║
+║ big tx 5k                      │     1 │        182895 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │     2 │               │         144710 │               19462 ║
+║ big tx - diff from previous    │     2 │               │         144594 │               19334 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │    10 │       1485530 │                │                     ║
+║ big tx 5k                      │    10 │       1484454 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │    11 │               │         144783 │               19535 ║
+║ big tx - diff from previous    │    11 │               │         144619 │               19359 ║
 ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝
 

test/entrypoint.test.ts

@@ -20,12 +20,13 @@ import {
   TestSignatureAggregator__factory,
   MaliciousAccount__factory,
   TestWarmColdAccount__factory,
+  TestPaymasterRevertCustomError__factory,
   IEntryPoint__factory,
   SimpleAccountFactory__factory,
   IStakeManager__factory,
   INonceManager__factory,
   EntryPoint__factory,
-  TestPaymasterRevertCustomError__factory, EntryPoint
+  EntryPoint
 } from '../typechain'
 import {
   AddressZero,

test/paymaster.test.ts

@@ -231,14 +231,13 @@ describe('EntryPoint with paymaster', function () {
           expect(await paymaster.allowance(account2.address, account.address)).to.eq(ethers.constants.MaxUint256)
         })
 
-        it('griefing attempt should cause handleOp to revert', async () => {
+        it('griefing attempt in postOp should cause the execution part of UserOp to revert', async () => {
           // account1 is approved to withdraw going to withdraw account2's balance
 
           const account2Balance = await paymaster.balanceOf(account2.address)
           const transferCost = parseEther('1').sub(account2Balance)
           const withdrawAmount = account2Balance.sub(transferCost.mul(0))
           const withdrawTokens = paymaster.interface.encodeFunctionData('transferFrom', [account2.address, account.address, withdrawAmount])
-          // const withdrawTokens = paymaster.interface.encodeFunctionData('transfer', [account.address, parseEther('0.1')])
           const execFromEntryPoint = account.interface.encodeFunctionData('execute', [paymaster.address, 0, withdrawTokens])
 
           const userOp1 = await fillAndSign({
@@ -255,12 +254,17 @@ describe('EntryPoint with paymaster', function () {
             callGasLimit: 1e6
           }, accountOwner, entryPoint)
 
-          await expect(
-            entryPoint.handleOps([
+          const rcpt =
+            await entryPoint.handleOps([
               userOp1,
               userOp2
             ], beneficiaryAddress)
-          ).to.be.revertedWith('transfer amount exceeds balance')
+
+          const transferEvents = await paymaster.queryFilter(paymaster.filters.Transfer(), rcpt.blockHash)
+          const [log1, log2] = await entryPoint.queryFilter(entryPoint.filters.UserOperationEvent(), rcpt.blockHash)
+          expect(log1.args.success).to.eq(true)
+          expect(log2.args.success).to.eq(false)
+          expect(transferEvents.length).to.eq(2)
         })
       })
     })