Skip to content

fix(evm): check for global failure instead of DSTest flag when reverting snapshots #5404

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 10 commits into from
Jul 17, 2023

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Jul 15, 2023

Motivation

Closes #3792.

The root issue of these soundness issues comes from storage layout shifts depending on the inheritance order of the test contract. We, right now, check for the _failed flag on DSTest, which, assuming the user has the correct inheritance order, is a valid way to ensure we're keeping track of snapshot failures. However, if it's not correct, the flag will not be at the expected slot, and therefore we'll get unsound behavior.

Solution

What we do instead is now rely on the global failure slot that dstest keeps on the hevm address (keccak256("failed")) to check for failures. This should avoid any unsoundness issues regarding the inheritance order when it comes to snapshots.

@Evalir Evalir requested review from mattsse and mds1 July 15, 2023 23:10
@Evalir Evalir force-pushed the evalir/solve-snapshot-soundness branch from 073ca47 to f147cb1 Compare July 16, 2023 00:52
Copy link
Member Author

@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.

I'm a bit suspicious of the is_global_failure function. While debugging, the "failed" slot should absolutely be set to 1 on repro 3055, but the function is not returning this. However, logging the slot with dstest will show that the slot is indeed 1. Will check if there's something wrong in this func tomorrow

EDIT: Just needed same treatment as the usual way to check contract state—check present value.

@Evalir Evalir added the A-evm Area: EVM label Jul 17, 2023
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.

nice find!

needs some docs for the slot

@@ -65,6 +65,9 @@ type ForkLookupIndex = usize;
const DEFAULT_PERSISTENT_ACCOUNTS: [H160; 3] =
[CHEATCODE_ADDRESS, DEFAULT_CREATE2_DEPLOYER, CALLER];

const GLOBAL_FAILURE_SLOT: &str =
Copy link
Member

Choose a reason for hiding this comment

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

needs docs that this is keccak("failed")

Copy link
Member Author

Choose a reason for hiding this comment

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

done! a75f9fb

a note: this is actually only "failed" in bytes and padded—not keccak("failed")

@Evalir Evalir merged commit e00c0a0 into master Jul 17, 2023
@Evalir Evalir deleted the evalir/solve-snapshot-soundness branch July 17, 2023 20:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-evm Area: EVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test reverts with Assertion failed. when reverting to a fork snapshot, without any assertion
2 participants