Arbitrum Governor V2 Review

Arbitrum Research & Development Committee (ARDC) Security Member
1

Summary

A recent proposal funded the development of new governor contracts with the aim to enhance the user experience when interacting with the Arbitrum ecosystem on Tally. OpenZeppelin was asked by the ARDC to review the contributions from ScopeLift as well as their deployment scripts that create the proposal and enact the changes on-chain.

Timeline: 2024-07-08 – 2024-07-19
Total Issues: 9 (8 resolved)
Low Severity Issues: 2 (2 resolved)
Notes & Additional Information: 6 (6 resolved)
Client Reported Issues: 1 (0 resolved)

Scope

We reviewed the ScopeLift/arbitrum-governor-upgrade repository at the 8ed945c commit.

In scope were the following files:

arbitrum-governor-upgrade
├── src
│   ├── L2ArbitrumGovernorV2.sol
│   ├── lib
│   │   └── governance
│   │       └── extensions
│   │           └── GovernorCountingFractionalUpgradeable.sol
│   └── gov-action-contracts
│       └── TimelockRolesUpgrader.sol
└── script
    ├── BaseDeployer.sol
    ├── BaseGovernorDeployer.sol
    ├── DeployCoreGovernor.s.sol
    ├── DeployImplementation.s.sol
    ├── DeployTimelockRolesUpgrader.s.sol
    ├── DeployTreasuryGovernor.s.sol
    ├── SharedGovernorConstants.sol
    ├── SubmitUpgradeProposalScript.s.sol
    └── helpers
        └── CreateL2ArbSysProposal.sol

Files within the script directory were only reviewed for correctness in executing the intended operations.

System Overview

Arbitrum has two different governors on Arbitrum One which controls all voting for the DAO. Both Governors currently are proxy contracts which share an implementation contract but have slightly different parameters to enable their constitutional/non-constitutional functionality. The deployment scripts in scope deploy two new proxy contracts with the new L2ArbitrumGovernorV2 implementation. With these deployed, a proposal to switch to the new governors needs to create a call to the respective timelocks to remove the old governor’s proposer and canceller roles and add those roles for the new governors. Of course, this proposal needs to take its usual path of going from the L2 timelock, to the L1 timelock, and finally to the L2 executor which can make upgrade calls on the L2 timelock.

The changes to the governor contract were not drastic. L2ArbitrumGovernorV2, now allows proposers to cancel their proposal if it is pending. It also allows a new, fractional vote to be cast in proposal votes. Previously, a voter’s entire voting power had to go to a single option. With fractional voting, anyone can split their votes between options or not cast their votes with their entire voting power. We anticipate this to be very beneficial for contracts with delegated voting power that can now split their votes according to their own logic. The new governor uses updated, prerelease versions of the OpenZeppelin Contracts v5.1 library. Because of this, there are some new functions exposed on the new governor:

  • cancel is the new function proposers can use to cancel their pending proposal
  • CLOCK_MODE & clock provide the contract’s clock data as described in EIP-6372
  • eip712Domain describes the domain separator that is used for verification of EIP-712 signatures (see EIP-5267)
  • nonces will return the next nonce for a user. Useful for users/contracts who castVoteBySig.
  • proposalNeedsQueuing simply lets any caller know that proposals need to be queued before execution (i.e. it always returns true)
  • proposalProposer returns the proposer for any proposal id
  • usedVotes returns the voting power already cast for a proposal by a contract. This is useful now that users can split their voting power.

Trust Assumptions

The deployment scripts rely on Foundry to sign transactions and we assume that it works as intended and described.

Low Severity

Proxy Contract Initialization Should Be Done In One Step

Within BaseGovernorDeployer.sol, the deployment of the proxy and its initialization is done in two steps. Foundry will create transactions for each of these steps, meaning a determined actor could front-run the initialization step and render the deployment ineffective. The remedy would be to simply re-attempt the creation/initialization of these contracts. But to remove this risk entirely, consider encoding the initialize call as the _data parameter in the TransparentUpgradeableProxy constructor. The _data will be passed down to _upgradeToAndCall where it is used in a delegate call on the new implementation contract.

Update: Resolved in PR #23 at commit a4dd5ff

Floating Pragma

Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.

The GovernorCountingFractionalUpgradeable.sol file has the solidity ^0.8.20 floating pragma directive.

Consider using fixed pragma directives.

Update: Resolved in PR #26 at commit 8813043

Notes & Additional Information

Proposal Description Could Be More Descriptive

Within SubmitUpgradeProposalScript.s.sol, the proposal to change the governor is described as “Upgrade timelock roles”. Consider making the change of governors part of the description.

Update: Resolved in PR #28 at commit 918b32c

Missing Docstrings

Throughout the codebase, there are multiple code instances that do not have docstrings. Consider thoroughly documenting all functions (and their parameters) in both the L2ArbitrumGovernorV2 and TimelockRolesUpgrader contracts. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Update: Resolved in PR #29 at commit 1b26d60

Lack of Security Contact

Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.

Throughout the codebase, there are contracts that do not have a security contact:

Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Resolved in PR #31 at commit 766226b

Missing Named Parameters in Mappings

Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping’s purpose.

Within GovernorCountingFractionalUpgradeable.sol, there are multiple mappings without named parameters:

Consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.

Update: Resolved in PR #27 at commit 79d4c31

Unnecessary Casts

Within TimelockRolesUpgrader.sol, there are two instances where address is cast to address:

To improve the overall clarity, intent, and readability of the codebase, consider removing these casts.

Update: Resolved in PR #24 at commit 5cfb15c

Typographical Improvements

There are a few minor typographical errors in a comment block of L2ArbitrumGovernorV2. These are

  • Line 40: “addresses” should be “address”.
  • Line 41: “address” should be “addresses”.
  • Line 43: “of” should be “or”.

Consider fixing these for better readability.

Update: Resolved in PR #25 at commit 1d77696

Client Reported

New Registry Contract Is Necessary

Current governance action contracts (GACs) rely on an L2AddressRegistry contract to provide data and check for correct implementations. Consider creating a deployment script for a new registry contract with the new governor address.

Conclusion

During this engagement we reviewed a new version of Arbitrum’s governance contract, which now allows voters to split votes and proposers to cancel their own pending proposals. Along with the contracts, we reviewed the scripts that are going to be used for deploying said contracts and those that will be used for proposing the relevant role changes on the respective time lock contracts. We found the codebase to be of high quality and have only found Low and Note severity issues. We found the test suite to be comprehensive and well written.

4 Likes
2

hey @openzeppelin why did you started this post as an “audit”, and then edited it to be a “review” ?

Did you audited the code, or did you reviewed this code?

CleanShot 2024-09-13 at 02.07.49@2x
CleanShot 2024-09-13 at 02.07.49@2x3072×1390 374 KB

3

The ARDC’s DAO Advocate, L2Beats, requested that we call the report a “review” rather than an “audit”. However, our level of scrutiny was equal to that of our standard audit.

2 Likes