-
Notifications
You must be signed in to change notification settings - Fork 118
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonce = vm.envUint("ROLLBACK_NONCE"); | |
nonce = vm.envUint("EXPECTED_NONCE"); |
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
Review Error for OKEAMAH @ 2024-07-13 21:31:35 UTC |
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: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 thehash
used byecrecover
to differ from the correcthash
. In turn, this lead to invalid addresses being returned which were then used to sort the signatures incorrectly.