c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Bug Description

The chainlink data feed function latestAnswer() returns a int valuetype. This is because price can sometimes be negative hence the need to have it as as signed value. see here. But the kelp dao integration of chainlink expects chainlink's latestAnswer() to always return a uint type value.

This wrong intergration will cause:

getAssetPrice() in LRTOracle.col and ChainlinkPriceOracle.sol to return false underflowed values if chainlink reports negative price values
cause getRSETHPrice() to report wrong underflowed values that are close to the type(uint256).max value or even cause reverts in the calculation if totalAssetAmt * assetER > type(uint256).max.
cause wrong amounts of RsETH to be minted to users when they deposit via depositAsset()
cause getRsETHAmountToMint() function to return very large uint value or cause revert if amount * lrtOracle.getAssetPrice(asset)) > type(uint256).max.

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

    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer(); //999 423 390 497 362 700 
    }

If negative int value is returned by chainlink's latestAnswer(), getAssetPrice() will return a false/very large uint value due to underflow.

the function getAssetPrice() in LRTOracle.sol is used to get asset prices and is also used by getRSETHPrice() to calculate the price of RsETH. In the event that chainlink price is -0.998e18 for example , getAssetPrice() in LRTOracle.sol will return the price as ~type(uint).max - 0.998e18 due to underflow. This will cause calculations to result to exteremly large values or even unexpected reverts in getRSETHPrice() if totalAssetAmt * assetER > type(uint256).max. Snippet of affected getRSETHPrice() calculation below.

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L66C1-L71C55

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

Impact

Due to the incorrect integration of chainlink, in some market conditions where chainlink data feed contracts return a negative integer value, the value returned by getAssetPrice() in LRTOracle.sol will be close to type(uint).max due to uint underflow. This value will be inaccurate.

This wrong intergration will cause:

getAssetPrice() in LRTOracle.col and ChainlinkPriceOracle.sol to return false underflowed values if chainlink reports negative price values
cause getRSETHPrice() to report wrong underflowed values that are close to the type(uint256).max value or even cause reverts in the calculation if totalAssetAmt * assetER > type(uint256).max.
cause wrong amounts of RsETH to be minted to users when they deposit via depositAsset()
cause getRsETHAmountToMint() function to return very large uint value or cause revert if amount * lrtOracle.getAssetPrice(asset)) > type(uint256).max.

Tools Used

manual review

Recommended Mitigation Steps

1.) in the interface and functions directly calling the chainlink latestAnswer(), change the returned type to int

interface AggregatorInterface {
    function latestAnswer() external view returns (int256);
}

interface IPriceFetcher {
    // events
    event AssetPriceFeedUpdate(address indexed asset, address indexed priceFeed);

    function getAssetPrice(address asset) external view returns (int256);
    function assetPriceFeed(address asset) external view returns (address);
}

    function getAssetPrice(address asset) public view onlySupportedAsset(asset) returns (int256) {
        return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset);
    }

2.) in the function getRSETHPrice(), modify logic to make assetprice assetER to be zero 0 if the price gotten from chainlink is negative.