Use of deprecated Chainlink function latestAnswer()

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L38

Vulnerability details

Explanation

According to Chainlink's documentation, the latestAnswer function is deprecated. This function won't revert if no answer has been reached. Instead, it will return 0. Chainlink oracles are used for fetching the price of all the assets that can be deposited into the protocol. As for now, the assets are cbETH, rETH and stETH.

        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();

This prices will be used for calculating the price of rsETH, which is used for calculating the amount of rsETH the user will receive for the deposit he made.

            uint256 assetER = getAssetPrice(asset);

Impact

The protocol can DDoS if no prices are fetched correctly or users could mint rsETH for cheaper if some prices are correctly fetched and some others aren't.

Proof of Concept

In the case where all the assets' oracles return a 0, users won't be able to deposit funds since totalETHInPool would be 0, therefore lrtOracle.getRSETHPrice() returning 0 and making the calculation of rsethAmountToMint revert since it would try do divide by 0.

In case where 2/3 oracles return a 0, users would be able to mint rsETH for cheaper, since the calculations of the totalETHInPool would be wrong, resulting in lrtOracle.getRSETHPrice() returning a lower value and leading to a higher rsethAmountToMint.

        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

            uint256 assetER = getAssetPrice(asset);


            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;


            unchecked {
                ++asset_idx;
            }
        }


        return totalETHInPool / rsEthSupply;

Tools Used

Manual review.

Recommended Mitigation Steps

Consider using latestRoundData() instead of latestAnswer() and also validating the output of it to match the following code snippet:

(
        uint80 roundID,
        int256 price,
        ,
        uint256 updateTime,
        uint80 answeredInRound
      ) = AggregatorInterface(assetPriceFeed[asset]).latestRoundData();
      require(
          answeredInRound >= roundID,
          "Chainlink Price Stale"
      );
      require(price > 0, "Chainlink Malfunction");
      require(updateTime != 0, "Incomplete round");

I would also suggest that even if all the current supported tokens have 18 decimals in the price feed, the LST added in the future may not have 18 decimals in the price feed, so it would also be great to check the decimals.

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 insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #34

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #215

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory:
Invalid

[xAriextz13]

This issue also mentions the decimals issue with the Chainlink oracle as the issue #479

[c4-judge]

fatherGoose1 marked the issue as not a duplicate

[c4-judge]

fatherGoose1 marked the issue as duplicate of #479

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)