Kelp DAO | rsETH - tallo's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L52 https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L121 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L47 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L71 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L109

Vulnerability details

Impact

A strategy change will result in an incorrect price evaluation for RSETH due to it undervaluing the amount of underlying token deposited.

Vulnerability Explanation

1. When LRTOracle#getRSETHPrice is called it gets the price of RSETH based on the calculated total amount of ETH based on total balance of underlying assets (stETH, rETH, cbETH) multiplied by the exchange rate to ETH

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        //...

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        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;
    }

2. To get the total deposits, LRTDepositPool#getTotalAssetDeposits is called which sums up the amount of assets in the 3 possible locations returned by LRTDepositPool#getAssetDistributionData

    function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

3. LRTDepositPool#getAssetDistributionData sums up the total assets from 3 possible locations: i. In the LRTDepositPool ii. Inside the node delegator queue contracts iii. Inside the asset eigenlayer strategy

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
1@>        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));
        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
2@>            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
3@>            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

4. The amount of assets stored inside the eigenlayer strategy is calculated from NodeDelegator#getAssetBalance

    function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

5. If the CRTConfig admin were to call CRTConfig#updateAssetStrategy to change the strategy, it would result in the funds inside the previous strategy not being included in the accumulated funds; resulting in an incorrect price evaluation for rsETH.

Proof of Concept

1. Assume rsETH's initial price is 1 ETH/rsETH with 100 rETH of underyling token deposit in the strategy. The following calculation are done based on the formulas inside LRTOracle#getRSETHPrice

The before calculations for the price of rsETH are as follows:

totalAssetAmt = (0+0+100) rETH = 100 rETH 
totalETHInPool = totalAssetAmt * assetER = 100 rETH * 1 ETH/rETH = 100 ETH 
rsETHPrice = totalETHInPool / rsEthSupply = 100 ETH/100 rsETH= 1 ETH/rsETH

2. Strategy gets changed resulting in the following price calculation for rsETH:

totalAssetAmt = (0+0+0) rETH = 0 rETH 
totalETHInPool = totalAssetAmt * assetER = 0 rETH * 1 ETH/rETH = 0 ETH 
rsETHPrice = totalETHInPool / rsEthSupply = 0 ETH/100 rsETH = 0 ETH/rsETH

3. The price of rsETH is now 0 (or ideally just above 0 so it doesnt revert) after the strategy migration, allowing the user to effectively mint rsETH for free

Tools Used

VIM

Recommended Mitigation Steps

Add functionality inside NodeDelegator that properly migrates the funds from one eigenlayer strategy to another whenever LRTConfig#updateAssetStrategy is called. This function should migrate on all the relevant NodeDelegators that are affected by the changed strategy.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L80 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L52

Vulnerability details

Impact

LRTConfig only has the ability to add supported assets and not remove them. This means that the Kelp protocol will be forever vulnerable if there is an issue with any of the underlying assets.

Proof of Concept

There are several main issues that can be problematic:

1. Admin accidentally adds an unintended asset to the supported list
2. Existing underlying has a security incident with an attacker or malicious admin
3. Eigenlayer removes support for an asset

While the administrator can stop deposits by just setting the deposit limit of the compromised/unintended asset to 0, the assets price is still taken into account inside the LRTOracle#getRSETHPrice function to determine the price of RSETH.

     address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
     uint256 supportedAssetCount = supportedAssets.length;
     for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];

            //@audit here get the asset price
@>            uint256 assetER = getAssetPrice(asset);

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

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;

This means that the price of RSETH will forever be reliant on the compromised/unintended asset which can greatly affect the integrity of the protocol should something go wrong.

Tools Used

VIM

Recommended Mitigation Steps

Add the ability for the admin to remove support for an asset inside LRTConfig

Assessed type

Other