-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update EIP-7702: refine based on discussions #8561
Conversation
✅ All reviewers have approved. |
Co-authored-by: vbuterin <v@buterin.com>
Co-authored-by: Ahmad Bitar <33181301+smartprogrammer93@users.noreply.github.com>
I just want to clarify that, given the discussions on eth magicians, it's clear that many (most) of the updates in this PR are still in contention (even among the EIP authors), so I personally don't think that this is a good time to merge this PR, considering that we will have another ACDE meeting coming up to discuss 7702 and hopefully align on some of these potential updates. Merging this PR now might mislead the core devs and other community members into believing that decisions have already been made one way or the other, when in reality the discussion is still very much ongoing. Edit: there's now broad agreement on most of the points in this PR, after the last few meetings/breakouts. |
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
@@ -88,11 +155,33 @@ Specifically: | |||
|
|||
## Backwards Compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing from the list of broken assumptions is signature malleability:
Specifically, a user might sign an unverified experimental 7702 contract on a testnet under the assumption (which was correct until now) that it is a sandbox, and nothing affects other network.
This becomes disastrous, as this contract can be used anywhere - and currently is un-revocable.
The user never opt-in to this new feature: he merely continued his work with an existing (though updated) wallet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drortirosh chainId variable will be added to the signature. Also, this type of signed data has a specific magic. Wallets are expected to warn the user heavily about what they are about to sign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing an unverified / experimental 7702 contract would never happen with a wallet that correctly implements 7702.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any problem with "wallet that correctly implements the standard".
Leaving the chainid out of signature EIP breaks the invariant that "testnet are sandbox and can't affect any other network".
Please add that text into your security consideration, if you think that's acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing an unverified / experimental 7702 contract would never happen with a wallet that correctly implements 7702.
The only way to implement this is by completely blocking (by the wallet) from ever signing anything un-approved by the wallet, not even with a warning message.
That's the only way you can block this concern.
Even with a warning message such as:
Beware of this signature: it opens your account to malicious operations not only on this network, but on any other network, and this operation is un-revocable. are you sure you want to continue
And I'm afraid that on testnets, some people will STILL sign this message, as the current narrative is "testnets are sandbox"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which networks are banning chain id 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which networks are banning chain id 0?
The question is, on which network you can submit a non-EIP-155 transaction? All block builders on all networks I know of block them.
That's a de-facto standard (that all TXs are EIP-155).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're conflating protocol functionality with protections offered at the RPC level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying that there is an implicit assumptions today by users, that this EIP breaks:
- That signing data on testnet stays in testnet and doesn't affect other networks.
- That signing anything has a one-shot effect, not long-lasting (or permanent)
And I think that breaking these assumptions AT LEAST deserves being mentioned in the "security consideration" (or backward compatibility) section.
After specifying them, you can add your argument why you think it is OK to break them - but at least acknowledge that we're introducing a network change that alters existing users assumptions.
This is not a change that affect new users that use those new features, or new wallets: it affect all existing users that happen to upgrade their existing wallet, and are unaware of such changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That signing data on testnet stays in testnet and doesn't affect other networks.
Again, this is not correct. The protocol allows for non EIP-155 txs to be replayed on any chain. If users think they're safe because the wallet says they're on a testnet today, it's because apps, wallets, and RPC providers have done well to create this feeling of security. They could have just as easily continued signing and allowing txs w/o chain id baked in. I have no reason to believe post-7702 things will be any different.
That signing anything has a one-shot effect, not long-lasting (or permanent)
This would be true for 7702 auths. Happy to put reword the security considerations if it is not clear there.
The commit 23f2ec0 (as a parent of 133edf9) contains errors. |
Some thoughts I'd like to bring up as I review this revision:
So this means that if we have
Isn't this a good thing? I believe the context this is in makes it sound like a bad thing. I bias towards maximal flexibility. This "creation by template" doesn't necessarily mean that what the EOA can sign/execute is constrained right? It is just choosing the way that code should be signed over?
Potentially dumb question but what happens if an SSTORE opcode is called within a 7702 transaction? Would it just not do anything or act kind of like memory storage because EOA's don't have built-in storage? Also, what are the unnecessary inefficiencies with an external storage contract? - would love to identify them to keep note of them.
I wrote some thoughts here around time-based revocation. Not sure if it is at all useful here but thought I'd throw it into the ring.
Would this list of params be signed over in the |
If I produce and broadcast one 7702 signature without nonce, I can never again use 7702 without the risk of being frontrun with my previous signature to prevent the new code from being deployed on my account. This does seem like something the protocol should give tools to prevent. |
@frangio I am not sure how would be front run. If you send a 7702 tx that you control what authorizations go inside it. Even if there was a 7702 tx before, it wont affect how your tx is executed. |
Right, I was thinking of a bundle submitted by a sponsor, and "frontrunning" in the sense of including an authorization before another in a 7702 tx authorization list. I guess that clarifies why it shouldn't be considered a problem? A sponsor can always just choose not to include your signed authorization, so including two of your authorizations as a way to censor one of them is not really a new capability. |
@lightclient can we specify exactly the behavior in case the validations of an authorization fails? my assumption is that it gets ignored but the tx goes through regardless. if that is not the case and tx should fail, do we spend all gas? seems excessive. |
Let's clarify how null nonce (as opposed to zero) is encoded in |
@yperbasis you're correct, as specified it was not possible to represent null nonce value. I've updated it to wrap the nonce in a list object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
Based on some discussions I've made the following updates:
2500
, mostly to account for reading the template accountAdditionally, I have added a lot of rationale for why decisions were made.