Price feed oracle lacks proper validation which may lead to incorrect calculations during rsETH minting

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Lacking proper price feeds oracle data can lead to the usage of stale prices for any of the supported assets - cbETH, rETH, etc in the system will result in incorrect calculations during the minting of rsETH in the protocol. This is an undesired behavior and may lead to users minting an incorrect amount of rsETH.

POC

When a user deposits a supported asset into the protocol, the equivalent amount of rsETH that the user receives is dependent on the chainlink price feed for that particular asset as seen here:

// calculate rseth amount to mint based on asset amount and asset exchange rate
rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

The current implementation lacks proper oracle price feed validation which would lead to multiple problems such as:

• the code doesn't revert when price of supported asset == 0
• price staleness is not checked.

Oracle price feeds can become stale for a number of reasons.

This could lead to stale prices according to the Chainlink documentation:

• https://docs.chain.link/docs/historical-price-data/#historical-rounds
• https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

And there is no check if the return value is stale data.

Recommendation

Consider refactoring the ChainlinkPriceOracle::getAssetPrice() function to include proper price feeds oracle validation for cases where the a zero value or staled price is returned, as demonstrated in the provided code snippet.

For example:

    /////////////////////////////////////////////////////
    // ERRORS
    /////////////////////////////////////////////////////

    error ChainlinkPriceOracle__StalePrice();
    error ChainlinkPriceOracle__PriceCannotBeZero(); 

    // 1 hour buffer period used in this example. protocol team must decide proper buffer period suitable for Kelp DAO.
    uint256 private constant STALENESS_TOLERANCE = 25 hours; // 25 * 60 (minutes) * 60 (seconds) = 90000 seconds

  function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256 price) {
        (, int256 price, , uint256 updatedAt,) =
            AggregatorInterface(assetPriceFeed[asset]).lastestRoundData();
        uint256 secondsSince = block.timestamp - updatedAt;

        if (secondsSince > STALENESS_TOLERANCE) {
            revert ChainlinkPriceOracle__StalePrice();
        } 

        if (price <= 0) {
            revert ChainlinkPriceOracle__PriceCannotBeZero(); 
        }
        // cast to uint256 since price already checked to be > 0. 
        uint256(price);
    }

Alternatively, use a secondary oracle. While this is in the works for the protocol, the price feed oracle data validation is important for the current implementation which uses only Chainlink for price feeds.

Tool used

Manual Review

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 #194

[c4-pre-sort]

raymondfam marked the issue as duplicate of #723

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)