# Arbitrum Governor V2 Review **URL:** \ **Category:** ARDC Security Member\ **Created:** [August 6, 2024, 7:23pm UTC](https://forum.arbitrum.foundation/t/arbitrum-governor-v2-review/26118 "2024-08-06T19:23:13Z")\ **Posts on this page:** 1\ **Showing post:** 1
**Author:** ![openzeppelin](https://yyz1.discourse-cdn.com/flex029/user_avatar/forum.arbitrum.foundation/openzeppelin/32/10623_2.png) [@openzeppelin](https://forum.arbitrum.foundation/u/openzeppelin)\ **Post date:** [August 6, 2024, 7:23pm UTC](https://forum.arbitrum.foundation/t/arbitrum-governor-v2-review/26118/1 "2024-08-06T19:23:13Z")
## Summary A [recent proposal](https://forum.arbitrum.foundation/t/expand-tally-support-for-the-arbitrum-dao/22387) funded the development of new governor contracts with the aim to enhance the user experience when interacting with the Arbitrum ecosystem on [Tally](https://www.tally.xyz/gov/arbitrum). OpenZeppelin was asked by the ARDC to review the contributions from [ScopeLift](https://scopelift.co/) 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`](https://github.com/ScopeLift/arbitrum-governor-upgrade/tree/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea) commit. In scope were the following files: ```auto 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](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/governance/extensions/GovernorCountingFractionalUpgradeable.sol) 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](https://eips.ethereum.org/EIPS/eip-6372) - `eip712Domain` describes the domain separator that is used for verification of EIP-712 signatures (see [EIP-5267](https://eips.ethereum.org/EIPS/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](https://github.com/foundry-rs/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](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/script/BaseGovernorDeployer.sol#L25-L37). 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](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/dc44c9f1a4c3b10af99492eed84f83ed244203f6/contracts/proxy/ERC1967/ERC1967Upgrade.sol#L59-L64). _ **Update** : Resolved in [PR #23](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/23) at commit [`a4dd5ff`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/23/commits/a4dd5ffab63c3dc7e396e16b64d2c8da8048ff2e)_ ### Floating Pragma Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled. The [`GovernorCountingFractionalUpgradeable.sol`](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/lib/governance/extensions/GovernorCountingFractionalUpgradeable.sol#L3) file has the [`solidity ^0.8.20`](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/lib/governance/extensions/GovernorCountingFractionalUpgradeable.sol#L3) floating pragma directive. Consider using fixed pragma directives. _ **Update** : Resolved in [PR #26](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/26) at commit [`8813043`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/26/commits/88130438a3cd616376c2ea830e877c885912c8f9)_ ## 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”](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/script/SubmitUpgradeProposalScript.s.sol#L40). Consider making the change of governors part of the description. _ **Update** : Resolved in [PR #28](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/28) at commit [`918b32c`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/28/commits/918b32c2cd39704d12b0b731016a63a95386977f)_ ### 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`](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/L2ArbitrumGovernorV2.sol) and [`TimelockRolesUpgrader`](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/gov-action-contracts/TimelockRolesUpgrader.sol) 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](https://solidity.readthedocs.io/en/latest/natspec-format.html) (NatSpec). _ **Update** : Resolved in [PR #29](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/26) at commit [`1b26d60`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/29/commits/1b26d60b5842739d594dac7a4b51814b3572bf5e)_ ### 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: - The [`GovernorCountingFractionalUpgradeable` abstract contract](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/lib/governance/extensions/GovernorCountingFractionalUpgradeable.sol). - The [`L2ArbitrumGovernorV2` contract](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/L2ArbitrumGovernorV2.sol). - The [`TimelockRolesUpgrader` contract](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/gov-action-contracts/TimelockRolesUpgrader.sol). 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](https://wizard.openzeppelin.com/) and the [ethereum-lists](https://github.com/ethereum-lists/contracts#tracking-new-deployments). _ **Update** : Resolved in [PR #31](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/31) at commit [`766226b`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/31/commits/766226b98af522eaec85502221ebe31053251bff)_ ### Missing Named Parameters in Mappings Since [Solidity 0.8.18](https://github.com/ethereum/solidity/releases/tag/v0.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: - The [`usedVotes` state variable](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/lib/governance/extensions/GovernorCountingFractionalUpgradeable.sol#L49). - The [`_proposalVotes` state variable](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/lib/governance/extensions/GovernorCountingFractionalUpgradeable.sol#L57). Consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase. _ **Update** : Resolved in [PR #27](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/27) at commit [`79d4c31`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/27/commits/79d4c31d9597b5bdd3da8b70f270321105f3b73d)_ ### Unnecessary Casts Within `TimelockRolesUpgrader.sol`, there are two instances where `address` is cast to `address`: - The [`address(_governor)` cast](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/gov-action-contracts/TimelockRolesUpgrader.sol#L56). - The [`address(_governor)` cast](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/gov-action-contracts/TimelockRolesUpgrader.sol#L60). To improve the overall clarity, intent, and readability of the codebase, consider removing these casts. _ **Update** : Resolved in [PR #24](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/24) at commit [`5cfb15c`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/24/commits/5cfb15cda072a4ef7ec201cce93c94271c93d26a)_ ### Typographical Improvements There are a few minor typographical errors in a [comment block](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/L2ArbitrumGovernorV2.sol#L36-L43) of `L2ArbitrumGovernorV2`. These are - [Line 40](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/L2ArbitrumGovernorV2.sol#L40): “addresses” should be “address”. - [Line 41](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/L2ArbitrumGovernorV2.sol#L41): “address” should be “addresses”. - [Line 43](https://github.com/ScopeLift/arbitrum-governor-upgrade/blob/8ed945c799f5a8a54d3b5a19d686db8ac30a5dea/src/L2ArbitrumGovernorV2.sol#L43): “of” should be “or”. Consider fixing these for better readability. _ **Update** : Resolved in [PR #25](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/25) at commit [`1d77696`](https://github.com/ScopeLift/arbitrum-governor-upgrade/pull/25/commits/1d77696f0cc851f232f805f68898fdeb2019dcc2)_ ## Client Reported ### New Registry Contract Is Necessary Current [governance action contracts](https://github.com/ArbitrumFoundation/governance/tree/main/src/gov-action-contracts) ( **GAC** s) rely on an [`L2AddressRegistry` contract](https://arbiscan.io/address/0x56C4E9Eb6c63aCDD19AeC2b1a00e4f0d7aBda9d3#code) 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. --- _[View the full topic](https://forum.arbitrum.foundation/t/arbitrum-governor-v2-review/26118)._