AA-467 Fixing actual gas used penalty (eth-infinitism#532)

shahafn · web-flow · commit fe4b2df5f2d9 · 2025-02-06T18:19:13.000+02:00
* Passing actualGasCost with unused execution gas penalty to paymaster

* Fixing verification gl overhead on simulations

* Adding GasCalcPaymasterWithPostOp instead of the one used in tests
diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol
@@ -716,12 +716,20 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
             uint256 gasPrice = getUserOpGasPrice(mUserOp);
 
             address paymaster = mUserOp.paymaster;
+            // Calculating a penalty for unused execution gas
+            {
+                uint256 executionGasUsed = actualGas - opInfo.preOpGas;
+                // this check is required for the gas used within EntryPoint and not covered by explicit gas limits
+                actualGas += _getUnusedGasPenalty(executionGasUsed, mUserOp.callGasLimit);
+            }
+            uint256 postOpUnusedGasPenalty;
             if (paymaster == address(0)) {
                 refundAddress = mUserOp.sender;
             } else {
                 refundAddress = paymaster;
                 if (context.length > 0) {
                     actualGasCost = actualGas * gasPrice;
+                    uint256 postOpPreGas = gasleft();
                     if (mode != IPaymaster.PostOpMode.postOpReverted) {
                         try IPaymaster(paymaster).postOp{
                             gas: mUserOp.paymasterPostOpGasLimit
@@ -732,22 +740,12 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
                             revert PostOpReverted(reason);
                         }
                     }
+                    // Calculating a penalty for unused postOp gas
+                    uint256 postOpGasUsed = postOpPreGas - gasleft();
+                    postOpUnusedGasPenalty = _getUnusedGasPenalty(postOpGasUsed, mUserOp.paymasterPostOpGasLimit);
                 }
             }
-            actualGas += preGas - gasleft();
-
-            // Calculating a penalty for unused execution gas
-            {
-                uint256 executionGasLimit = mUserOp.callGasLimit + mUserOp.paymasterPostOpGasLimit;
-                uint256 executionGasUsed = actualGas - opInfo.preOpGas;
-                // this check is required for the gas used within EntryPoint and not covered by explicit gas limits
-                if (executionGasLimit > executionGasUsed) {
-                    uint256 unusedGas = executionGasLimit - executionGasUsed;
-                    uint256 unusedGasPenalty = (unusedGas * PENALTY_PERCENT) / 100;
-                    actualGas += unusedGasPenalty;
-                }
-            }
-
+            actualGas += preGas - gasleft() + postOpUnusedGasPenalty;
             actualGasCost = actualGas * gasPrice;
             uint256 prefund = opInfo.prefund;
             if (prefund < actualGasCost) {
@@ -818,4 +816,15 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
         (bool success, bytes memory ret) = target.delegatecall(data);
         revert DelegateAndRevert(success, ret);
     }
+
+    function _getUnusedGasPenalty(uint256 gasUsed, uint256 gasLimit) internal pure returns (uint256) {
+        unchecked {
+            if (gasLimit <= gasUsed) {
+                return 0;
+            }
+            uint256 unusedGas = gasLimit - gasUsed;
+            uint256 unusedGasPenalty = (unusedGas * PENALTY_PERCENT) / 100;
+            return unusedGasPenalty;
+        }
+    }
 }
diff --git a/contracts/core/EntryPointSimulations.sol b/contracts/core/EntryPointSimulations.sol
@@ -192,7 +192,7 @@ contract EntryPointSimulations is EntryPoint, IEntryPointSimulations {
 
     //slightly stricter gas limit than the real EntryPoint
     function _getVerificationGasLimit(uint256 verificationGasLimit) internal pure virtual override returns (uint256) {
-        return verificationGasLimit - 300;
+        return verificationGasLimit - 500;
     }
 
 
diff --git a/contracts/test/GasCalcPaymasterWithPostOp.sol b/contracts/test/GasCalcPaymasterWithPostOp.sol
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-3.0-only
+pragma solidity ^0.8.23;
+
+import "./TestPaymasterAcceptAll.sol";
+/* solhint-disable no-empty-blocks */
+
+/**
+ * test paymaster, that pays for everything, without any check.
+ * explicitly returns a context, to test cost (for entrypoint) to call postOp
+ */
+contract GasCalcPaymasterWithPostOp is TestPaymasterAcceptAll {
+    constructor(IEntryPoint _entryPoint) TestPaymasterAcceptAll(_entryPoint) {
+    }
+
+    function _validatePaymasterUserOp(PackedUserOperation calldata, bytes32, uint256)
+    internal virtual override view
+    returns (bytes memory context, uint256 validationData) {
+        // return a context, to force a call for postOp.
+        return ("1", SIG_VALIDATION_SUCCESS);
+    }
+
+    function _postOp(PostOpMode, bytes calldata, uint256 actualGasCost, uint256)
+    internal override {
+    }
+}
diff --git a/contracts/test/TestPaymasterWithPostOp.sol b/contracts/test/TestPaymasterWithPostOp.sol
@@ -9,6 +9,7 @@ import "./TestPaymasterAcceptAll.sol";
  * explicitly returns a context, to test cost (for entrypoint) to call postOp
  */
 contract TestPaymasterWithPostOp is TestPaymasterAcceptAll {
+    event PostOpActualGasCost(uint256 actualGasCost);
 
     constructor(IEntryPoint _entryPoint) TestPaymasterAcceptAll(_entryPoint) {
     }
@@ -20,7 +21,8 @@ contract TestPaymasterWithPostOp is TestPaymasterAcceptAll {
         return ("1", SIG_VALIDATION_SUCCESS);
     }
 
-    function _postOp(PostOpMode, bytes calldata, uint256, uint256)
+    function _postOp(PostOpMode, bytes calldata, uint256 actualGasCost, uint256)
     internal override {
+        emit PostOpActualGasCost(actualGasCost);
     }
 }
diff --git a/gascalc/4-paymaster-postop.gas.ts b/gascalc/4-paymaster-postop.gas.ts
@@ -1,5 +1,5 @@
 import { parseEther } from 'ethers/lib/utils'
-import { TestPaymasterWithPostOp__factory } from '../typechain'
+import { GasCalcPaymasterWithPostOp__factory } from '../typechain'
 import { ethers } from 'hardhat'
 import { GasChecker } from './GasChecker'
 import { Create2Factory } from '../src/Create2Factory'
@@ -14,9 +14,9 @@ context('Paymaster with PostOp', function () {
   let paymasterAddress: string
 
   before(async () => {
-    const paymasterInit = hexValue(new TestPaymasterWithPostOp__factory(ethersSigner).getDeployTransaction(g.entryPoint().address).data!)
+    const paymasterInit = hexValue(new GasCalcPaymasterWithPostOp__factory(ethersSigner).getDeployTransaction(g.entryPoint().address).data!)
     paymasterAddress = await new Create2Factory(ethers.provider, ethersSigner).deploy(paymasterInit, 0)
-    const paymaster = TestPaymasterWithPostOp__factory.connect(paymasterAddress, ethersSigner)
+    const paymaster = GasCalcPaymasterWithPostOp__factory.connect(paymasterAddress, ethersSigner)
     await paymaster.addStake(1, { value: 1 })
     await g.entryPoint().depositTo(paymaster.address, { value: parseEther('10') })
   })
diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt
@@ -12,36 +12,36 @@
 ║                                │       │               │     (delta for │        (compared to ║
 ║                                │       │               │    one UserOp) │     account.exec()) ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │     1 │         77450 │                │                     ║
+║ simple                         │     1 │         77434 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │     2 │               │          41574 │               12315 ║
+║ simple - diff from previous    │     2 │               │          41582 │               12323 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │    10 │        451932 │                │                     ║
+║ simple                         │    10 │        451844 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │    11 │               │          41659 │               12400 ║
+║ simple - diff from previous    │    11 │               │          41667 │               12408 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │     1 │         83281 │                │                     ║
+║ simple paymaster               │     1 │         83249 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │     2 │               │          40106 │               10847 ║
+║ simple paymaster with diff     │     2 │               │          40134 │               10875 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │    10 │        444654 │                │                     ║
+║ simple paymaster               │    10 │        444514 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │    11 │               │          40199 │               10940 ║
+║ simple paymaster with diff     │    11 │               │          40227 │               10968 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │     1 │        167209 │                │                     ║
+║ big tx 5k                      │     1 │        167193 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │     2 │               │         130811 │               16109 ║
+║ big tx - diff from previous    │     2 │               │         130795 │               16093 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │    10 │       1344642 │                │                     ║
+║ big tx 5k                      │    10 │       1344470 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │    11 │               │         130788 │               16086 ║
+║ big tx - diff from previous    │    11 │               │         130820 │               16118 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ paymaster+postOp               │     1 │         84458 │                │                     ║
+║ paymaster+postOp               │     1 │         84616 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ paymaster+postOp with diff     │     2 │               │          41318 │               12059 ║
+║ paymaster+postOp with diff     │     2 │               │          41452 │               12193 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ paymaster+postOp               │    10 │        456453 │                │                     ║
+║ paymaster+postOp               │    10 │        457985 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ paymaster+postOp with diff     │    11 │               │          41369 │               12110 ║
+║ paymaster+postOp with diff     │    11 │               │          41551 │               12292 ║
 ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝
 
diff --git a/test/entrypoint.test.ts b/test/entrypoint.test.ts
diff --git a/test/testutils.ts b/test/testutils.ts

Original file line number	Diff line number	Diff line change
`@@ -192,7 +192,7 @@ contract EntryPointSimulations is EntryPoint, IEntryPointSimulations {`
`192`	`192`
`193`	`193`	`//slightly stricter gas limit than the real EntryPoint`
`194`	`194`	`function _getVerificationGasLimit(uint256 verificationGasLimit) internal pure virtual override returns (uint256) {`
`195`		`- return verificationGasLimit - 300;`
	`195`	`+ return verificationGasLimit - 500;`
`196`	`196`	`}`
`197`	`197`
`198`	`198`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,7 @@ import "./TestPaymasterAcceptAll.sol";`
`9`	`9`	`* explicitly returns a context, to test cost (for entrypoint) to call postOp`
`10`	`10`	`*/`
`11`	`11`	`contract TestPaymasterWithPostOp is TestPaymasterAcceptAll {`
	`12`	`+ event PostOpActualGasCost(uint256 actualGasCost);`
`12`	`13`
`13`	`14`	`constructor(IEntryPoint _entryPoint) TestPaymasterAcceptAll(_entryPoint) {`
`14`	`15`	`}`
`@@ -20,7 +21,8 @@ contract TestPaymasterWithPostOp is TestPaymasterAcceptAll {`
`20`	`21`	`return ("1", SIG_VALIDATION_SUCCESS);`
`21`	`22`	`}`
`22`	`23`
`23`		`- function _postOp(PostOpMode, bytes calldata, uint256, uint256)`
	`24`	`+ function _postOp(PostOpMode, bytes calldata, uint256 actualGasCost, uint256)`
`24`	`25`	`internal override {`
	`26`	`+ emit PostOpActualGasCost(actualGasCost);`
`25`	`27`	`}`
`26`	`28`	`}`