From ca1b6495f2bc9d8c22c499a42543ff2242e76ecd Mon Sep 17 00:00:00 2001 From: Alex Forshtat Date: Thu, 9 Feb 2023 03:42:50 +0400 Subject: [PATCH] AA-139 N-08: Inconsistent ordering [core and samples] (#211) * AA-139 N-08: Inconsistent ordering [core and samples] --------- Co-authored-by: shahafn --- contracts/interfaces/IEntryPoint.sol | 64 ++++++++++--------- contracts/interfaces/IPaymaster.sol | 12 ++-- contracts/samples/SimpleAccount.sol | 17 ++--- contracts/samples/bls/BLSAccount.sol | 2 - .../samples/bls/BLSSignatureAggregator.sol | 5 +- contracts/samples/bls/IBLSAccount.sol | 1 + reports/gas-checker.txt | 24 +++---- 7 files changed, 64 insertions(+), 61 deletions(-) diff --git a/contracts/interfaces/IEntryPoint.sol b/contracts/interfaces/IEntryPoint.sol index bed7d3fe..06de1616 100644 --- a/contracts/interfaces/IEntryPoint.sol +++ b/contracts/interfaces/IEntryPoint.sol @@ -67,6 +67,39 @@ interface IEntryPoint is IStakeManager { */ error SignatureValidationFailed(address aggregator); + /** + * Successful result from simulateValidation. + * @param returnInfo gas and time-range returned values + * @param senderInfo stake information about the sender + * @param factoryInfo stake information about the factor (if any) + * @param paymasterInfo stake information about the paymaster (if any) + */ + error ValidationResult(ReturnInfo returnInfo, + StakeInfo senderInfo, StakeInfo factoryInfo, StakeInfo paymasterInfo); + + /** + * Successful result from simulateValidation, if the account returns a signature aggregator + * @param returnInfo gas and time-range returned values + * @param senderInfo stake information about the sender + * @param factoryInfo stake information about the factor (if any) + * @param paymasterInfo stake information about the paymaster (if any) + * @param aggregatorInfo signature aggregation info (if the account requires signature aggregator) + * bundler MUST use it to verify the signature, or reject the UserOperation + */ + error ValidationResultWithAggregation(ReturnInfo returnInfo, + StakeInfo senderInfo, StakeInfo factoryInfo, StakeInfo paymasterInfo, + AggregatorStakeInfo aggregatorInfo); + + /** + * return value of getSenderAddress + */ + error SenderAddressResult(address sender); + + /** + * return value of simulateHandleOp + */ + error ExecutionResult(uint256 preOpGas, uint256 paid, uint64 validAfter, uint64 validBefore, bool targetSuccess, bytes targetResult); + //UserOps handled, per aggregator struct UserOpsPerAggregator { UserOperation[] userOps; @@ -111,30 +144,6 @@ interface IEntryPoint is IStakeManager { */ function simulateValidation(UserOperation calldata userOp) external; - /** - * Successful result from simulateValidation. - * @param returnInfo gas and time-range returned values - * @param senderInfo stake information about the sender - * @param factoryInfo stake information about the factor (if any) - * @param paymasterInfo stake information about the paymaster (if any) - */ - error ValidationResult(ReturnInfo returnInfo, - StakeInfo senderInfo, StakeInfo factoryInfo, StakeInfo paymasterInfo); - - - /** - * Successful result from simulateValidation, if the account returns a signature aggregator - * @param returnInfo gas and time-range returned values - * @param senderInfo stake information about the sender - * @param factoryInfo stake information about the factor (if any) - * @param paymasterInfo stake information about the paymaster (if any) - * @param aggregatorInfo signature aggregation info (if the account requires signature aggregator) - * bundler MUST use it to verify the signature, or reject the UserOperation - */ - error ValidationResultWithAggregation(ReturnInfo returnInfo, - StakeInfo senderInfo, StakeInfo factoryInfo, StakeInfo paymasterInfo, - AggregatorStakeInfo aggregatorInfo); - /** * gas and return values during simulation * @param preOpGas the gas used for validation (including preValidationGas) @@ -170,11 +179,6 @@ interface IEntryPoint is IStakeManager { */ function getSenderAddress(bytes memory initCode) external; - /** - * return value of getSenderAddress - */ - error SenderAddressResult(address sender); - /** * simulate full execution of a UserOperation (including both validation and target execution) @@ -190,7 +194,5 @@ interface IEntryPoint is IStakeManager { * @param targetCallData callData to pass to target address */ function simulateHandleOp(UserOperation calldata op, address target, bytes calldata targetCallData) external; - - error ExecutionResult(uint256 preOpGas, uint256 paid, uint64 validAfter, uint64 validBefore, bool targetSuccess, bytes targetResult); } diff --git a/contracts/interfaces/IPaymaster.sol b/contracts/interfaces/IPaymaster.sol index fa5eb5ba..590ef151 100644 --- a/contracts/interfaces/IPaymaster.sol +++ b/contracts/interfaces/IPaymaster.sol @@ -9,6 +9,12 @@ import "./UserOperation.sol"; */ interface IPaymaster { + enum PostOpMode { + opSucceeded, // user op succeeded + opReverted, // user op reverted. still has to pay for gas. + postOpReverted //user op succeeded, but caused postOp to revert. Now its a 2nd call, after user's op was deliberately reverted. + } + /** * payment validation: check if paymaster agree to pay. * Must verify sender is the entryPoint. @@ -41,10 +47,4 @@ interface IPaymaster { * @param actualGasCost - actual gas used so far (without this postOp call). */ function postOp(PostOpMode mode, bytes calldata context, uint256 actualGasCost) external; - - enum PostOpMode { - opSucceeded, // user op succeeded - opReverted, // user op reverted. still has to pay for gas. - postOpReverted //user op succeeded, but caused postOp to revert. Now its a 2nd call, after user's op was deliberately reverted. - } } diff --git a/contracts/samples/SimpleAccount.sol b/contracts/samples/SimpleAccount.sol index b17ac990..7ea3bae0 100644 --- a/contracts/samples/SimpleAccount.sol +++ b/contracts/samples/SimpleAccount.sol @@ -24,6 +24,15 @@ contract SimpleAccount is BaseAccount, UUPSUpgradeable, Initializable { uint96 private _nonce; address public owner; + IEntryPoint private immutable _entryPoint; + + event SimpleAccountInitialized(IEntryPoint indexed entryPoint, address indexed owner); + + modifier onlyOwner() { + _onlyOwner(); + _; + } + /// @inheritdoc BaseAccount function nonce() public view virtual override returns (uint256) { return _nonce; @@ -34,9 +43,6 @@ contract SimpleAccount is BaseAccount, UUPSUpgradeable, Initializable { return _entryPoint; } - IEntryPoint private immutable _entryPoint; - - event SimpleAccountInitialized(IEntryPoint indexed entryPoint, address indexed owner); // solhint-disable-next-line no-empty-blocks receive() external payable {} @@ -46,11 +52,6 @@ contract SimpleAccount is BaseAccount, UUPSUpgradeable, Initializable { _disableInitializers(); } - modifier onlyOwner() { - _onlyOwner(); - _; - } - function _onlyOwner() internal view { //directly from EOA owner, or through the account itself (which gets redirected through execute()) require(msg.sender == owner || msg.sender == address(this), "only owner"); diff --git a/contracts/samples/bls/BLSAccount.sol b/contracts/samples/bls/BLSAccount.sol index 33183699..79db2318 100644 --- a/contracts/samples/bls/BLSAccount.sol +++ b/contracts/samples/bls/BLSAccount.sol @@ -45,8 +45,6 @@ contract BLSAccount is SimpleAccount, IBLSAccount { return 0; } - event PublicKeyChanged(uint256[4] oldPublicKey, uint256[4] newPublicKey); - /** * Allows the owner to set or change the BLS key. * @param newPublicKey public key from a BLS keypair that will have a full ownership and control of this account. diff --git a/contracts/samples/bls/BLSSignatureAggregator.sol b/contracts/samples/bls/BLSSignatureAggregator.sol index 6e3552d7..f033d72e 100644 --- a/contracts/samples/bls/BLSSignatureAggregator.sol +++ b/contracts/samples/bls/BLSSignatureAggregator.sol @@ -16,6 +16,9 @@ contract BLSSignatureAggregator is IAggregator { bytes32 public constant BLS_DOMAIN = keccak256("eip4337.bls.domain"); + //copied from BLS.sol + uint256 public constant N = 21888242871839275222246405745257275088696311157297823662689037894645226208583; + /** * @return publicKey - the public key from a BLS keypair the Aggregator will use to verify this UserOp; * normally public key will be queried from the deployed BLSAccount itself; @@ -132,8 +135,6 @@ contract BLSSignatureAggregator is IAggregator { return ""; } - //copied from BLS.sol - uint256 public constant N = 21888242871839275222246405745257275088696311157297823662689037894645226208583; /** * aggregate multiple signatures into a single value. diff --git a/contracts/samples/bls/IBLSAccount.sol b/contracts/samples/bls/IBLSAccount.sol index b3b3151f..df1a9c21 100644 --- a/contracts/samples/bls/IBLSAccount.sol +++ b/contracts/samples/bls/IBLSAccount.sol @@ -7,6 +7,7 @@ import "../../interfaces/IAggregatedAccount.sol"; * a BLS account should expose its own public key. */ interface IBLSAccount is IAggregatedAccount { + event PublicKeyChanged(uint256[4] oldPublicKey, uint256[4] newPublicKey); /** * @return public key from a BLS keypair that is used to verify the BLS signature, both separately and aggregated. diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt index 91d0a3bb..e9a2b4fc 100644 --- a/reports/gas-checker.txt +++ b/reports/gas-checker.txt @@ -1,35 +1,35 @@ == gas estimate of direct calling the account's "execFromEntryPoint" method the destination is "account.nonce()", which is known to be "hot" address used by this account it little higher than EOA call: its an exec from entrypoint (or account owner) into account contract, verifying msg.sender and exec to target) -- gas estimate "simple" - 31045 +- gas estimate "simple" - 31033 - gas estimate "big tx 5k" - 127295 ╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗ ║ handleOps description │ count │ total gasUsed │ per UserOp gas │ per UserOp overhead ║ ║ │ │ │ (delta for │ (compared to ║ ║ │ │ │ one UserOp) │ account.exec()) ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 1 │ 78111 │ │ ║ +║ simple │ 1 │ 78087 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 2 │ │ 43585 │ 12540 ║ +║ simple - diff from previous │ 2 │ │ 43549 │ 12516 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 10 │ 470521 │ │ ║ +║ simple │ 10 │ 470485 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 11 │ │ 43708 │ 12663 ║ +║ simple - diff from previous │ 11 │ │ 43720 │ 12687 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ simple paymaster │ 1 │ 84411 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 2 │ │ 42586 │ 11541 ║ +║ simple paymaster with diff │ 2 │ │ 42598 │ 11565 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 10 │ 467990 │ │ ║ +║ simple paymaster │ 10 │ 467918 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 11 │ │ 42746 │ 11701 ║ +║ simple paymaster with diff │ 11 │ │ 42770 │ 11737 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 1 │ 179834 │ │ ║ +║ big tx 5k │ 1 │ 179846 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 2 │ │ 144934 │ 17639 ║ +║ big tx - diff from previous │ 2 │ │ 144910 │ 17615 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 10 │ 1489629 │ │ ║ +║ big tx 5k │ 10 │ 1489653 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 11 │ │ 146451 │ 19156 ║ +║ big tx - diff from previous │ 11 │ │ 146391 │ 19096 ║ ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝