code-423n4 / 2023-11-kelp-findings

13 stars 11 forks source link

Unhandled Chainlink revert can deny access to oracle prices #704

Closed c4-submissions closed 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L37-L39 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L109 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L68

Vulnerability details

The ChainlinkPriceOracle::getAssetPrice() lacks a preventive approach to handling the case of the Chainlink's latestAnswer() reverts, resulting in a denial of service to accessing oracle prices.

Proof of Concept

The ChainlinkPriceOracle::getAssetPrice() makes use of Chainlink's latestAnswer() to get the latest prices of LST assets (e.g., stETH, cbETH, rETH assets). The calls to the latestAnswer() can be reverted for several reasons, such as Chainlink's multisigs block access to price feeds, etc.

However, there is no preventive approach to handling the case of the latestAnswer() reverts, resulting in a denial of service to accessing oracle prices.

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

Impact

The price data reported by the getAssetPrice() are required by crucial functions: LRTDepositPool::getRsETHAmountToMint() and LRTOracle::getRSETHPrice().

The reverts of Chainlink's latestAnswer() can block access to LST assets' price, affecting the minting mechanism of the rsETH tokens and other functions.

Tools Used

Manual Review

Recommended Mitigation Steps

Wrap the call to the latestAnswer() in a try/catch block and handle any errors appropriately—for instance, fallback to Uniswap's TWAP oracle or other off-chain oracle services such as Tellor. The fallback solution will also ensure the price data availability to the protocol.

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

        //@audit -- an example of try/catch block for the Chainlink's latestAnswer()
+       try AggregatorInterface(assetPriceFeed[asset]).latestAnswer() returns (
+           uint256 price
+       ) {
+           // Handle the successful execution here
+           ...
+
+           return price;
+
+       } catch Error(string memory) {
+           // Handle errors here: (e.g., fallback to Uniswap's TWAP oracle or other off-chain oracle services such as Tellor)
+           ...
+       }
    }

Assessed type

Oracle

c4-pre-sort commented 11 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #32

c4-pre-sort commented 11 months ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #878

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #723

c4-judge commented 10 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)