AA-161 validate Nonce in EntryPoint #247

drortirosh · 2023-03-20T20:07:22Z

account doesn't check nonce. instead, entryPoint validate and increment nonce.
nonces are two-dimensional, with 192 bit of "key" and 64 bit of sequence.

For description about this change, please see this doc

merge v0.5 to main

- NonceManager to handle 2d nonces - _validateAndUpdateNonce() called after validateUserOp - getNonce(sender, key) - remove nonce checking from BaseAccount - BaseAccount implements nonce() using ep.getNonce

contracts/core/NonceManager.sol

leekt · 2023-03-21T16:47:48Z

contracts/core/EntryPoint.sol

@@ -511,6 +514,11 @@ contract EntryPoint is IEntryPoint, StakeManager {
        uint256 gasUsedByValidateAccountPrepayment;
        (uint256 requiredPreFund) = _getRequiredPrefund(mUserOp);
        (gasUsedByValidateAccountPrepayment, validationData) = _validateAccountPrepayment(opIndex, userOp, outOpInfo, requiredPreFund);
+
+        if (!_validateAndUpdateNonce(mUserOp.sender, mUserOp.nonce)) {


I noticed some pattern on old sample accounts that it skip nonce increment when initCode.length > 0.
I assume it's for gas savings, but I don't see it here. Is it intended?

It was an internal optimization of SimpleWallet.

leekt · 2023-03-21T16:59:47Z

contracts/samples/SimpleAccount.sol

+        //increment nonce during construction.
+        // This way, the cost of moving from "0" to "1" is absorbed
+        // by first (construction) transaction, instead of during the 2nd transaction
+        entryPoint().incrementNonce(0);


I know that this part is just a sample but just out of curiosity, since this will be called on construction and userOp.nonce will start with 1 as far as i know since validateUserOp will check for nonce after initialize. Is this a best practice?

And on my understanding, this nonce will be increased to 1 after validateUserOp even without this. Why bother going 0 -> 1 -> 2 for first userOp? instead of going 0 -> 1?

It was done so that even wallets that are not created via a dedicated UserOp, will have their next operation gas "clean".
A wallet can be created by execution its initCode either by the UserOp, or by other means.
Calling "incrementNonce" the first time will cost 20000 gas, and any further call 5000 (I ignore for now explicitly setting to a new key, which is will cost 20000 again, but that an explicit operation a wallet owner does)
Not incrementing it during construction will mean first TX after construction will cost 20000 gas more than any further TX. We prefer to make gas-per-transaction as consistent as possible, and thus make only the construction transaction different.

Calling "incrementNonce" from the factory, indeed causes double-increment of the nonce, but the overhead is minimal (~400 gas)
The side-effect is indeed an extra increment of the nonce.

contracts/core/EntryPoint.sol

create account doesn't increment nonce automatically. The nonce is incremented if it is created through a UserOp Side-effect: if account is created NOT through AA, then first UserOp will cost 20000 gas more. (if created as a userop, then this extra cost is part of the "creation" and not on a separate transaction.

yoavw · 2023-03-23T13:14:37Z

contracts/interfaces/INonceManager.sol

+    function getNonce(address sender, uint192 key)
+    external view returns (uint256 nonce);
+
+    function incrementNonce(uint192 key) external;


Maybe add documentation, explaining that this function doesn't need to be called by the account?

actually, it is not needed at all. I just wonder if we should keep it for completeness, or remove it altogether..

I thought it was needed for the "bypass" case where an account wants to support non-EntryPoint interactions and still use the same nonce. Is that no longer the case?

I think one side-effect of this function is breaking continuity of nonce, that lose the 'one nonce corresponding to one OP' property.

For example, wallet can call _validateSignature in OP calldata to increase nonce by 2 in one single OP.

Also, wallet can call incrementNonce in _validateSignature, just before checking in _validateAndUpdateNonce, which makes it feasible to execute OP with nonce 3 when the next nonce is 2. So I suggest to check nonce _validateAndUpdateNonce before _validateSignature.

The match of "one nonce to one OP" exists, but in any case, it can't be trusted as a counter: e.g. "current nonce is # of past transactions" is not true, because:

you can incrementNonce(), but also:

you can as many different key values, each with another sent of transactions.

accounts can support "batch", which means a single UserOp can map to multiple transactions.

In short, at the system level, nonces are nothing but replay protection..

contracts/samples/gnosis/EIP4337Manager.sol

shahafn · 2023-03-26T12:44:00Z

contracts/core/EntryPoint.sol

@@ -345,11 +346,12 @@ contract EntryPoint is IEntryPoint, StakeManager {
    /**
     * Get counterfactual sender address.
     *  Calculate the sender contract address that will be generated by the initCode and salt in the UserOperation.
-     * this method always revert, and returns the address in SenderAddressResult error
+     * this method always revert, and returns the address (and the nonce) in SenderAddressResult error


Not returning nonce anymore

shahafn · 2023-03-26T12:53:07Z

contracts/interfaces/IEntryPoint.sol

@@ -174,7 +175,7 @@ interface IEntryPoint is IStakeManager {
    /**
     * Get counterfactual sender address.
     *  Calculate the sender contract address that will be generated by the initCode and salt in the UserOperation.
-     * this method always revert, and returns the address in SenderAddressResult error
+     * this method always revert, and returns the address (and nonce) in SenderAddressResult error


Doesn't return nonce

shahafn · 2023-03-26T12:56:23Z

eip/EIPS/eip-4337.md

@@ -907,7 +920,7 @@ See `https://github.com/eth-infinitism/account-abstraction/tree/main/contracts`

 ## Security Considerations

-The entry point contract will need to be very heavily audited and formally verified, because it will serve as a central trust point for _all_ [ERC-4337](./eip-4337.md). In total, this architecture reduces auditing and formal verification load for the ecosystem, because the amount of work that individual _accounts_ have to do becomes much smaller (they need only verify the `validateUserOp` function and its "check signature, increment nonce and pay fees" logic) and check that other functions are `msg.sender == ENTRY_POINT` gated (perhaps also allowing `msg.sender == self`), but it is nevertheless the case that this is done precisely by concentrating security risk in the entry point contract that needs to be verified to be very robust.
+The entry point contract will need to be very heavily audited and formally verified, because it will serve as a central trust point for _all_ [EIP-4337](./eip-4337.md). In total, this architecture reduces auditing and formal verification load for the ecosystem, because the amount of work that individual _accounts_ have to do becomes much smaller (they need only verify the `validateUserOp` function and its "check signature, and pay fees" logic) and check that other functions are `msg.sender == ENTRY_POINT` gated (perhaps also allowing `msg.sender == self`), but it is nevertheless the case that this is done precisely by concentrating security risk in the entry point contract that needs to be verified to be very robust.


I thought we changed EIP to ERC

nonce() is no longer "do-nothing" method. use instead account()

Gooong · 2023-03-28T09:41:16Z

contracts/interfaces/INonceManager.sol

+    function getNonce(address sender, uint192 key)
+    external view returns (uint256 nonce);
+
+    function incrementNonce(uint192 key) external;


I think one side-effect of this function is breaking continuity of nonce, that lose the 'one nonce corresponding to one OP' property.

For example, wallet can call _validateSignature in OP calldata to increase nonce by 2 in one single OP.

Also, wallet can call incrementNonce in _validateSignature, just before checking in _validateAndUpdateNonce, which makes it feasible to execute OP with nonce 3 when the next nonce is 2. So I suggest to check nonce _validateAndUpdateNonce before _validateSignature.

Gooong · 2023-03-28T09:47:27Z

contracts/core/NonceManager.sol

+    // (mainly so that during construction nonce can be made non-zero,
+    // to "absorb" the gas cost of first nonce increment to 1st transaction (construction),
+    // not to 2nd transaction)
+    function incrementNonce(uint192 key) public override {


Wallet can call incrementNonce in _validateSignature, just before checking in _validateAndUpdateNonce, which makes it feasible to execute OP with nonce 3 when the next nonce is 2. So I suggest to check nonce _validateAndUpdateNonce before _validateSignature.

I agree that in make sense to modify the nonce before calling a user-provided method.
Still, our opcode rules prevent the account from calling the entryPoint (including incrementNonce) from within validation.

* EntryPoint to manage Nonce - NonceManager to handle 2d nonces - _validateAndUpdateNonce() called after validateUserOp - getNonce(sender, key) - remove nonce checking from BaseAccount - BaseAccount implements getNonce() using ep.getNonce

drortirosh and others added 8 commits March 2, 2023 02:35

Merge pull request #230 from eth-infinitism/develop

75f0245

merge v0.5 to main

EntryPoint to manage Nonce

931df7e

- NonceManager to handle 2d nonces - _validateAndUpdateNonce() called after validateUserOp - getNonce(sender, key) - remove nonce checking from BaseAccount - BaseAccount implements nonce() using ep.getNonce

getSenderAddress to return nonce

c1290e7

incrementNonce for gnosis

b2068fa

support gnosis

33a8e54

remove key_offset

de441b0

lints

98ee474

gas calcs

60e869c

fangting-alchemy reviewed Mar 20, 2023

View reviewed changes

contracts/core/NonceManager.sol Show resolved Hide resolved

leekt reviewed Mar 21, 2023

View reviewed changes

shahafn reviewed Mar 22, 2023

View reviewed changes

contracts/core/EntryPoint.sol Show resolved Hide resolved

yoavw reviewed Mar 23, 2023

View reviewed changes

drortirosh added 4 commits March 23, 2023 16:59

PR comments.

85371d1

Merge branch 'develop' into ep-nonce

bb8dc1f

eip updates, added test.

33cbda9

fix tests

ac89c73

shahafn reviewed Mar 26, 2023

View reviewed changes

drortirosh added 2 commits March 26, 2023 15:50

Merge branch 'develop' into ep-nonce

beba6a7

PR comment

5fed963

shahafn reviewed Mar 26, 2023

View reviewed changes

update nonce comment

b646697

drortirosh force-pushed the ep-nonce branch from 2ab608c to b646697 Compare March 26, 2023 13:12

drortirosh added 4 commits March 26, 2023 16:35

remove unused "filler"

4618401

fix gas calc

69e19f8

nonce() is no longer "do-nothing" method. use instead account()

rename "nonce()" to "getNonce()"

8bc2f63

Merge branch 'develop' into ep-nonce

dab35aa

drortirosh force-pushed the ep-nonce branch from 90a373d to dab35aa Compare March 27, 2023 11:19

Gooong reviewed Mar 28, 2023

View reviewed changes

livingrockrises mentioned this pull request Mar 28, 2023

support for future EP getNonce() bcnmy/scw-contracts#80

Closed

drortirosh changed the title ~~validate Nonce in EntryPoint~~ AA-161 validate Nonce in EntryPoint Apr 2, 2023

drortirosh force-pushed the ep-nonce branch from 1a0fbef to 7b918ae Compare April 9, 2023 09:24

rename nonces hash

e0ee20b

drortirosh force-pushed the ep-nonce branch from 7b918ae to e0ee20b Compare April 9, 2023 09:39

Merge branch 'develop' into ep-nonce

98d6888

shahafn approved these changes Apr 9, 2023

View reviewed changes

drortirosh merged commit 19918cd into develop Apr 9, 2023

drortirosh deleted the ep-nonce branch May 4, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AA-161 validate Nonce in EntryPoint #247

AA-161 validate Nonce in EntryPoint #247

drortirosh commented Mar 20, 2023 •

edited

Loading

leekt Mar 21, 2023

drortirosh Mar 22, 2023

leekt Mar 21, 2023 •

edited

Loading

drortirosh Mar 22, 2023

yoavw Mar 23, 2023

drortirosh Mar 23, 2023

yoavw Mar 24, 2023

Gooong Mar 28, 2023

drortirosh Mar 28, 2023

shahafn Mar 26, 2023

shahafn Mar 26, 2023

shahafn Mar 26, 2023

Gooong Mar 28, 2023

Gooong Mar 28, 2023

drortirosh Mar 28, 2023

AA-161 validate Nonce in EntryPoint #247

AA-161 validate Nonce in EntryPoint #247

Conversation

drortirosh commented Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leekt Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drortirosh commented Mar 20, 2023 •

edited

Loading

leekt Mar 21, 2023 •

edited

Loading