Risk of Incorrect Asset Pricing by LRTOracle in Case of Underlying Aggregator Reaching minAnswer

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L45-L47

Vulnerability details

Impact

Chainlink aggregators have a built-in circuit breaker to prevent the price of an asset from deviating outside a predefined price range. This circuit breaker may cause the oracle to persistently return the minPrice instead of the actual asset price in the event of a significant price drop, as witnessed during the LUNA crash. If we considered that [L-02] Avoid the use of deprecated Chainlink functions in the ChainlinkPriceOracle.sol is fixed and latestRoundData() is used to fetch the prices, the issue described in this report still exists.

Proof of Concept

In order for LRTOracle.sol to fetch the price for an asset in Asset/ETH exchange rate it is calling

function getAssetPrice(address asset) public view onlySupportedAsset(asset) returns (uint256) {
        return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset);
    }

which at the moment is fetching prices from Chainlink oracles. This is the function that fetches the price in ChainlinkPriceOracle.sol

 function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();
    }

The protocol as of now interacts with three tokens (stETH, rETH, cbETH). If an asset's price falls below the minPrice, the protocol continues to value the token at the minPrice rather than its real value. If a user decides to deposit stETH but the price of stETH is below the minAnswer user will receive more rsETH tokens than he is supposed to. This discrepancy could have the protocol end up minting drastically larger amount of rsETH.

Tools Used

Manual review

Recommended Mitigation Steps

ChainlinkPriceOracle.sol should cross-check the returned answer against the minPrice/maxPrice and revert if the answer is outside of these bounds. Once [L-02] Avoid the use of deprecated Chainlink functions is fixed, consider adding the following check to getAssetPrice() in the ChainlinkPriceOracle.sol contract

if (price >= maxPrice || price <= minPrice) revert();

Assessed type

Oracle

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #32

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

I personally participated and submitted this similar finding in the 2023-07-reserve-findings. It was invalidated per the sponsor's comment.

from chainlink: Hello @pmckelvy1 , there's no need to add this step as it would be redundant. If the answer falls outside the min/max values, the price feed won't update. All our feeds are set with a minimum value of 0 and a maximum value that's the highest possible number that can be generated. Please let me know if you've got any more questions. all min/max values are set at 0/UINT_MAX, so this step would be unnecessary

But I'll let the Kelp sponsor decide on this.

[manoj9april]

Hi @raymondfam, where can i find this min/max value for any chainlink price feed ?

[manoj9april]

We agree with chainlink as an oracle provider and we are okay with 2% price boundary.

[c4-sponsor]

manoj9april (sponsor) disputed

[raymondfam]

Hi @raymondfam, where can i find this min/max value for any chainlink price feed ?

It’s not publicly available on their website. You will need to write in to chainlink for this info but it will probably take them about 2 weeks to get you the same reply I quoted earlier.

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[fatherGoose1]

Lookout has provided helpful insight into the infeasibility of this report.

[AtanasDimulski]

@fatherGoose1 I want to reference a comment https://github.com/code-423n4/2023-11-kelp-findings/issues/696#issuecomment-1837051094. I agree with what @serial-coder has written and I would like to add that minAnswer issues historically have been judged as mediums. As can be seen from this link the minAnswer is not to 0. Would appreciate your feedback.

[adeolu98]

I will like to provide more recent information gotten from the chainlink api documentation which states that minAnswer/maxAnswer values is no longer used in most data feeds.

https://docs.chain.link/data-feeds/api-reference#variables-and-functions-in-accesscontrolledoffchainaggregator

With this in mind, it is recommended by chainlink that integrators evaluate their usecase and implement circuit breakers that verify price answer themselves.

example: stETH/ETH data feed code with no min/maxAnswer variable --> https://etherscan.io/address/0x86392dc19c0b719886221c78ab11eb8cf5c52812#code

[fatherGoose1]

@adeolu98 is correct. The oracles in question do not contain min/maxAnswer variables.