Compound Governor Upgrade Audit

Summary

Timeline: From 2024-11-18 To 2024-12-23

Total Issues: 15 (11 resolved)

High Severity Issues: 2 (2 resolved)

Medium Severity Issues: 1 (1 resolved)

Low Severity Issues: 5 (4 resolved)

Notes & Additional Information: 7 (4 resolved)

Scope

OpenZeppelin reviewed pull request #20 of the compound-finance/compound-governance repository at commit f78cc0f.

In scope were the following files:

contracts
├── CompoundGovernor.sol
└── extensions
    ├── GovernorCountingFractionalUpgradeable.sol
    ├── GovernorCountingSimpleUpgradeable.sol
    ├── GovernorPreventLateQuorumUpgradeable.sol
    ├── GovernorSequentialProposalIdUpgradeable.sol
    ├── GovernorSettableFixedQuorumUpgradeable.sol
    ├── GovernorSettingsUpgradeable.sol
    ├── GovernorTimelockCompoundUpgradeable.sol
    ├── GovernorUpgradeable.sol
    ├── GovernorVotesCompUpgradeable.sol
    └── IGovernor.sol
script
├── CompoundGovernorConstants.sol
├── DeployCompoundGovernor.s.sol
└── ProposeUpgradeBravoToCompoundGovernor.s.sol

System Overview

The audited code focuses on a redesign of Compound’s governance contracts and incorporates OpenZeppelin’s upgradeable governance library. This redesign replaces the GovernorBravoDelegator and GovernorBravoDelegate contracts with tailored implementations of the OpenZeppelin upgradeable governance library, ensuring partial compatibility with the original GovernorBravo functionality while introducing several key improvements:

  • ERC-1271 Compatibility: Enhances support for functions such as castVoteBySig, enabling seamless signature verification by smart wallets and other contracts. This addition broadens the range of participants in governance processes by accommodating both EOAs and contract-based accounts.
  • Dynamic Governor Settings: Parameters like proposalThreshold, votingDelay, and votingPeriod are now fully adjustable without minimum or maximum limits, offering greater flexibility for the DAO.
  • Configurable Quorum: The quorum remains non-fractional and is now updatable through a simple, custom quorum extension contract.
  • Late Quorum Prevention: A new mechanism has been introduced to automatically extend the voting period if quorum is reached late in the voting period. This helps mitigate governance attacks.
  • Unlimited Proposal Actions: The CompoundGovernor contract eliminates the cap on the number of actions a proposal can include, resulting in increased flexibility for governance decisions.
  • Fractional Voting: The redesigned governance supports fractional voting, allowing users to allocate their voting power proportionally across “for”, “against”, and “abstain” options in a single proposal. This flexibility enables smart contract delegates to implement customized voting strategies, improving the governance process while accommodating diverse preferences.

Certain features from GovernorBravo were preserved to ensure seamless continuity and maintain compatibility with existing governance operations:

  • Proposal and Whitelist Guardians: The proposal cancellation mechanism remains unchanged, retaining DAO control over critical roles, such as managing whitelisted proposers and overseeing proposal integrity.
  • Sequential Proposal IDs: Proposal IDs are incrementally assigned instead of being derived from proposal content, ensuring predictability and compatibility with off-chain components. This approach, implemented based on the GovernorStorage contract, simplifies indexing and helps maintain consistent references across various tools and integrations.

However, the architectural redesign of the CompoundGovernor introduces some breaking changes:

  • Removal of Off-Chain Proposal Creation: The proposeBySig function, which previously allowed initiating proposals via off-chain signatures, is no longer supported. As a result, all new proposals must now be created through on-chain submissions, increasing friction for users who previously relied on gasless, off-chain workflows.
  • Alteration of Off-Chain Voting: The original castVoteWithReasonBySig has been replaced by castVoteWithReasonAndParamsBySig. While the new function can replicate the old behavior by passing default parameters to the function, existing integrations that relied in the earlier function will need adjustment. This change does not remove the underlying off-chain voting capability; stakeholders can continue casting votes off-chain and finalize them on-chain with minimal additional overhead.
  • Transition to Function Selectors in calldata: The CompoundGovernor contract no longer supports string function signatures. Instead, function selectors are now incorporated directly into the calldata and stored alongside the descriptionHash. As a result, the txHash stored in the Timelock contract now consists of an empty-string signature and the 4-byte function selector appended to the data field.
  • Replacement of getActions with proposalDetails: The getActions function has been removed, with proposalDetails being offered as an alternative for retrieving detailed proposal information. The data structure has changed, omitting the signature, embedding the function selector directly into the calldata, and returning the descriptionHash as a new field.
  • Removal of receipts in Favor of hasVoted and usedVotes: receipts, which were used to query voter participation via getReceipt, are no longer supported. Now, the hasVoted and usedVotes functions are to be used instead.

The Governor interface has undergone substantial changes in the redesign. These include the addition of new functionalities, modifications to data storage structures, and the removal of support for certain previously available functions. These changes aim to enhance flexibility and extensibility but may introduce risks for Governance and compatibility challenges with existing tools or integrations.

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:

  1. 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.
  2. 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.
  3. 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:

  1. Proposal ID Alignment: PR #87 at commit f845299 modifies the setNextProposalId function to correctly assign the next proposal ID.
  2. 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.