c4-submissions commented 11 months ago

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:

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

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.