Kelp DAO | rsETH - GREY-HAWK-REACH's results

Lines of code

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

Vulnerability details

Impact

First time rsETH is minted, its price is set to the price of 1 ETH at the time of mint.

After that, every time rsETH is minted, to get the price of rsETH, LRTOracle#getRSETHPrice calculates the total value of LSTs in Kelp contracts:

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

and divides it by the supply of rsETH.

Because LRTDepositPool transfers the LST from the user first, and then calculates the price from it, the subsequent depositors will get significantly less rsETH per unit of LST than the first one.

Proof of Concept

1. Alice, the first depositor, deposits 1e18 stETH and gets 1e18 rsETH.
2. Bob deposits 1e18 stETH and receives:

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

amount = 1e18

getAssetPrice = 1e18

getRSETHPrice = totalETHInPool / rsEthSupply

    totalETHInPool = stETH deposits * stETH/ETH price = 2e18  * 1e18

    rsETHSupply = 1e18

    getRSETHPrice = 2e18

rsethAmountToMint = (1e18 * 1e18) / 2e18 = 0.5e18

As you can see, despite depositing the same amount of stETH and at the same stETH/ETH price, Bob receives only a half of rsETH that Alice received.

Foundry PoC

Tools Used

Foundry

Recommended Mitigation Steps

Compute the price first, and then transfer funds into DepositPool.

+       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
        if (
            !IERC20(asset).transferFrom(
                msg.sender,
                address(this),
                depositAmount
            )
        ) {
            revert TokenTransferFailed();
        }
-       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L95-L110

Vulnerability details

Impact

This issue persists regardless if the "Error in price calculation" finding is fixed or not. For this issue, we found it reasonable to assume that the mitigation is implemented.

Because rsETH's price depends on the balances of LSTs that DepositPool/NodeDelegators hold, rsETH's price calculation is vulnerable to the classic ERC4626 inflation attack.

After rsETH have just been deployed, whenever there's a pending deposit of X of LST tokens in the mempool,

Adversary may frontrun the victim: deposit 1 wei of LST, get 1 wei of rsETH, and then donate the same X of LST tokens into DepositPool.

As a result, the victim sends X of LST, gets 0 rsETH; Adversary's 1 wei of rsETH now represents two times more LSTs that the Adversary donated.

Proof of Concept

1. Kelp is deployed
2. Alice deposits 1 wei of stETH and mints 1 wei of rsETH
3. Bob tries to deposit 1e18 of stETH
4. Alice frontruns Bob and donates 1e18 of stETH into DepositPool.
5. Bob's transaction is minted.

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

    getRSETHPrice = totalETHInPool / rsEthSupply

    totalETHInPool = stETH deposits * stETH/ETH price = (1e18 + 1) * 1e18

    rsETHSupply = 1

rsethAmountToMint = (1e18 * 1e18) / ((1e18 + 1) * 1e18) = 0

As a result, Bob receives 0 shares. Alice owes the only 1 wei of rsETH in existence. If another user tries to mint rsETH, they will need to provide at least x >= 2e18 + 1 stETH. And they may receive become an another victim, if Alice donates x - 2e18 stETH into the pool.

Assuming the withdrawal amounts will be derived from rsETH/(stETH + cbETH + rETH) ratio, Alice's 1 wei of rsETH will be redeemable for all 2e18 + 1 stETH in the pool, while she spent only a half of that amount.

Foundry PoC

Tools Used

Foundry

Recommended Mitigation Steps

Implement Virtual offset described in https://docs.openzeppelin.com/contracts/5.x/erc4626#defending_with_a_virtual_offset

Assessed type

ERC4626

[L-01] The max amount for a single deposit into EigenLayer strategy can't be reached.

Lines of code

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

Impact

Before users can deposit funds into the LRTDepositPool.sol contract via the depositAsset() function, a check is performed to see if the current amount doesn't exceed the maxPerDeposit set by the EigenLyaer strategy. The EigenLayer strategy contract StrategyBaseTVLLimits.sol where funds will be deposited has two important checks

/// The maximum deposit (in underlyingToken) that this strategy will accept per deposit
    uint256 public maxPerDeposit;

and

/// The maximum deposits (in underlyingToken) that this strategy will hold
    uint256 public maxTotalDeposits;

The depositLimitByAsset set in the LRTConfig.sol contract is there to check if the maxPerDeposit set in the Eigenlayer protocol is not exceeded. Problem arises in how this value is checked. As can be seen in the POC getAssetBalance() returns deposited tokens + accrued rewards. Depending on how often deposits on EigenLayer strategies are not paused Kelp may not gather all of the allowed amount for a deposit. Which results in loosing possible rewards. As each deposit is performed there will be more accrued rewards resulting in a smaller pot gathered by depositors for a stake in EigenLayer strategies.

Proof of Concept

1. When kelp first start gathering deposit the hardcoded limit is 100_000e18, if this funds are collected and deposited now kelp have a deposit into EigenLayer which is generating yield 2. Then kelp will increase the depositLimitByAsset to 200_000e18 in order to gather new deposits from users, and deposit in a for example a month when eigen unpauses their deposit function. 3. During this time kelp has already deposited 100_000e18 tokens that will be generating yield 4. Instead of gathering 100_000e18 for a new deposit they will gather lets say 98_000e18 5. Which results in 2_000e18 lost deposits that can be generating yield for the next month Keep in mind that the probability of maxPerDeposit being set to 100_000e18 on mainet is low, as the first time EigenLayer opened up their deposits the maxTotalDeposit for strategies was 3600e18.

• In order to deposit funds the user calls depositAsset() function in the LRTDepositPool.sol contract

 function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

• depositAsset() in turn calls getAssetCurrentLimit() which first calls depositLimitByAsset() in order to take the governance limit set for the specified asset

function getAssetCurrentLimit(address asset) public view override returns (uint256) {
        return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
    }

• Then substracts the amount returned from getTotalAssetDeposits()

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

• getTotalAssetDeposits() in turn calls getAssetDistributionData()

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

• getAssetDistributionData() check the balance of LRTDepositPool.sol for the selected token and the balance of NodeDelegator.sol for the selected token then calls getAssetBalance() in the NodeDelegator.sol

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

• getAssetBalance() calls userUnderlyingView on the EigenLayer protocol,
• which in turn calls sharesToUnderlyingView which converts the number of shares to the equivalent amount of underlying tokens for the selected strategy meaning. Meaning that this will return already deposited tokens + accrued rewards.

[L-02] NatSpec mismatch

LRTDepositPool.sol#L159-L162

    /// @notice add new node delegator contract addresses
    /// @dev only callable by LRT manager
    /// @param nodeDelegatorContracts Array of NodeDelegator contract addresses
    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {

NatSpec: "only callable by LRT manager" modifier: onlyLRTAdmin (not onlyLRTManager, or vice versa for docs)

[L-03] Some funds may be deposited later than they could be

Because deposits are 3-steps (user -> DepositPool, DepositPool -> NodeDelegator, NodeDelegator -> EigenLayer), it is possible that some user deposits into Kelp may happen right before the Manager deposits funds from NodeDelegator into EigenLayer.

Consider using depositing from DepositPool into EigenLayer via transferFrom. This would only require setting infinite approval from DepositPool to NodeDelegator, and changing NodeDelegator#depositAssetIntoStrategy to transferFrom(depositPool, strategy, amount).