Kelp DAO | rsETH - 0xffchain's results

Lines of code

https://github.com/Layr-Labs/eigenlayer-contracts/blob/75e59432d079c6f90d48d4e950a68c15867c82b2/src/contracts/core/StrategyManager.sol#L237-L251 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L108-L122

Vulnerability details

Impact

A copy of each strategy for an asset is stored in the LRTConfig contract, This copy of the asset to strategy mapping allows for the updating of a strategy mapped to an asset in updateAssetStrategy. It is also of note that the contract does not allow an already supported asset to be unsupported. But a once supported strategy could be updated for another new strategy as specified in the code.

function  updateAssetStrategy(

address  asset,

address  strategy

)

external

onlyRole(DEFAULT_ADMIN_ROLE)

onlySupportedAsset(asset)

{

UtilLib.checkNonZeroAddress(strategy);

if (assetStrategy[asset] == strategy) {

revert  ValueAlreadyInUse();

}

assetStrategy[asset] = strategy;
}

The challenge with this is that when a deposit is made to a strategy, and the strategy is swapped for another new one, it means that the deposits made to the previous strategy is now un-redeemable as it is no more in the system anywhere again.

Proof of Concept

1. Strategy mapped to CbETH has about 200 CbETH stored in it on the Eigen Layer and 50 CbEth on the NDC.

2. There is a vulnerability on CbEth strategy specifically or on CbEth protocol and EigenLayer removes the Strategy from strategyIsWhitelistedForDeposit from its codebase

3. The admin/manager on Kelp has two options, either to pause the entire contract cause of one strategy vulnerability or remove that strategy and update to a new one which the EigenLayer will deploy.

4. If the admin goes for the second, it then means that deposits to the strategy is now trapped. This should not be the case as even EigenLayer only disables deposits and not deposits and withdrawal.

5. Admin might also choose to both pause the contract and update the strategy, as the strategy update like other setters on config isn't pause protected.

Tools Used

Manual

Recommended Mitigation Steps

Should not allow for updating of strategy explicitly rather create a mechanism to disable a strategy for deposit only.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

As https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/ mentions, "multisigs can immediately block access to price feeds at will" When this occurs, executing AggregatorInterface(assetPriceFeed[asset]).latestAnswer() will revert so calling the getAssetPrice functions also revert, which cause denial of service when calling functions like depositAsset for depositing funds into the protocol, cause now assets cannot be priced to allow minting of rETH tokens, This will DOS the protocol and limits its intended use.

Proof of Concept

Chainlink oracle data feed is used for getting the LSD token's price.

Bob calls the depositAsset function to deposit into the pool and Mint rETH.

Chainlink's multisigs suddenly blocks access to price feeds so executing AggregatorInterface(assetPriceFeed[asset]).latestAnswer(); will revert.

Bob Tries again to perform the intended action and it keeps reverting due to the previous action on chainlink multisigs.

Tools Used

Manual

Recommended Mitigation Steps

Oz recommends "https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/" "Therefore, to prevent denial of service scenarios, it is recommended to query ChainLink price feeds using a defensive approach with Solidity’s try/catch structure. In this way, if the call to the price feed fails, the caller contract is still in control and can handle any errors safely and explicitly"

The Oracle contract's getAssetPrice functions can be updated to refactor ``AggregatorInterface(assetPriceFeed[asset]).latestAnswer();intotry AggregatorInterface(assetPriceFeed[asset]).latestAnswer() returns (int256 answer) { ... } catch Error(string memory) { ... }.` The logic for getting the asset token's price from the Chainlink oracle data feed should be placed in the try block while some fallback logic when the access to the Chainlink oracle data feed is denied should be placed in the catch block. This Fallback logic could be an alternative oracle like uniswap v3, using the twap price.

Assessed type

DoS