`depositAsset` could mint wrong amount of rsETH tokens due to current implementation of `getRSETHPrice()`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L34-L40 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L91-L110

Vulnerability details

Impact

If the price of the asset from the underlying aggregator goes above the maxAnswer or hits the minAnswer then Chainlink is going to return these border prices for as long as the prices are outside the bound, which would lead to massive under/over valuing or assets, and causing Kelp to minting rsETH in the wrong ratio to assets's prices.

NB: This is not in line or same with the Using deprecated function finding of the bot, cause even if this is fixed to use the latestRoundData() issue still exists

Proof of Concept

Take a look at ChainlinkPriceOracle.sol#L34-L40

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

Using cbETH as a case study, i.e the priceFeed Address: 0x536218f9E9Eb48863970252233c8F271f554C2d0 , we can see that a call to latestAnswer calls the underlying aggregator

Now in regards to Kelp, this function is what's used to get the price of an asset, and multiple calls are routed to this, down to even knowing the amount of rETH to mint for a user that deposits an asset, i.e the flow for depositing asset would be depositAsset() -> _mintRsETH() -> getRsETHAmountToMint() -> getRSETHPrice() -> getAssetPrice()

So If the wrong price for the asset is returned, then the totalETHInPool would be either way overvalued or undervalued in getRSETHPrice() totalETHInPool += totalAssetAmt * assetER;

Inherently causing the RSETHPrice to also be way overvalued or undervalued rsETHPrice = totalETHInPool / rsEthSupply, which goes way back to affect the amount of tokens that would be minted for the users.

NB: The key cause of this issue is getAssetPrice() which in the instance of depositAsset() is called twice, i.e while calculating totalETHInPool in getRSETHPrice() and also rsETHAmountToMint in getRsETHAmountToMint(), which just exarcebates the issue.

Tool used

Manual Review

Recommended Mitigation Steps

A popular fix for this issue, would be to implement the max/min circuit breakers for feeds while querying prices and then revert if the answer is outside of the specified bounds.

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 duplicate of #468

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[Bauchibred]

Hi, @fatherGoose1, thanks for judging, but I have to disagree that, this is not valid, this bug case was exactly what happened during the whole Do Kwon UST/LUNA saga from a while back, leading to more than $20M loss of TVL, from some of the documented cases alone.

A very simple POC is

• Assume minPrice is $0.1... realistic cause this has been set as the minPrice for some assets in the past.
• The real asset's price plummets and is now valued at $0.01.
• Chainlink still returns the price as $0.1 leading to a massive overvaluation of the asset.
  • If user is trying to withdraw asset by returning RsETH they lose enormous amount of funds.
  • If attacker sees the opportunity just like with VENUS during the LUNA saga, they can mint outrageous amount of RsETH for 10% of the real value.

[fatherGoose1]

@Bauchibred , the official stETH/ETH price feed from the chainlink website is 0x86392dC19c0b719886221c78AB11eb8Cf5c52812 and does not contain min/max answer variables.