Summary
Timeline: From 2024-09-16 To 2024-09-25
Total Issues: 5 (4 resolved)
Low Severity Issues: 5 (4 resolved)
Scope
OpenZeppelin started the review on pull request #1 of the RobinNagpal/comet repository at commit bb32bfb, and later on we were requested to confirm the changes at commit 368842d and commit f05d35c. Under review was the introduction of an alternative Compound governance process that would facilitate quicker and more efficient upgrades to the Comet markets.
In scope were the following files:
contracts
├── CometStorage.sol
├── Configurator.sol
├── CometProxyAdmin.sol
└── marketupdates
├── MarketAdminPermissionChecker.sol
├── MarketAdminPermissionCheckerInterface.sol
├── MarketUpdateProposer.sol
└── MarketUpdateTimelock.sol
System Overview
The scope introduces a new MarketUpdateTimelock
contract for Compound Governance, enabling the marketAdmin
(an EOA) to propose and execute parameter changes on Comet markets without requiring a community vote. The marketAdmin
submits these proposals to the MarketUpdateProposer
, where they are immediately queued for a period determined by the MarketUpdateTimelock
delay. After this delay, the marketAdmin
can execute the proposals to update market parameters via the Configurator
contract and upgrade the Comet markets via the CometProxyAdmin
.
This expedited process allows updates to supply and borrow kinks, interest rates, tracking speeds, and the minimum borrow limit. Additionally, the marketAdmin
can adjust the borrow, liquidate collateral, and liquidation factors of the collateral assets of the markets, along with their supply caps.
Security Model and Trust Assumptions
The market updates are intended to take an expedited path to execution via the new MarketUpdateTimelock
contract. Any proposals submitted by the marketAdmin
are immediately queued for a period of time set by the governor, presumably close to the minimum of 2 days, to facilitate efficient market updates. Although the governor has the ability to pause the market admin functionality, the governance process would take far longer than the time the market update needs to execute. Thus, Compound governance will be powerless to react to any market updates that are not in the best interest of the community and could be potentially disastrous for the Comet markets.
To alleviate this risk, two new roles have been introduced: the marketAdminPauseGuardian
, which has the ability to pause any updates originating from the marketAdmin
, and the proposalGuardian
, which can cancel any market update proposals that are not yet executed. These two parties play a crucial role in preventing harmful proposals from being executed. If the marketAdmin
goes rogue, the two roles must be set to different, independent, and trusted multisig accounts in order to avoid having a single point of failure. Unpausing of the market updates by the marketAdmin
can only happen through governance.
It is also expected that the market admin will execute queued proposals in a timely manner and avoid unnecessary delays without valid reasons. This ensures that users can better anticipate system updates, reducing uncertainty, and minimizing centralization.
Privileged Roles
- The
marketAdmin
can propose and execute parameter changes on Comet markets, bypassing the governance vote. - The
marketAdminPauseGuardian
can pause updates of parameters and deployments of new Comets. - The
proposalGuardian
can cancel a market update proposal while it is queued.
Low Severity
Missing Docstrings
Throughout the codebase, multiple instances of functions without docstrings were identified:
- The
propose
function of theMarketUpdateProposer
contract - The
getProposal
function of theMarketUpdateProposer
contract - The
state
function of theMarketUpdateProposer
contract - The
setDelay
of theMarketUpdateTimelock
contract - The
setGovernor
of theMarketUpdateTimelock
contract - The
setMarketUpdateProposer
of theMarketUpdateTimelock
contract - The
queueTransaction
of theMarketUpdateTimelock
contract - The
cancelTransaction
of theMarketUpdateTimelock
contract - The
executeTransaction
of theMarketUpdateTimelock
contract
Consider adding explanatory docstrings to all public and external functions to improve code clarity and usability.
Update: Resolved in commit 10e9275.
The MarketUpdateProposer
Does Not Enforce a Maximum Number of Operations
The propose
function of the MarketUpdateProposer
contract does not enforce a maximum number of operations within a single proposal. If a proposal contains an excessive amount of operations, it could potentially run out of gas.
Consider enforcing a limit similar to the proposalMaxOperations
function of the GovernorBravoDelegate
contract.
Update: Resolved in commit 10e9275 in line 156.
Gas Optimizations
Throughout the codebase, multiple opportunities for gas optimization were identified:
- The proposal description is unnecessarily stored in storage within the
propose
function. - The proposal is reassigned to storage in line 161.
- The
initialProposalId
could be a constant.
Consider addressing the above to improve the gas efficiency of the codebase.
Update: Resolved in commit 10e9275.
Code Redundancies
Throughout the codebase, multiple instances of redundant code were identified:
- Unnecessary casting in the
constructor
of theMarketUpdateProposer
contract - Unused named returns in the
getProposal
function of theMarketUpdateProposer
contract - Unnecessary use of the
add256
function of theMarketUpdateProposer
contract - Unnecessary use of
ownerOrMarketAdmin
modifier in the_upgrade
and_upgradeAndCall
private functions of theCometProxyAdmin
contract
Consider addressing the aforementioned instances of redundant code to improve the overall code quality, readability, and efficiency.
Update: Resolved in commits 10e9275and ad06d63.
Conflicts Between marketAdmin
Actions and Main Governance Proposals
The proposed alternative governance path has non-exclusive access to the following functionality in the Configurator
:
setSupplyKink
setSupplyPerYearInterestRateSlopeLow
setSupplyPerYearInterestRateSlopeHigh
setSupplyPerYearInterestRateBase
setBorrowKink
setBorrowPerYearInterestRateSlopeLow
setBorrowPerYearInterestRateSlopeHigh
setBorrowPerYearInterestRateBase
setBaseTrackingSupplySpeed
setBaseTrackingBorrowSpeed
setBaseBorrowMin
updateAssetBorrowCollateralFactor
updateAssetLiquidateCollateralFactor
updateAssetLiquidationFactor
updateAssetSupplyCap
There may still be proposals that must go through the main governance process because they involve other functions. When the main governance path is used to submit a proposal, all values that can be altered via the above functions should be explicitly set to mitigate the risk of conflicting changes made by the marketAdmin
.
For example, when only governance can change market parameters, it is correct to assume that the market parameter values after proposal enactment will only differ by the changes proposed. However, since the marketAdmin
has the power to override the current values before the execution of the governance proposal, that assumption no longer holds.
Setting all values, whether a change to the value is desired, is the only way to ensure the market is configued as intended after proposal enactment.
While this safeguard is effective, the marketAdmin
should avoid making changes that affect or are affected by in-progress or queued governance proposals.
Update: Acknowledged. The issue will be communicated to the marketAdmin
.
Conclusion
The alternative governance path introduced can facilitate faster market updates, enhancing market efficiency. However, it concentrates significant power in the hands of the
Market Admin and heavily relies on guardian roles to prevent potentially catastrophic proposals if the Market Admin acts maliciously. In such scenarios, governance would be unable to intervene. As such, we urge caution in integrating the contracts for providing alternative market updates. The community should carefully weigh the associated risks and benefits before deciding to adopt this approach.