From 692374c1052d157cb50003e68b7326462eb0086c Mon Sep 17 00:00:00 2001 From: Alex Forshtat Date: Tue, 17 Dec 2024 13:34:16 +0100 Subject: [PATCH] AA-466: Prevent InitCode frontrunning; AA-470: Make SenderCreator public --- contracts/core/EntryPoint.sol | 2 +- contracts/core/EntryPointSimulations.sol | 4 +-- contracts/core/SenderCreator.sol | 9 +++++ reports/gas-checker.txt | 42 ++++++++++++------------ test/entrypoint.test.ts | 33 ++++++++++++------- 5 files changed, 54 insertions(+), 36 deletions(-) diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol index 44501524d..9045087da 100644 --- a/contracts/core/EntryPoint.sol +++ b/contracts/core/EntryPoint.sol @@ -30,7 +30,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, SenderCreator private immutable _senderCreator = new SenderCreator(); - function senderCreator() internal view virtual returns (SenderCreator) { + function senderCreator() public view virtual returns (SenderCreator) { return _senderCreator; } diff --git a/contracts/core/EntryPointSimulations.sol b/contracts/core/EntryPointSimulations.sol index b8c41ea12..43e96a42c 100644 --- a/contracts/core/EntryPointSimulations.sol +++ b/contracts/core/EntryPointSimulations.sol @@ -24,7 +24,7 @@ contract EntryPointSimulations is EntryPoint, IEntryPointSimulations { _senderCreator = SenderCreator(createdObj); } - function senderCreator() internal view virtual override returns (SenderCreator) { + function senderCreator() public view virtual override returns (SenderCreator) { // return the same senderCreator as real EntryPoint. // this call is slightly (100) more expensive than EntryPoint's access to immutable member return _senderCreator; @@ -35,7 +35,7 @@ contract EntryPointSimulations is EntryPoint, IEntryPointSimulations { * it as entrypoint, since the simulation functions don't check the signatures */ constructor() { - require(block.number < 100, "should not be deployed"); + require(block.number < 1000, "should not be deployed"); } /// @inheritdoc IEntryPointSimulations diff --git a/contracts/core/SenderCreator.sol b/contracts/core/SenderCreator.sol index 43ea80367..f2d253d3b 100644 --- a/contracts/core/SenderCreator.sol +++ b/contracts/core/SenderCreator.sol @@ -6,6 +6,12 @@ pragma solidity ^0.8.23; * which is explicitly not the entryPoint itself. */ contract SenderCreator { + address public immutable entryPoint; + + constructor(){ + entryPoint = msg.sender; + } + /** * Call the "initCode" factory to create and return the sender account address. * @param initCode - The initCode value from a UserOp. contains 20 bytes of factory address, @@ -15,6 +21,9 @@ contract SenderCreator { function createSender( bytes calldata initCode ) external returns (address sender) { + if (msg.sender != entryPoint) { + revert("AAxx should call from EntryPoint"); + } address factory = address(bytes20(initCode[0:20])); bytes memory initCallData = initCode[20:]; bool success; diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt index 692322bf7..400e5948c 100644 --- a/reports/gas-checker.txt +++ b/reports/gas-checker.txt @@ -4,7 +4,7 @@ ╔══════════════════════════╤════════╗ ║ gas estimate "simple" │ 28979 ║ ╟──────────────────────────┼────────╢ -║ gas estimate "big tx 5k" │ 125224 ║ +║ gas estimate "big tx 5k" │ 125212 ║ ╚══════════════════════════╧════════╝ ╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗ @@ -12,44 +12,44 @@ ║ │ │ │ (delta for │ (compared to ║ ║ │ │ │ one UserOp) │ account.exec()) ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 1 │ 79994 │ │ ║ +║ simple │ 1 │ 80004 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 2 │ │ 42192 │ 13213 ║ +║ simple - diff from previous │ 2 │ │ 42204 │ 13225 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 10 │ 459921 │ │ ║ +║ simple │ 10 │ 459895 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 11 │ │ 42223 │ 13244 ║ +║ simple - diff from previous │ 11 │ │ 42307 │ 13328 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 1 │ 86113 │ │ ║ +║ simple paymaster │ 1 │ 86147 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 2 │ │ 41024 │ 12045 ║ +║ simple paymaster with diff │ 2 │ │ 41048 │ 12069 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 10 │ 455444 │ │ ║ +║ simple paymaster │ 10 │ 455730 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 11 │ │ 41088 │ 12109 ║ +║ simple paymaster with diff │ 11 │ │ 41076 │ 12097 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 1 │ 181026 │ │ ║ +║ big tx 5k │ 1 │ 181036 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 2 │ │ 142714 │ 17490 ║ +║ big tx - diff from previous │ 2 │ │ 142702 │ 17490 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 10 │ 1465443 │ │ ║ +║ big tx 5k │ 10 │ 1465453 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 11 │ │ 142686 │ 17462 ║ +║ big tx - diff from previous │ 11 │ │ 142710 │ 17498 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp │ 1 │ 87712 │ │ ║ +║ paymaster+postOp │ 1 │ 87770 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp with diff │ 2 │ │ 42671 │ 13692 ║ +║ paymaster+postOp with diff │ 2 │ │ 42659 │ 13680 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp │ 10 │ 471754 │ │ ║ +║ paymaster+postOp │ 10 │ 471848 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp with diff │ 11 │ │ 42728 │ 13749 ║ +║ paymaster+postOp with diff │ 11 │ │ 42656 │ 13677 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 1 │ 128777 │ │ ║ +║ token paymaster │ 1 │ 128821 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 2 │ │ 66386 │ 37407 ║ +║ token paymaster with diff │ 2 │ │ 66408 │ 37429 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 10 │ 726504 │ │ ║ +║ token paymaster │ 10 │ 726746 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 11 │ │ 66394 │ 37415 ║ +║ token paymaster with diff │ 11 │ │ 66488 │ 37509 ║ ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝ diff --git a/test/entrypoint.test.ts b/test/entrypoint.test.ts index 4a934c447..aa69f6058 100644 --- a/test/entrypoint.test.ts +++ b/test/entrypoint.test.ts @@ -2,10 +2,18 @@ import './aa.init' import { BigNumber, Event, Wallet } from 'ethers' import { expect } from 'chai' import { + EntryPoint, + IEntryPoint__factory, + INonceManager__factory, + IStakeManager__factory, + MaliciousAccount__factory, + SenderCreator__factory, SimpleAccount, SimpleAccountFactory, - TestAggregatedAccount__factory, + SimpleAccountFactory__factory, + TestAggregatedAccount, TestAggregatedAccountFactory__factory, + TestAggregatedAccount__factory, TestCounter, TestCounter__factory, TestExpirePaymaster, @@ -14,20 +22,13 @@ import { TestExpiryAccount__factory, TestPaymasterAcceptAll, TestPaymasterAcceptAll__factory, + TestPaymasterRevertCustomError__factory, + TestPaymasterWithPostOp, + TestPaymasterWithPostOp__factory, TestRevertAccount__factory, - TestAggregatedAccount, TestSignatureAggregator, TestSignatureAggregator__factory, - MaliciousAccount__factory, - TestWarmColdAccount__factory, - TestPaymasterRevertCustomError__factory, - IEntryPoint__factory, - SimpleAccountFactory__factory, - IStakeManager__factory, - INonceManager__factory, - EntryPoint, - TestPaymasterWithPostOp__factory, - TestPaymasterWithPostOp + TestWarmColdAccount__factory } from '../typechain' import { AddressZero, @@ -834,6 +835,14 @@ describe('EntryPoint', function () { let createOp: PackedUserOperation const beneficiaryAddress = createAddress() // 1 + it('should reject create if SenderCreator not called from EntryPoint', async () => { + const senderCreatorAddress = await entryPoint.senderCreator() + const senderCreator = SenderCreator__factory.connect(senderCreatorAddress, ethersSigner) + await expect( + senderCreator.createSender('0xdeadbeef', { gasLimit: 1000000 }) + ).to.be.revertedWith('AAxx should call from EntryPoint') + }) + it('should reject create if sender address is wrong', async () => { const op = await fillSignAndPack({ initCode: getAccountInitCode(accountOwner.address, simpleAccountFactory),