AA-139 N-08: Inconsistent ordering [core and samples] (#211)

* AA-139 N-08: Inconsistent ordering [core and samples] --------- Co-authored-by: shahafn <shahaflol@gmail.com>
eth-infinitism · Feb 8, 2023 · ca1b649 · ca1b649
1 parent 1633c06
commit ca1b649
Show file tree

Hide file tree

Showing 7 changed files with 64 additions and 61 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 ║
 ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝