Market Admin Audit

1

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:

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 the MarketUpdateProposer contract
  • Unused named returns in the getProposal function of the MarketUpdateProposer contract
  • Unnecessary use of the add256 function of the MarketUpdateProposer contract
  • Unnecessary use of ownerOrMarketAdmin modifier in the _upgrade and _upgradeAndCall private functions of the CometProxyAdmin 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.

1 Like