AA-159 prevent recursive call into handleOps (#257)

* AA-159 prevent recursive call into handleOps * added BeforeExecution event without this event, all events in validation are attributed to the first UserOperation. We can't attribute them to specific UseROp (unless there is exactly one) - but at least not always assign the to the first.
eth-infinitism · Apr 9, 2023 · 9b5f2e4 · Gooong · Apr 10, 2023 · 9b5f2e4
1 parent 19918cd
commit 9b5f2e4
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 16 deletions.
diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol
@@ -17,8 +17,9 @@ import "./StakeManager.sol";
 import "./SenderCreator.sol";
 import "./Helpers.sol";
 import "./NonceManager.sol";
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
 
-contract EntryPoint is IEntryPoint, StakeManager, NonceManager {
+contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard {
 
     using UserOperationLib for UserOperation;
 
@@ -88,7 +89,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager {
      * @param ops the operations to execute
      * @param beneficiary the address to receive the fees
      */
-    function handleOps(UserOperation[] calldata ops, address payable beneficiary) public {
+    function handleOps(UserOperation[] calldata ops, address payable beneficiary) public nonReentrant {
 
         uint256 opslen = ops.length;
         UserOpInfo[] memory opInfos = new UserOpInfo[](opslen);
@@ -101,6 +102,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager {
         }
 
         uint256 collected = 0;
+        emit BeforeExecution();
 
         for (uint256 i = 0; i < opslen; i++) {
             collected += _executeUserOp(i, ops[i], opInfos[i]);
@@ -118,7 +120,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager {
     function handleAggregatedOps(
         UserOpsPerAggregator[] calldata opsPerAggregator,
         address payable beneficiary
-    ) public {
+    ) public nonReentrant {
 
         uint256 opasLen = opsPerAggregator.length;
         uint256 totalOps = 0;
@@ -143,6 +145,8 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager {
 
         UserOpInfo[] memory opInfos = new UserOpInfo[](totalOps);
 
+        emit BeforeExecution();
+
         uint256 opIndex = 0;
         for (uint256 a = 0; a < opasLen; a++) {
             UserOpsPerAggregator calldata opa = opsPerAggregator[a];

diff --git a/contracts/interfaces/IEntryPoint.sol b/contracts/interfaces/IEntryPoint.sol
@@ -46,6 +46,12 @@ interface IEntryPoint is IStakeManager, INonceManager {
      */
     event UserOperationRevertReason(bytes32 indexed userOpHash, address indexed sender, uint256 nonce, bytes revertReason);
 
+    /**
+     * an event emitted by handleOps(), before starting the execution loop.
+     * any event emitted before this event, is part of the validation.
+     */
+    event BeforeExecution();
+
     /**
      * signature aggregator used by the following UserOperationEvents within this bundle.
      */

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 │         78743 │                │                     ║
+║ simple                         │     1 │         81901 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │     2 │               │          44162 │               15148 ║
+║ simple - diff from previous    │     2 │               │          44212 │               15198 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │    10 │        476282 │                │                     ║
+║ simple                         │    10 │        479854 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │    11 │               │          44174 │               15160 ║
+║ simple - diff from previous    │    11 │               │          44236 │               15222 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │     1 │         85002 │                │                     ║
+║ simple paymaster               │     1 │         88172 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │     2 │               │          43139 │               14125 ║
+║ simple paymaster with diff     │     2 │               │          43165 │               14151 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │    10 │        473554 │                │                     ║
+║ simple paymaster               │    10 │        476994 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │    11 │               │          43150 │               14136 ║
+║ simple paymaster with diff     │    11 │               │          43260 │               14246 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │     1 │        179788 │                │                     ║
+║ big tx 5k                      │     1 │        182958 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │     2 │               │         144673 │               19413 ║
+║ big tx - diff from previous    │     2 │               │         144723 │               19463 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │    10 │       1481914 │                │                     ║
+║ big tx 5k                      │    10 │       1485438 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │    11 │               │         144698 │               19438 ║
+║ big tx - diff from previous    │    11 │               │         144712 │               19452 ║
 ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝
 
diff --git a/test/entrypoint.test.ts b/test/entrypoint.test.ts
@@ -42,7 +42,7 @@ import {
   simulationResultCatch,
   createAccount,
   getAggregatedAccountInitCode,
-  simulationResultWithAggregationCatch
+  simulationResultWithAggregationCatch, decodeRevertReason
 } from './testutils'
 import { DefaultsForUserOp, fillAndSign, getUserOpHash } from './UserOp'
 import { UserOperation } from './UserOperation'
@@ -783,6 +783,23 @@ describe('EntryPoint', function () {
         await calcGasUsage(rcpt, entryPoint, beneficiaryAddress)
       })
 
+      it('should fail to call recursively into handleOps', async () => {
+        const beneficiaryAddress = createAddress()
+
+        const callHandleOps = entryPoint.interface.encodeFunctionData('handleOps', [[], beneficiaryAddress])
+        const execHandlePost = account.interface.encodeFunctionData('execute', [entryPoint.address, 0, callHandleOps])
+        const op = await fillAndSign({
+          sender: account.address,
+          callData: execHandlePost
+        }, accountOwner, entryPoint)
+
+        const rcpt = await entryPoint.handleOps([op], beneficiaryAddress, {
+          gasLimit: 1e7
+        }).then(async r => r.wait())
+
+        const error = rcpt.events?.find(ev => ev.event === 'UserOperationRevertReason')
+        expect(decodeRevertReason(error?.args?.revertReason)).to.eql('Error(ReentrancyGuard: reentrant call)', 'execution of handleOps inside a UserOp should revert')
+      })
       it('should report failure on insufficient verificationGas after creation', async () => {
         const op0 = await fillAndSign({
           sender: account.address,