Skip to content

fix: eip7702 cheatcodes multiple auth #10623

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 12 commits into
base: master
Choose a base branch
from

Conversation

zjesko
Copy link

@zjesko zjesko commented May 24, 2025

fixes #10551

Add the ability to have multiple active delegations and signed auth. Also takes cares of correctly incrementing the nonce.

vm.signAndAttach
vm.signAndAttach
vm.signAndAttach
performTx

now works correctly

@zjesko
Copy link
Author

zjesko commented May 24, 2025

@grandizzy @mattsse @DaniPopes
please review

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

thank you! good start, couple of things wrt nonce management

  • here we need to handle the case when authority_acc is already in active delegations (hence nonce already incremented)

// If we don't have a nonce then use next auth account nonce.
authority_acc.data.info.nonce + 1

so we need to walk through SignedAuthorization within active_delegations and increment the nonce of the last one for signer address (if any). For example if we have active delegations as
- alice with nonce 1
- alice with nonce 2
- bob with nonce 1
if we add another delegation for alice it should have nonce 3

  • the same check should be done when we attach delegation / validate next nonce here
    if authority_acc.data.info.nonce + 1 != auth.nonce {
    return Err("invalid nonce".into());
    }

if authority_acc present in active delegation list then we need to compare with that incremented one

  • since return Err("invalid nonce".into()); it's quite useless, could you enhance this one to include expected / current nonce and authority address?

// Increment nonce to reflect the signed authorization.
account.info.nonce += 1;
// Increment nonce to reflect the signed authorizations.
account.info.nonce += auth_count;
Copy link
Collaborator

Choose a reason for hiding this comment

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

here we need to walk through auth list and get the last nonce of sender address (if any), then increment and set, e.g if sender is alice with nonce 10 and auth list contains auth list as

  • bob with nonce 1
  • alice with nonce 11
  • bob with nonce 2
  • alice with nonce 12

then we need to set here alice's nonce to 13. If alice not present in auth list then add 1 to its current nonce (to reflect parent tx).

@zjesko
Copy link
Author

zjesko commented May 26, 2025

alright, thanks for the feedback will patch this quickly

@zjesko
Copy link
Author

zjesko commented May 27, 2025

hello @grandizzy I have added the fix as you described it, please let me know how it looks

also fixed the clippy and fmt errors in ci

@grandizzy grandizzy self-assigned this May 30, 2025
@grandizzy grandizzy added T-bug Type: bug C-forge Command: forge labels Jun 3, 2025
@grandizzy grandizzy self-requested a review June 3, 2025 07:04
grandizzy
grandizzy previously approved these changes Jun 3, 2025
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

thank you! lgtm, made couple of changes to reflect increment of sender nonce only (if also in auth list), fix fmt and test
waiting for other reviews before merging
CC @klkvr could you also pls check?

@grandizzy grandizzy moved this to Ready For Review in Foundry Jun 3, 2025
@grandizzy grandizzy requested a review from klkvr June 4, 2025 13:11
cshein45

This comment was marked as spam.

klkvr
klkvr previously requested changes Jun 4, 2025
Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

only have nits

broadcast: &Option<Broadcast>,
account_nonce: u64,
) -> u64 {
match active_delegations.iter().rfind(|auth| auth.recover_authority().unwrap() == authority) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid unwrap here as we have support for attaching arbitrary delegations and this might not be safe

Copy link
Collaborator

Choose a reason for hiding this comment

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

changed in 8e17fe8

@grandizzy grandizzy requested a review from klkvr June 4, 2025 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-bug Type: bug
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

bug(forge): EIP-7702 cheatcodes cannot handle multiple auth
6 participants