Kelp DAO | rsETH - T1MOH's results

Lines of code

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

Vulnerability details

Impact

Price of rsETH is calculated as totalLockedETH / rsETHSupply. rsETH price is used to calculate rsETH amount to mint when user deposits. Formulas are following:

• rsethAmountToMint = amount * assetPrice / rsEthPrice
• rsEthPrice = totalEthLocked / rsETHSupply

Problem is that it transfers deposit amount before calculation of rsethAmountToMint. It increases totalEthLocked. As a result rsethAmountToMint is less than intended because rsEthPrice is higher.

For example:

1. Suppose totalEthLocked = 10e18, assetPrice = 1e18, rsETHSupply = 10e18
2. User deposits 30e18. He expects to receive 30e18 rsETH
3. However actual received amount will be 30e18 * 1e18 / ((30e18 * 1e18 + 10e18 * 1e18) / 10e18) = 7.5e18

Proof of Concept

Here you can see that it firstly transfers asset to address(this), then calculates amount to mint:

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        ...

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

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

There is long chain of calls:

_mintRsETH()
    getRsETHAmountToMint()
        LRTOracle().getRSETHPrice()
            getTotalAssetDeposits()
                getTotalAssetDeposits()

Finally getTotalAssetDeposits() uses current balanceOf(), which was increased before by transferrign deposit amount:

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

Tools Used

Manual Review

Recommended Mitigation Steps

Transfer tokens in the end:

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        ...

+       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

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

-       // interactions
-       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
-
        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L79-L88

Vulnerability details

Impact

Attacker can perform classic first depositor attack:

1. Deposit 1 wei of LST
2. Make big donation to LRTDepositPool or NodeDelegator
3. Price of rsETH is too high, it leads to high rounding of minted rsETH amount to new depositors

Proof of Concept

1. Amount to mint is calculated dividing by rsETH price

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

        // calculate rseth amount to mint based on asset amount and asset exchange rate
@>      rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

2. To calculate rsETH price, it divides total locked value by rsETHSupply

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

        ...

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

3. getTotalAssetDeposits() uses balanceOf to get locked value

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

So attacker can deposit 1 wei of LST, therefore supply of rsETH will be 1 wei. Then he donates 1_000e18 LST to Pool - it makes rsETH price to be 1_000e36. Hence every deposit of amount less than 1_000 ETH will round down to 0 minted rsETH

Tools Used

Manual Review

Recommended Mitigation Steps

Perform initial deposit in initialize(). Deposit around 0.5 ETH, it will be enough

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L123

Vulnerability details

Impact

How currently rsETH price is calculated? totalEthLocked is divided by rsETH supply. How totalEthLocked is calculated? It sums 3 amounts per every asset: 1) balance of LRTDepositPool.sol, 2) balance of all node delegators, 3) already deposited amount of asset to EigenLayer's Strategy. We are interested in the 3rd point.

EigenLayer has 2 types of functions to fetch the user's deposited amount: 1) view, which doesn't update state, 2) function that potentially modifies state. Here's problem, now only view function is used. Future version of Strategy contract can contain specific logic which affects user's balance calculation, like interest accruing for example.

As a result rsETH price can be incorrect in protocol. It affects deposits and most likely withdrawals.

Proof of Concept

That's callback order:

LRTOracle.getRSETHPrice()
    LRTDepositPool.getTotalAssetDeposits()
        LRTDepositPool.getAssetDistributionData()
            NodeDelegator.getAssetBalance()
                IStrategy.userUnderlyingView()

NodeDelegator in its turn calls view version of function:

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

That's EigenLayer documentation: https://github.com/Layr-Labs/eigenlayer-contracts/blob/master/src/contracts/strategies/StrategyBase.sol#L246-L259 https://github.com/Layr-Labs/eigenlayer-contracts/blob/master/src/contracts/strategies/StrategyBase.sol#L193-L217

    function underlyingToShares(uint256 amountUnderlying) external view virtual returns (uint256) {
        return underlyingToSharesView(amountUnderlying);
    }

    function userUnderlyingView(address user) external view virtual returns (uint256) {
        return sharesToUnderlyingView(shares(user));
    }

    /**
     * @notice Used to convert a number of shares to the equivalent amount of underlying tokens for this strategy.
     * @notice In contrast to `sharesToUnderlying`, this function guarantees no state modifications
     * @param amountShares is the amount of shares to calculate its conversion into the underlying token
     * @return The amount of underlying tokens corresponding to the input `amountShares`
@>   * @dev Implementation for these functions in particular may vary significantly for different strategies
     */
    function sharesToUnderlyingView(uint256 amountShares) public view virtual override returns (uint256) {
        ...
    }

    /**
     * @notice Used to convert a number of shares to the equivalent amount of underlying tokens for this strategy.
     * @notice In contrast to `sharesToUnderlyingView`, this function **may** make state modifications
     * @param amountShares is the amount of shares to calculate its conversion into the underlying token
     * @return The amount of underlying tokens corresponding to the input `amountShares`
@>   * @dev Implementation for these functions in particular may vary significantly for different strategies
     */
    function sharesToUnderlying(uint256 amountShares) public view virtual override returns (uint256) {
        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Create 2 versions of function getRSETHPrice(): view and state modifying. In state modifying version use Strategy.userUnderlying() instead of Strategy.userUnderlyingView(). And utilize state modifying version in deposit.

Assessed type

Other

1. Low. getAssetCurrentLimit() will revert if admin sets new limit to less than deposited now

Vulnerability Details

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L56-L58

Limit from config can be lower than actually deposited, therefore function will revert instead of returning 0

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

Recommended mitigation steps

Return 0 instead of revert

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

2. Low. Protocol assumes that all LSTs have 18 decimals

Vulnerability Details

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTOracle.sol#L52-L79

Protocol assumes that all LSTs have 18 decimals, it can be different in the future. Here function expects that totalAssetAmt has 1e18 precission. Otherwise it will over/under-estimate asset:

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

        if (rsEthSupply == 0) {
            return 1 ether;
        }

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

Recommended mitigation steps

Normalize asset amount to 18 decimals:

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

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
+           uint256 decimals = IERC20(asset).decimals();
+           totalAssetAmt = decimals < 18 ? totalAssetAmt * (10 ** (18 - decimals)) : totalAssetAmt / (10 ** (decimals - 18));
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

3. Low. It is dangerous to give max approve to external contracts

Vulnerability Details

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/NodeDelegator.sol#L38

    function maxApproveToEigenStrategyManager(address asset)
        external
        override
        onlySupportedAsset(asset)
        onlyLRTManager
    {
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);
@>      IERC20(asset).approve(eigenlayerStrategyManagerAddress, type(uint256).max);
    }

Recommended mitigation steps

Consider giving approval before deposit:

    function depositAssetIntoStrategy(address asset)
        external
        override
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
        onlyLRTManager
    {
        address strategy = lrtConfig.assetStrategy(asset);
        IERC20 token = IERC20(asset);
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

        uint256 balance = token.balanceOf(address(this));
+
+       token.approve(eigenlayerStrategyManagerAddress, balance);

        emit AssetDepositIntoStrategy(asset, strategy, balance);

        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
    }