mirror of
https://github.com/getwax/zk-account-abstraction.git
synced 2026-01-08 04:03:59 -05:00
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.
This commit is contained in:
Dror Tirosh
@@ -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];
|
||||
|
||||
@@ -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.
|
||||
*/
|
||||
|
||||
@@ -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 ║
|
||||
╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user