Skip to content
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

add: resetNonce cheatcode #5033

Merged
merged 5 commits into from
May 25, 2023
Merged

Conversation

joaquinlpereyra
Copy link
Contributor

@joaquinlpereyra joaquinlpereyra commented May 24, 2023

Motivation

Right now, it is impossible to reset the nonce of an account to zero. Surprisingly, this is the only thing missing to be able to mimic SELFDESTRUCT almost to the letter. This has been a complaint in the past and it is also giving us trouble when trying to reproduce some advanced attacks on learn-evm-attacks.

Solution

Add a very simple resetNonce cheatcode to Foundry. This cheatcode simply resets an account nonce to zero. There are already other cheatcodes that modify the nonce of an account, so this one should not be particularly controversial. A new cheatcode resetNonce is added instead of modifying the requirement of setNonce to be only incremental to maintain backwards compatibility and also to make the new behavior explicit.

There's a companion PR on Forge-std which adds a destroyAccount cheatcode.

@mattsse
Copy link
Member

mattsse commented May 24, 2023

Right now, it is impossible to reset the nonce of an account to zero.

why does setNonce(addr, 0) not work here?

or is this for convenience?

@Evalir Evalir requested review from mattsse and Evalir May 24, 2023 19:10
@joaquinlpereyra
Copy link
Contributor Author

@MATTSE setNonce cannot decrement the nonce, see:

        HEVMCalls::SetNonce(inner) => {
            with_journaled_account(
                &mut data.journaled_state,
                data.db,
                inner.0,
                |account| -> Result {
                    // nonce must increment only
                    let current = account.info.nonce;
                    let new = inner.1;
                    ensure!(
                        new >= current,
                        "New nonce ({new}) must be strictly equal to or higher than the \
                         account's current nonce ({current})."
                    );
                    account.info.nonce = new;
                    Ok(Bytes::new())
                },
            )??
        }

@mds1
Copy link
Collaborator

mds1 commented May 24, 2023

A new cheatcode resetNonce is added instead of modifying the requirement of setNonce to be only incremental to maintain backwards compatibility and also to make the new behavior explicit.

I like this approach: most users will only need setNonce so having the safety check to avoid footguns is nice, and for the less common selfdestruct case a dedicated resetNonce can be used. An alternative that I'd also support is setNonceUnsafe(address,uint64) which just lets you decrease the value, and of course 0 is a valid input there. This might be more flexible than a resetNonce cheat, though I don't know when you'd want to decrement without making it zero, so maybe it's not beneficial

@joaquinlpereyra
Copy link
Contributor Author

@mds1 I thought about setNonceUnsafe but:

  1. I personally dislike safe/unsafe options generally speaking
  2. Couldn't find an usecase to set an arbitrary nonoce
  3. I wanted the PR to be as uncontroversial as possible :)

Honestly (3) was the biggest factor here. As for (2)... well, evidently no one saw an usecase for setting the nonce to zero before either so 🤷 not a big factor there. (1) is of course not really important.

@mds1
Copy link
Collaborator

mds1 commented May 24, 2023

@joaquinlpereyra yea that's all fair, I'm down to move forward with resetNonce then as long as @mattsse and @Evalir are onboard

edit: ah I see @mattsse just commented about preferring the other approach 😅 ultimately I'm pretty indifferent here so up to you all

@mattsse
Copy link
Member

mattsse commented May 24, 2023

I see,

I'd like to go with @mds1 suggestion:

Add a new cheatcode that "unsafely" sets the nonce, this is basically decreasing the nonce, so perhaps the name should reflect this; like downgradeNonceTo or something and resetNonce just calls this with a 0

@joaquinlpereyra
Copy link
Contributor Author

@mattsse OK. I'm with @mds1 on this, pretty indifferent. So I have no problems with the new approach. So I will:

  • keep resetNonce with hardcoded zero
  • add setNonceUnsafe with arbirtrary values

@joaquinlpereyra
Copy link
Contributor Author

joaquinlpereyra commented May 24, 2023

OK! Done. There are a few tests failing (they come and go 👀 ) but I don't think they are related to this functionality. Lemme know if they are.

Edit: found some issues, already pushing a new commit :)

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm!

ty for the tests!

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

Looks good! Just some comments on the comments.

Might also need to run cargo +nightly fmt & cargo +nightly clippy and fix linting warnings to make tests pass

testdata/cheats/Cheats.sol Outdated Show resolved Hide resolved
evm/src/executor/inspector/cheatcodes/env.rs Outdated Show resolved Hide resolved
@joaquinlpereyra
Copy link
Contributor Author

@Evalir Awesome, thanks for spotting those. I've fixed the comments and ran the nightly linters ✔️

@mattsse
Copy link
Member

mattsse commented May 25, 2023

failing test unrelated

@joaquinlpereyra mind adding them to forge-std next: foundry-rs/forge-std#390

@mattsse mattsse merged commit aa25054 into foundry-rs:master May 25, 2023
data.db,
inner.0,
|account| -> Result {
account.info.nonce = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing I just thought of is that contract nonces start at 1, so we probably need to check if the account has code and then set it to 1 or 0 based on that: https://eips.ethereum.org/EIPS/eip-161

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pulled into #5043 to track

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants