Kelp DAO | rsETH - circlelooper's results

Lines of code

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

Vulnerability details

Impact

User may deposit low value asset in order to get high value asset, high value asset depositor will suffer loss.

Proof of Concept

User deposits asset and receives RSETH, when withdraw (in the future when it's implemented), it's highly likely that user will not get the same asset back, for that the contract simply mints RSETH to user, but not save the asset user deposits.

The assets used to deposit have different prices, so users are tempted to deposit low value asset in order to withdraw high value asset. High value asset depositors will suffer loss if they are unable to withdraw the same asset as they deposit.

Tools Used

Manual Review

Recommended Mitigation Steps

Please consider to save user's depositing asset, so user can withdraw the same asset.

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

Incorrect amount of RSETH is minted to depositor.

Proof of Concept

To mint RSETH, user is required to transfer asset to LRTDepositPool 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);
    }

The amount of RSETH to be minted is calculated in function getRsETHAmountToMint:

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

The amount is thus determined by deposit asset amount, asset price and RSETH price. Let's examine the RSETH price in getRSETHPrice.

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

If there is no RSETH has been minted, then RSETH price is 1e18, otherwise the price is determined by the asset amount in the contract and the RSETH supply. Recall that when user deposits, the asset is transferred to the contract firstly, this is wrong and incorrect RSETH is minted to user. Consider the following 2 scenarios:

1. Alice deposits 2 ether asset when RSETH supply is 0, she will get 2 ether RSETH
2. Alice deposits 1 ether asset when RSETH supply is 0, she gets 1 ether RSETH, then she deposits again with 1 ether asset, she will only get get 0.5 ether RSETH because the 1 ether asset is transferred to the contract firstly, the asset amount in the contract is 2 ether.

Tools Used

Manual Review

Recommended Mitigation Steps

Please consider to mint first then transfer the asset to the contract.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Queued withdrawal asset in EigenLayer is ignored when calculate the asset balance.

Proof of Concept

Kelp's total asset balance is composed of 3 parts: asset lying in deposit pool, asset lying in NodeDelegators and asset staked in EigenLayer:

        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);

When calculate the asset staked in Eigenlayer, Kelp calls EigenLayer strategy contract's function userUnderlyingView:

    /// @dev Returns the balance of an asset that the node delegator has deposited into the strategy
    /// @param asset the asset to get the balance of
    /// @return stakedBalance the balance of the asset
    function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

This function converts user's share to the underlying asset, when user deposit or withdraw, user's share will be updated. However, 2 steps are required to withdraw from EigenLayer.

1. First, user should call function queueWithdrawal to queue a withdrawal of the given amount of shares from each of the respective given strategies, in this function, user's share will be deducted;
2. Secondly, user calls function completeQueuedWithdrawal or completeQueuedWithdrawals to complete the specified queuedWithdrawal, in these functions, user's asset will be returned.

It's possible that a user deposits after a withdrawal is queued but before the withdrawal is completed, if that is the case, the total asset balance is calculated without the queued asset, because the total asset balance will affect the RSETH price, which would in turn affect the minted amount when user deposits, so wrong amount of RSETH will be minted.

Tools Used

Manual Review

Recommended Mitigation Steps

Please consider to add queued withdrawal asset balance to total asset balance in the calculation.

Assessed type

Other