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 Rollback template to setup templates #149

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

Conversation

stevieraykatz
Copy link
Contributor

@stevieraykatz stevieraykatz commented Mar 29, 2024

TLDR; this PR implements an example script which shows that a rollback transaction should explicitly set the nonce returned by _getNonce() using an .env var. Hopefully this template will help avoid issues seen below.

Full context:

In executing mainnet/2024-03-05-pause-unpause-test we ran into an issue wherein the signatures were being incorrectly ordered by the execution call. This was leading to reverts since the Safe requires that signatures are arranged in address-ascending order.

The root cause was traced back to the fact that _getNonce was being overwritten by the following:

    function _getNonce(IGnosisSafe safe) internal override view returns (uint256 nonce) {
        uint256 _nonce = safe.nonce();
        console.log("Safe current nonce:", _nonce);
        console.log("Incrementing by 1 to account for planned `Pause` tx");
        return _nonce+1;
    }

This was fine for signing and simulating the transaction because these occurred before the preceding Pause transaction had been submitted. But when it came time to execute the transaction, this nonce increment was used in the execution context causing the hash used by ecrecover to differ from the correct hash. In turn, this lead to invalid addresses being returned which were then used to sort the signatures incorrectly.

@stevieraykatz stevieraykatz changed the title Add Rollback template to script Add Rollback template to setup templates Mar 29, 2024
Copy link
Collaborator

@nadir-akhtar-coinbase nadir-akhtar-coinbase left a comment

Choose a reason for hiding this comment

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

Overall onboard! These small changes might make it clearer, but at your discretion @stevieraykatz

// but both txs will be signed ahead of time. Need to explicitly override the nonce to
// ensure that the correct nonce is used in the sign, simulate and execution steps.
function _getNonce(IGnosisSafe) internal override view returns (uint256 nonce) {
nonce = vm.envUint("ROLLBACK_NONCE");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
nonce = vm.envUint("ROLLBACK_NONCE");
nonce = vm.envUint("EXPECTED_NONCE");

Comment on lines +40 to +42
// This transaction expects that there will be a `Pause` transaction before this one
// but both txs will be signed ahead of time. Need to explicitly override the nonce to
// ensure that the correct nonce is used in the sign, simulate and execution steps.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// This transaction expects that there will be a `Pause` transaction before this one
// but both txs will be signed ahead of time. Need to explicitly override the nonce to
// ensure that the correct nonce is used in the sign, simulate and execution steps.
// This transaction expects that there will be a `Pause` transaction before this one
// but both txs will be signed ahead of time. Need to explicitly override the nonce to
// ensure that the correct nonce is used in the sign, simulate and execution steps.
// Note that dynamically calculating the nonce will lead to errors in signature sorting,
// hence the use of an envvar to fix the nonce to a static value.

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.

None yet

2 participants