July 10, 2023
This security assessment was prepared by OpenZeppelin.
Type : DeFi
Timeline : From 2023-06-20 : To 2023-06-26
Languages : Solidity
Total Issues : 3 (1 resolved)
Critical Severity Issues : 0 (0 resolved)
High Severity Issues : 0 (0 resolved)
Medium Severity Issues : 0 (0 resolved)
Low Severity Issues : 0 (0 resolved)
Notes & Additional Information : 3 (1 resolved)
Client Reported Issues : 0 (0 resolved)
We audited the compound-finance/comet repository at the 044600f commit, part of PR #757. The scope of this audit is one file:
- contracts/pricefeeds/MultiplicativePriceFeed.sol
We recently audited a similar smart contract: contracts/pricefeeds/WBTCPriceFeed.sol. That contract creates a more accurate price feed for WBTC by multiplying two different Chainlink price feeds together (WBTC/BTC and BTC/USD). This pattern has been abstracted into a genericized version of the contract that will allow arbitrary multiplicative feeds to be created at will, sharing the same behavior across all implementations.
The only new risk this audit will involve for the platform is one that the Compound protocol has been comfortable assuming in the past: using Chainlink oracles to price assets on the protocol. Careful consideration should be made when adopting these feeds, as not every price feed is equally robust. Chainlink's pricing feeds depend on the resilience of the Chainlink network, and the ability of aggregators and operators to provide accurate data. Chainlink themselves have created a grading scale to measure feeds' robustness. However, we recommend that the protocol conduct its own independent research before integrating new feeds.
In latestRoundData both priceFeedA and priceFeedB return metadata (e.g. startedAt, updatedAt, answeredInRound, & roundId). However, only priceFeedB's metadata is returned. This is clearly documented in the code and understood by the developers who rightly pointed out that this metadata is discarded when price-feeds are queried by the protocol. This note is to highlight that should this metadata be incorporated into the protocol in the future, clear reasoning will need to be made for why one feed's data should be preferred over another's. We recommend considering the question and coming to an agreement before such an issue should present itself.
Update: Acknowledged, not resolved. The Compound team stated:
Action: No change. We believe that the protocol would never have to use any of the non-price metadata. Any improvements to Compound’s oracle system would be made directly to the oracle itself. For example, to support staleness checks, the oracle itself would be responsible for checking that the
updatedAtmetadata is recent enough.
The public variable description, like decimals, is not meant to be updated or changed and indeed there is no avenue for modifying it. We recommend marking it immutable like decimals.
Update: Resolved. The Compound team stated:
Action: No change. Solidity does not currently support immutable strings. An immutable
byte32could be used, but we prefer not to restrict thedescriptionto only 32 bytes.
In MultiplicativePriceFeed.sol, the latestRoundData function calls out to the Chainlink data feeds. Should either feed fail to update in a timely manner, the prices in the Comet market would stop reflecting the market value of an asset accurately. Consider implementing a method for determining if price data is stale and determine what special handling it would require in the market.
Update: Acknowledged, not resolved. The Compound team stated:
## RecommendationsAction: Risk accepted, noted. Stale price data is definitely a risk that should be properly handled. Such a change would likely have to be introduced in a larger effort to re-design and improve Comet’s oracle system.
Financial
Medium: Compound relies entirely on Chainlink for accurate asset pricing and depends on its network, logic, and participants.
Consider monitoring the number of aggregators and node operators that are healthy and participating in any Chainlink price feeds you utilize. Further, consider monitoring the price feeds to see if they become stale.