Risk of Deprecated Chainlink Function: latestAnswer

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

The usage of the latestAnswer function from the Chainlink library has been deprecated according to Chainlink’s documentation. This function, if no response is available, does not throw an error but returns a value of 0.

The getAssetPrice() function calls the deprecated latestAnswer function, potentially resulting in incorrect price data for various feeds or leading to a division by zero and possibly a Denial of Service vulnerability.

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

Impact

The affected functions, are getRsETHAmountToMint() and getRSETHPrice().
In getRsETHAmountToMint(), if getAssetPrice() returns 0:

1. If getRSETHPrice() also returns 0, it leads to a Denial of Service due to division by zero.

2. If getRSETHPrice() does not return 0, rsethAmountToMint becomes zero, meaning the user receives no share despite depositing an asset.

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

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

Tools Used

Manual review

Recommendations

To mitigate this risk, it is advisable to transition from using latestAnswer to latestRoundData. Additionally, incorporate checks on the return data and add error handling if the price is outdated or the round is incomplete:

-    function latestAnswer() external view returns (uint256);
+    function latestRoundData() external view returns (uint80, int, uint, uint, uint80);

@@ -35,7 +36,12 @@ contract ChainlinkPriceOracle is IPriceFetcher, LRTConfigRoleChecker, Initializa
     /// @param asset the asset for which exchange rate is required
     /// @return assetPrice exchange rate of asset
     function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
-        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();

+        (uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = AggregatorInterface(assetPriceFeed[ass
et]).latestRoundData();
+        require(answeredInRound >= roundID, "ChainLink: Stale price");
+        require(timeStamp > 0, "ChainLink: Round not complete");
+        require(price > 0, "ChainLink: invalid_oracle_answer");
+        return uint256(price);
     }

This will ensure a more robust and updated handling of price data, avoiding stale or incomplete responses and potential division by zero vulnerabilities.

Assessed type

Oracle

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[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

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)