Skip to content

Update EIP-7702: Add checks for signatures in authorization lists #9632

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nethoxa
Copy link

@nethoxa nethoxa commented Apr 12, 2025

This PR adds some checks that are being done by all EL clients to the signature values of authorization lists, which are not stated in the EIP. As such, developers following the spec may have issues, as they would be working with signatures that are invalid at the EL layer. As an example, see Geth (this is called when checking the auth signatures here)

@nethoxa nethoxa requested a review from eth-bot as a code owner April 12, 2025 13:34
@github-actions github-actions bot added c-update Modifies an existing proposal t-core labels Apr 12, 2025
@eth-bot
Copy link
Collaborator

eth-bot commented Apr 12, 2025

File EIPS/eip-7702.md

Requires 1 more reviewers from @adietrichs, @lightclient, @SamWilsn, @vbuterin

@eth-bot eth-bot added the a-review Waiting on author to review label Apr 12, 2025
@@ -100,6 +100,9 @@ following:
3. `authority = ecrecover(msg, y_parity, r, s)`
* `msg = keccak(MAGIC || rlp([chain_id, address, nonce]))`
* `s` value must be less than or equal to `secp256k1n/2`, as specified in [EIP-2](./eip-2.md).
* `r` value must be less than `secp256k1n`.
* `s` and `r` values must be non-zero.
* `y_parity` value must be either `0` or `1`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these checks are implicit in the ecrecover method but I think it is great to add this for clarification

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they are implicit, but we have found bugs in some EL clients like Besu that accepted values diff from the expected ones, leading to a chain split. Because of that, I think being more verbose here could be helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants