L2ArbitrumGovernor: Add proposal cancellation and frontrunning protection#365
L2ArbitrumGovernor: Add proposal cancellation and frontrunning protection#365wildmolasses wants to merge 9 commits intoArbitrumFoundation:delegate-total-poc-fixedfrom
Conversation
* Add V2 governor and basic testing, scripts for use later * Store proposers and revert cancel if not proposer * Add governor action contract, upgrade script, and tests * Add `MultiProxyUpgradeAction` contract and upgrade through proxy * Make fns virtual, mapping internal, and add natspec in GovernorV2 * Fix nits and separate test helper from deployConstants * Fix nits * Update naming in gov upgrade action contract * Fix nits and update constants * Update src/L2ArbitrumGovernorV2.sol --------- Co-authored-by: Ed Mazurek <Edward.R.Mazurek@gmail.com>
* Add propose, queue, and execute tests * Add `vm.assume`s in test
Co-authored-by: Ed Mazurek <Edward.R.Mazurek@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds proposal cancellation functionality and frontrunning protection to the L2ArbitrumGovernor contract. The frontrunning protection mechanism allows proposers to restrict proposal submission to a specific address by including a #proposer=0x... suffix in the proposal description.
Key changes:
- Implemented
cancel()function allowing proposers to cancel their own pending proposals - Added frontrunning protection via
_isValidDescriptionForProposer()to prevent proposal hijacking - Enhanced test coverage with new test contracts for Cancel, Propose, Queue, and Execute operations
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/L2ArbitrumGovernor.sol | Added proposers mapping, cancel() function, and frontrunning protection logic in propose() |
| test/L2ArbitrumGovernor.t.sol | Refactored test structure with setUp, helper functions, and comprehensive test suites for cancellation and proposal lifecycle |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // timelock.initialize(1, stubAddressArray, stubAddressArray); | ||
|
|
There was a problem hiding this comment.
Remove commented-out code. This line appears to be dead code that should be deleted rather than left as a comment.
| // timelock.initialize(1, stubAddressArray, stubAddressArray); |
| function testFuzz_RevertIf_NotProposer(uint256 _randomSeed, address _actor) public { | ||
| address _proposer = createAndMintToProposer(_randomSeed); | ||
| vm.assume(_actor != _proposer); | ||
| // vm.assume(_actor != proxyAdmin); |
There was a problem hiding this comment.
Remove commented-out code. If this assumption is not needed, the comment should be deleted.
| // vm.assume(_actor != proxyAdmin); |
| if (47 < c && c < 58) { | ||
| return (true, c - 48); |
There was a problem hiding this comment.
Replace magic numbers with named constants for better readability. The ASCII values 47, 58, 48, 64, 71, 55, 96, 103, and 87 should be defined as constants like ASCII_ZERO_MINUS_ONE, ASCII_NINE_PLUS_ONE, ASCII_ZERO, etc.
This PR adds proposal cancellation and frontrunning protection to L2ArbitrumGovernor.
We refactored our audited L2ArbitrumGovernorV2 to make in-place modifications to L2ArbitrumGovernor. Hoping we can get this in for next week's audit, per request. We'd also like to add the tests from our audited branch to the repo, aiming for next week on that.
We've also got an Upgrade proposal script, a GAC for the UpgradeExecutor, etc, but because the governors are already slated for upgrade, these might be unnecessary. For now we won't include them.
Would love to sync as early as Monday with any concerns! Cheers.