Lacking validations leads to consuming stale or incorrect price data

[c4-submissions]

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() reports price data to its callers without validating the staleness and incorrectness of the data.

Proof of Concept

This report raises an issue regarding the lack of stale/incorrect price data validation only, as the use of deprecated latestAnswer() was marked as a known issue.

The ChainlinkPriceOracle::getAssetPrice() reports price data fed from Chainlink's Price Feed Aggregators to its callers without validating the staleness and incorrectness of the data.

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

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

Impact

This report raises an issue regarding the lack of stale/incorrect price data validation only, as the use of deprecated latestAnswer() was marked as a known issue.

The price data reported by the getAssetPrice() will be consumed by crucial functions: LRTDepositPool::getRsETHAmountToMint() and LRTOracle::getRSETHPrice().

Consuming stale or incorrect prices can cause the minting of rsETH tokens to be incorrect.

Tools Used

Manual Review

Recommended Mitigation Steps

Validate the staleness and incorrectness of the fed price data as shown below.

1. Change Chainlink's latestAnswer() to latestRoundData() to access other returned parameters for validating the price data.
2. Implement logic for detecting stale or incorrect data.

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

+       (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, ) =
+           AggregatorInterface(assetPriceFeed[asset]).latestRoundData();

+       require(updatedAt != 0,"Round not complete");
+       require(answer > 0,"Chainlink's answer <= 0");

+       uint256 secondsSince = block.timestamp - updatedAt;
+       if (secondsSince > TIMEOUT) revert ChainlinkStalePrice(); //@audit -- set TIMEOUT to a proper value

+       return uint256(answer);
    }

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 not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #843

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[serial-coder]

Hi @fatherGoose1, @raymondfam, and @manoj9april.

I don't know why the return of stale/incorrect prices was invalidated, but I would like to provide additional info.

Here are excerpted parts from my report above:

The ChainlinkPriceOracle::getAssetPrice() reports price data to its callers without validating the staleness and incorrectness of the data.

The price data reported by the getAssetPrice() will be consumed by crucial functions: LRTDepositPool::getRsETHAmountToMint() and LRTOracle::getRSETHPrice().

Consuming stale or incorrect prices can cause the minting of rsETH tokens to be incorrect.

The situations that can cause the price data to be stale or incorrect:

• A stale price will be reported because the Chainlink update fails (price data cannot be updated within its heartbeat).
• The chainlink update round has not been completed.
• The chainlink update round has been completed but has gotten some errors (answer <= 0 can be reported).

[fatherGoose1]

@serial-coder, this report provides the base level of validations for checking for oracle freshness. The bot provides a link to Chainlink's documentation which highlights the latestRoundData() function.

There are several other submissions that highlight actual attack paths due to stale data or price discrepancies between the input assets. This current report does not match those in terms of actionable insights, and therefore will not be upgraded.