Kelp DAO | rsETH - Udsen's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L141 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L152 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L109 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L63-L78 https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L37-L39

Vulnerability details

Impact

The LRTDepositPool.getRsETHAmountToMint function is used to calculate the amount of rsEth tokens to mint when a particular asset is deposited via LRTDepositPool.depositAsset function. The lrtOracle.getRSETHPrice() function is called for the rsEth amount calculation as shown below:

    rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(); //@audit - check these external functions being called

The lrtOracle.getRSETHPrice() function in its execution flow gets the price for each of the assets in the supportedAssets array by calling the LRTOracle.getAssetPrice function as shown below:

    address[] memory supportedAssets = lrtConfig.getSupportedAssetList(); //@audit-info - get the supported asset list from the config contract
    uint256 supportedAssetCount = supportedAssets.length; //@audit-info - get the supported assets list length

    for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
        address asset = supportedAssets[asset_idx];
        uint256 assetER = getAssetPrice(asset); //@audit-info - get the asset price for each of the assets in the supported asset list

        uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset); //@audit-info - get the total asset amount from all three contracts
        totalETHInPool += totalAssetAmt * assetER; //@audit-info - increment the total asset amount in the contract via the ETH amount (because it is common denominator)

        unchecked {
            ++asset_idx;
        } //@audit-info - increment the index by one
    }

    return totalETHInPool / rsEthSupply;

In each of the above iterations in the for loop in supportedAssets array, the getAssetPrice function calls the chainlink price feed of the given asset to receive the current price of that asset as shown below:

function getAssetPrice(address asset) public view onlySupportedAsset(asset) returns (uint256) {
    return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset); //@audit-info - get the asset price from the oracle
}

The ChainlinkPriceOracle.getAssetPrice implemenation is as follows:

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

The issue here is that Chainlink's multisigs can immediately block access to price feeds at will. If this happens to any of the price feeds of the supported assets in the protocol the ChainlinkPriceOracle.getAssetPrice function will revert while calling latestAnswer for that blocked price feed. If a new pricefeed is not registered for that asset by chainlink immediately the calls to the blocked price feed will always revert.

The only other option left is to remove this particular asset (of which the pricefeed was blocked by chainlink) from the LRTConfig.supportedAssetList array. This way this particular asset will not be considered in the lrtOracle.getRSETHPrice() function for rsEth price calculation. But there is no functionality to remove an already added supported asset from the supportedAssetList array in the LRTConfig contract.

As a result this will revert the entire LRTDepositPool.depositAsset function (since the depositAsset function calls the lrtOracle.getRSETHPrice() in its execution flow) thus DoS the asset deposit functionality of the LRTDepositPool contract.

Proof of Concept

        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L141

        (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L152

        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L109

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        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;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L63-L78

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

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to implement following remedies to solve the above vulnerability:

1. The call to the latestAnswer (even this function should be replaced by the latestRoundData function) in the ChainlinkPriceOracle.getAssetPrice function, should be surrounded by the solidity's try/catch block. In a scenario where the call to latestAnswer reverts, the caller contract is still in control and can use the catch block to call a fallback oracle or handle the error in any other suitable way which is safe and explicit.

2. Implement a functionality in the LRTConfig contract which would allow to remove an already supportedAsset of the protocol if the need arises. This way if a particular price feed of a supported asset is blocked by chainlink and any new chainlink price feed is not added immediately to that asset, this particular supported asset can be removed from the LRTConfig.supportedAssetList array thus preventing the LRTDepositPool.depositAsset functionality being DoS.

Assessed type

DoS