diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol index 1ef39236..b5473a5e 100644 --- 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 index a1436197..69ce75c8 100644 --- 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 index 0fc7d798..17dea2e6 100644 --- 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 index 505e0368..253f087e 100644 --- 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,