Compound ezETH, rsETH, weETH Price Feeds Audit
July 1-5, 2024
Total Issues: 7 (7 resolved)
Notes & Additional Information: 7 (7 resolved)
Scope
We audited pull requests #872 and #877 of the compound-finance/comet repository at commit 228b6e4 and 2c33985 respectively.
In scope were the following files:
PR #872
contracts
└── pricefeeds
├── vendor
│ └── IBalancerRateProvider.sol
├── EzETHExchangeRatePriceFeed.sol
└── ReverseMultiplicativePriceFeed.sol
PR #877
contracts
├── IRateProvider.sol
└── pricefeeds
├── vendor
│ └── kelp
│ └──ILRTOracle.sol
├── RateBasedScalingPriceFeed.sol
└── RsETHScalingPriceFeed.sol
System Overview
Compound v3, referred to hereafter as Comet, till now only used Chainlink as its source of asset prices. The contracts under review enable the usage of exchange-rate oracles for liquid staking and restaking tokens. These exchange rate oracles calculate the price of a token by dividing the TVL of the token’s protocol by the total supply of the token. The TVL in turn can be calculated using exchange rate oracles or market rate oracles.
A brief summary of the main contracts
EzETHExchangeRatePriceFeed
RsETHScalingPriceFeed
RateBasedScalingPriceFeed
- This is similar to the
ScalingPriceFeed contract but it works with oracles that have the getRate() function. This contract is not specific to any asset. It will be used for weETH but can be used with other assets too.
ReverseMultiplicativePriceFeed
- This is similar to the
MultiplicativePriceFeed contract but it multiplies the inverse of the second price feed with the first. It is useful in scenarios where an asset X only has a USD denominated price feed, but its price is required in ETH. This contract will multiply the X/USD price with the inverse of ETH/USD price to calculate the X/ETH price.
Trust Assumptions
Previously stated oracle risks from the Gauntlet team still seem to be present with the usage of the LRTOracle contract specifically the getAssetPrice function. A summary of such is that it utilizes multiple oracles to price the assets it accepts as deposits and this reliance on multiple oracles is risky. Now, the rsETHScalingPriceFeed feed doesn’t use this function but instead uses the public state variable rsETHPrice. This can be updated at any time by anyone with the updateRSETHPrice function in the LRTOracle. This function
- Gets all supported assets:
stETH, ETHx, sfrxETH and ETH.
- Gets the total deposits of each asset in the LRT Deposit Pool
- Gets the asset price from the
getAssetPrice using the different oracles gathered in the assetPriceOracle state variable.
- Out of the four asset oracles,
ETHx and sfrxETH use exchange rate oracles while stETH and ETH have fixed prices equal to 1e18 wei.
- Each asset amount is then multiplied by its respective price and then totaled
- Finally the total (amount * price) is divided by the
rsEth total supply and some checks are done for price limits.
Since ETHx and sfrxETH are using exchange rate oracles, the oracle risks stated by the Gauntlet team are present for the rsETHScalingPriceFeed contract. It is assumed that taking such risks has been weighed by the Woof Software team.
In addition, there is an assumption that the price feed contracts are going to be deployed with the correct configuration.
Notes & Additional Information
Typographical Error
Within the ReverseMultiplicativePriceFeed contract there is a typographical error with the public state variable priceFeedAScalling and it should be priceFeedAScale.
To improve code readability, consider addressing this typographical error and thoroughly examining the codebase to rectify any additional ones.
Update: Resolved in commit df7bab0.
Misleading Natspec Comments
In ReverseMultiplicativePriceFeed we have identified the following comments which may be misleading
- On line 33 of the
ReverseMultiplicativePriceFeed contract, the natspec for priceFeedAScaling incorrectly refers to it as combinedScale.
- On line 73 of the
ReverseMultiplicativePriceFeed contract, the stated formula does not match the implementation on the next line.
These can be misleading to future developers and users when attempting to understand the ReverseMultiplicativePriceFeed contract.
Consider revising the comments to remove ambiguity for others reviewing the codebase.
Update: Resolved in commit df7bab0.
Use signed256() instead of directly typecasting with int256()
Throughout the codebase, the uint256 values returned by the underlying price feeds are directly converted to int256 using int256(). Although, the likelihood of overflow is unlikely while typecasting these returned values, caution should still be taken on:
- line 71 of the
EzETHExchangeRatePriceFeed contract
- line 74 of the
ReverseMultiplicativePriceFeed
- line 70 of the
RateBasedScalingPriceFeed contract
- line 69 of the
RsETHScalingPriceFeed contract
Consider using the signed256 function present in all the listed contracts removing the risk of overflow while typecasting.
Update: Resolved EzETHExchangeRatePriceFeed and ReverseMultiplicativePriceFeed in commit df7bab0 and RateBasedScalingPriceFeed and RsETHScalingPriceFeed in commit f242649.
Constants Not Using UPPER_CASE Format
Throughout the codebase, there are constants not using the UPPER_CASE format. For instance:
According to the Solidity Style Guide, constants should be named with all capital letters with underscores separating words. For better readability, consider following this convention.
Update: Resolved RateBasedScalingPriceFeed and RsETHScalingPriceFeed in commit f242649 and EzETHExchangeRatePriceFeed in commit 2553a7d.
Missing Sanity Checks For Function Argument
Throughout the codebase, the constructor parameter decimals_ is used without any checks for the validity of the data:
Consider implementing sanity checks for the decimals_ constructor parameter as is done in other price feed contracts.
Update: Resolved RateBasedScalingPriceFeed and RsETHScalingPriceFeed in commit 563f2ba and EzETHExchangeRatePriceFeed in commit df7bab0.
The added sanity check in EzETHExchangeRatePriceFeed deems the shouldUpscale check unnecessary, since ezETHRateProviderDecimals is hardcoded to 18, thus, shouldUpscale would always be false. The code could be refactored to rescaleFactor = signed256(10 ** (ezETHRateProviderDecimals - decimals_)).
Use of External Call to Retrieve the decimals of priceFeedB is Inefficient
The function latestRoundData in the ReverseMultiplicativePriceFeed contract uses an external call to retrieve the decimals of priceFeedB, on line 74. Due to the unlikelihood of the decimals for a Chainlink price feed changing, making this call each time is unnecessary and inefficient.
Consider creating an immutable variable to be set to the scale of the priceFeedB decimals in the constructor, similar to priceFeedA.
Update: Resolved in commit df7bab0.
Using Zero-values for Unutilized Fields Contradicts Precedent
Throughout the codebase, the function latestRoundData returns zero-values for all uninitialized fields. For instance:
- Line 71 of the
EzETHExchangeRatePriceFeed contract.
- Line 70 of the
RateBasedScalingPriceFeed contract.
- Line 70 of the
RsETHScalingPriceFeed contract.
In each case, only the price is retrieved with the respective method from the underlying price feed and these feeds do not provide the other data fields. The Comet protocol when using the latestRoundData in the function getPrice does not use these other data fields. However, In the ConstantPriceFeed contract there are non-zero values being used for the non-essential fields. Meaning there is a precedent to return non-zero values for those non-essential fields.
Consider returning non-zero values for roundId, startedAt, updatedAt, and answeredInRound, to ensure consistency with previous practices.
Update: Resolved RateBasedScalingPriceFeed and RsETHScalingPriceFeed in commit f242649 and EzETHExchangeRatePriceFeed in commit df7bab0.
Conclusion
The audited contracts add some new types of price feed contracts to the Comet codebase. Of these contracts EzETHExchangeRatePriceFeed and RsETHScalingPriceFeed would enable the usage of exchange rate oracles for their respective assets. RateBasedScalingPriceFeed adds support for the use of oracles that retrieves the price via the getRate() function. Lastly, ReverseMultiplicativePriceFeed will allow the ability to combine price feeds in ways previously not possible. No critical nor high issues were found. Only Minor suggestions were made to improve the code’s overall quality. Based on our evaluation, we are confident that these additional price feed contracts will seamlessly integrate with the Comet codebase.
. 