Skip to content

Conversation

@stevieraykatz
Copy link
Member

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

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

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
Contributor

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.

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jul 13, 2024

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@wbnns wbnns added the base protocol review To be reviewed by the Base web team label Apr 19, 2025
@jackchuma jackchuma deleted the add-tx-pair-template branch July 31, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

base protocol review To be reviewed by the Base web team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants