Security Model and Trust Assumptions
While the security model of the CompoundGovernor
generally aligns with that of GovernorBravo
, it introduces several key changes that significantly diverge, creating new risks that require careful consideration:
-
Risks from Complex Proposals:
- By removing the cap on the number of actions per proposal, flexibility is increased; however, this may lead to proposals that exceed the block gas limit and cannot be executed.
- Highly intricate proposals may overwhelm the community’s capacity to review them thoroughly, increasing the likelihood of vulnerabilities or exploits going undetected.
-
Governance Manipulation Risks from Parameter Flexibility:
- The removal of predefined constraints on governance parameters, such as
proposalThreshold
, votingDelay
, votingPeriod
, and quorum
, introduces flexibility but increases susceptibility to governance attacks. While not directly facilitating such attacks, these parameters could be strategically leveraged by malicious actors to amplify the attack impact.
-
Single-Step Admin Transfer of CompoundGovernor
:
- The
updateTimelock
function of the GovernorTimelockCompoundUpgradeable
contract allows governance to set a new timelock in a single step. While this relies on user review of governance proposals, an improper timelock update — whether accidental or malicious — could render the Governor contract unable to queue, cancel, or execute transactions on the timelock, including updating the timelock itself. While this functionality can facilitate quick upgrades during crises, it also introduces the risk of permanently rendering governance unusable if the timelock is improperly updated.
While the aforementioned changes enhance flexibility and customization, they also necessitate strict oversight and proactive safeguards to maintain governance integrity and mitigate potential exploitation.
After the migration proposal for the Governor is executed on-chain, no further proposals should be made on the GovernorBravo
contract, as they will become inexecutable once the Timelock admin is updated to the new CompoundGovernor
. We assume that, at the time of the Governor upgrade migration execution, the proposalCount
of the GovernorBravo
contract will match the proposal ID of the Governor upgrade proposal.
To ensure a seamless transition, any new proposals must be submitted to the CompoundGovernor
after the migration is completed (approximately 6.58 days following the proposal’s submission). Proposals submitted before this time will fail, as the _nextProposalId
is initialized to type(uint256).max
, and any attempt to propose would cause an overflow in line 104. The migration will invoke the setNextProposalId
function to correctly set the proposal count, ensuring continuity in proposal IDs from the GovernorBravo
and allowing new proposals to start being processed by the CompoundGovernor
.
Privileged Roles
The redesign retains the same privileged roles initially present in Compound’s GovernorBravo
without introducing any new roles. The privileged roles include:
- The
proposalGuardian
role: This role grants the ability to cancel any proposal. The role assignment is time-bound and expires after a governance-defined period.
- The
whitelistGuardian
role: This role can add proposers to the whitelist for a specific duration. Whitelisted proposers are able to submit proposals without meeting the required delegate vote threshold. This role also has the authority to cancel proposals from whitelisted proposers if they fail to meet the vote threshold.
High Severity
Whitelisted Users Cannot Propose with Less Than proposalThreshold
Delegate Votes
Non-expired whitelisted users, as set by the governance or whitelistGuardian
, cannot propose
if their delegate votes are less than the proposalThreshold
. This contradicts the current GovernorBravo
implementation and renders the whitelist ineffective.
Consider allowing whitelisted users to propose regardless of their voting power.
Update: Resolved in PR #89 at commit 4e3bc06.
The proposalGuardian
Role Can Expire but Retain Cancel Authority
In the CompoundGovernor
contract’s cancel
function, proposals can be canceled by the proposalGuardian
without verifying whether the role has expired. While the proposalGuardian
is a trusted role, if compromised, a malicious actor could misuse the authority to block all proposals, including those aimed at replacing the proposalGuardian
itself, effectively causing a denial-of-service on Compound’s governance.
Consider enforcing that the proposalGuardian
role must be active before it can cancel proposals.
Update: Resolved in PR #88 at commit ebcf2a5.
Medium Severity
Abstentions Count Toward Quorum Requirements
In the _quorumReached
function, abstainVotes
are calculated as part of the quorum calculation. This differs from the behavior present in GovernorBravo
, where a proposal succeeds only if the forVotes
exceed the quorum. While abstainVotes
typically constitute a small fraction of the total votes, their inclusion in the quorum calculation may be confusing and contradictory to discussions on the forum. This could lead to proposals passing with fewer than the intended 400,000 forVotes
.
Consider modifying the _quorumReached
function to calculate quorum based only on forVotes
to align with expectations and avoid potential discrepancies.
Update: Resolved in PR #86 at commit 9bcc3a0.
Low Severity
Namespace ID Does Not Adhere to ERC-7201 Specification
The current namespace annotation in the GovernorSettableFixedQuorumStorage
struct does not comply with the ERC-7201 standard. According to the ERC-7201 specification, a struct’s <NAMESPACE_ID>
in the @custom:storage-location <FORMULA_ID>:<NAMESPACE_ID>
annotation should be used in the formula to calculate the storage slot as follows:
keccak256(abi.encode(uint256(keccak256(bytes(<NAMESPACE_ID>))) - 1)) & ~bytes32(uint256(0xff))
However, the current tag uses openzeppelin.storage.GovernorSettableFixedQuorum
as the namespace ID, whereas the following formula is used to compute the storage location:
keccak256(abi.encode(uint256(keccak256("storage.GovernorSettableFixedQuorum")) - 1)) & ~bytes32(uint256(0xff));
The inclusion of the openzeppelin.
prefix in the namespace ID is misleading as it is not part of the computed formula. This causes the annotation tag to be misaligned with the specification.
To ensure compliance with ERC-7201 by accurately representing the namespace ID, consider updating the tag to the following:
/// @custom:storage-location erc7201:storage.GovernorSettableFixedQuorum
Update: Resolved in PR #85 at commit 1892c87.
Proposal ID Counter Set Incorrectly
The setNextProposalId
function sets the _nextProposalId
to match the current proposal counter on GovernorBravo
. The function is designed to be called by the timelock during the migration to ensure seamless continuity of the proposal count. However, the function sets _nextProposalId
to the current proposalCount
, which is then returned by the _propose
function in line 93. This approach results in _nextProposalId
being assigned the ID of the last proposal on GovernorBravo
instead of a new, unique ID for the newly created proposal. This could lead to complications with off-chain components and may cause confusion within the community.
Consider revising the logic in setNextProposalId
to ensure that the next proposal ID is correctly assigned while preserving the accuracy of the proposalCount
function.
Update: The issue has been resolved through two key updates:
- Proposal ID Alignment: PR #87 at commit f845299 modifies the
setNextProposalId
function to correctly assign the next proposal ID.
- Proposal Count Correction: PR #91 at commit 4c25a43 updates the
proposalCount
function to accurately return the total number of proposals. This adjustment addresses an off-by-one error that had been introduced by the changes in PR #87, ensuring the proposal count remains consistent and reliable.
Proposers Can Spam Governance
The CompoundGovernor
contract does not impose any restrictions on the number of proposals a proposer can initiate at a given time. This lack of constraints could enable malicious delegates or whitelisted users to flood Compound Governance with numerous proposals, potentially overwhelming the governance process.
Consider implementing a restriction that limits proposers to only have one live proposal at a time, similar to the limitation in GovernorBravo
. This would help prevent governance spamming while ensuring that proposals are more carefully considered.
Update: Resolved in PR #90 at commit 987fe78.
Incorrect NatSpec Storage Location Annotation
According to the ERC-7201 standard, structs should be annotated with the @custom:storage-location <FORMULA_ID>:<NAMESPACE_ID>
NatSpec tag, where <FORMULA_ID>
is defined by the ERC used. Since the storage slots are defined by the ERC-7201 keccak256(keccak256(id) - 1) & ~0xff
formula, the structs should be annotated with erc7201:<NAMESPACE_ID>
.
However, the GovernorVotesCompStorage
struct uses the @custom:storage-location IComp:storage.GovernorVotesComp
annotation, erroneously specifying the <FORMULA_ID>
as IComp
. Using a non-standard <FORMULA_ID>
introduces ambiguity and could result in compatibility issues with tools and applications that adhere to the ERC-7201 standard.
Consider adopting the erc7201
formula identifier (as specified in the ERC-7201 standard) for the @custom:storage-location
annotation. For example:
/// @custom:storage-location erc7201:storage.GovernorVotesComp
This ensures adherence to the ERC-7201 standard and prevents potential ambiguities when interpreting the storage location.
Update: Resolved in PR #84 at commit 7962809.
Missing ERC-7201 NatSpec Tag
The GovernorSequentialProposalIdUpgradeable
contract lacks the @custom:storage-location erc7201:<NAMESPACE_ID>
NatSpec tag in the GovernorSequentialProposalIdStorage
struct. This annotation is vital for ensuring compliance with the ERC-7201 standard, helping developers and tools identify the role, intent, and utility of the struct along with its storage location. The absence of this tag could lead to confusion or integration challenges with systems relying on ERC-7201 compatibility.
Consider adding the @custom:storage-location erc7201:<NAMESPACE_ID>
NatSpec tag to the GovernorSequentialProposalIdStorage
struct in order to improve clarity and ensure ERC-7201 compliance.
Update: Resolved in PR #82 at commit 75e801a.
Notes & Additional Information
Direct Use of _checkGovernance
Instead of onlyGovernance
Modifier
In the CompoundGovernor
contract, the setWhitelistGuardian
and setProposalGuardian
functions directly call the internal _checkGovernance
function instead of using the onlyGovernance
modifier. This approach could be less consistent with typical access control patterns and might affect code clarity.
Consider using the onlyGovernance
modifier for better readability and alignment with standard practices.
Update: Acknowledged, not resolved. The ScopeLift team stated:
This is a code style concern with no impact to the deployed contract logic. We prefer the clarity internal functions provide.
Inconsistent Version in IGovernor
The IGovernor
interface uses OpenZeppelin Contracts v.5.1.0-rc.0, whereas other libraries use v.5.1.0. Although there are no relevant code changes between the two versions, it is nonetheless best practice to consistently use the stable release version.
Consider updating IGovernor
to align with the stable release version.
Update: Resolved in PR #80 at commit 07afede.
Unnecessary Casting
In the __GovernorVotesComp_init_unchained
function, there is an unnecessary casting operation that converts _tokenAddress
to an address
and then back to IComp
.
Consider removing the aforementioned unnecessary casting in the __GovernorVotesComp_init_unchained
function to improve code clarity and maintainability.
Update: Resolved in PR #81 at commit 820454.
CompoundGovernor
Does Not Use ERC-7201 Storage Layout
The CompoundGovernor
contract does not currently utilize the ERC-7201 storage layout specification. While the parent contracts follow the namespace storage layout, adopting the ERC-7201 standard for the CompoundGovernor
contract can help prevent storage collisions in future upgrades.
Consider using the ERC-7201 storage layout for the CompoundGovernor
contract to ensure more reliable storage management and avoid potential conflicts in future contract upgrades.
Update: Acknowledged, not resolved. The ScopeLift team stated:
We acknowledge that CompoundGovernor does not use ERC-7201 storage layout. However, the contracts it extends do use it, which makes the storage locations easier to reason about. We’ll mark as “won’t fix” for now, but would gladly defer to community preference here.
CountingFractionalUpgradeable
Initializer Not Being Called
In the initializer
function of the CompoundGovernor
contract, the __GovernorCountingFractional_init
function is not being called to initialize the GovernorCountingFractionalUpgradeable
contract. While the function currently does not contain any logic, it is considered best practice to call empty initializers as they may be updated to include logic in future versions.
Consider calling the __GovernorCountingFractional_init
function from the initializer
function.
Update: Resolved in PR #83 at commit 16ea8b5.
Typographical Error
Typographical errors may lead to confusion and reduced code clarity. One such error was identified in line 2 of the IGovernor.sol
contract, where the word suppert
should be support
.
Consider addressing the aforementioned typographical error to improve the clarity and readability of the codebase.
Update: Resolved in PR #79 at commit a40086a.
Unused Function in GovernorSequentialProposalIdUpgradeable
The cancel function of the GovernorSequentialProposalIdUpgradeable
contract is currently redundant as it is not called within the CompoundGovernor
implementation. Instead, the CompoundGovernor
contract overrides cancel
and subsequently calls the _cancel
function of the GovernorTimelockCompoundUpgradeable
contract. This renders the cancel
function defined in GovernorSequentialProposalIdUpgradeable
unused and unnecessary.
Consider removing the unused cancel
function from the GovernorSequentialProposalIdUpgradeable
contract to streamline the code, reducing the contract size, and avoiding potential confusion for developers. If this function is intended for future use or serves as a placeholder, include detailed documentation to clarify its purpose and intended use case.
Update: Acknowledged, not resolved. The ScopeLift team stated:
The contract was written in the spirit of a typical extension contract that could be reused. It’s of no consequence to the deployed contract logic.
Conclusion
The redesign of Compound’s governance contracts — underpinned by OpenZeppelin’s upgradeable libraries — marks a significant evolution of Compound Governance, introducing greater flexibility, extensibility, and functionality. The audit identified several concerning issues, particularly involving privileged roles and the relaxation of previously established governance restrictions, which posed notable security and operational challenges. Thanks to the prompt and diligent efforts of the ScopeLift team, all identified vulnerabilities have been addressed, bolstering the system’s overall security posture.
Nonetheless, certain elevated risks remain as a natural consequence of this heightened flexibility. These residual concerns demand careful attention and ongoing oversight. Off-chain tools and integrations, in particular, must be updated to align with the new architecture and avoid operational disruptions. The migration scripts present no known issues, yet the final proposal calldata must be thoroughly examined to ensure that every element of the transition is both secure and seamless.