Kelp DAO | rsETH - xAriextz's results

Lines of code

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

Vulnerability details

Explanation

Users deposit assets by using depositAsset() function. This will transfer their assets to the contract before minting rsETH:

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


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

The problem arises since the amount to be minted uses the current rsETH price, which will be calculated by also taking into account the funds that were just deposited.

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

        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;

This will result in an unfair calculation of minted rsETH for the users as I will explain below.

Impact

Users will receive less rsETH than expected, specially when depositing large amounts.

Proof of Concept

1. Alice deposits 1 stETH using depositAsset(). As it's the first deposit, she will receive: 1e18 stETH * priceOfstETH / 1e18; In the case the price is currently 2000e18, she will receive 2000e18 rsETH tokens.
2. Now Bob deposits 1 stETH using depositAsset(). Let's assume stETH price hasn't changed. Now, as it is not the first deposit, more calculations will be needed. Bob will receive: 1e18 * 2000e18 / ( (2e18 * 2000e18) / 2000e18). Bob will receive only 1000e18 rsETH tokens, while Alice received double the amount for the same deposit. This happens because the total deposited was taken into account when the calculation was done, and Bob tokens were already deposited, making him receive less tokens.

This problem will be bigger when users deposit larger amounts. In case Bob deposited 100 stETH, the calculation would be as follows: 100e18 * 2000e18 / ( (101e18 * 2000e18) / 2000e18). This would make him receive less than 2000e18 rsETH. This means that Bob would receive less rsETH tokens than Alice, while depositing 100 times more funds into the pool.

Tools Used

Manual review

Recommended Mitigation Steps

When calculating the amounts to be minted don't take into account the funds the deposited has sent for the calculation of the total asset deposits.

Assessed type

Math

Node delegators cannot be removed

There is a function for adding Node Delegators to the nodeDelegatorQueue array: addNodeDelegatorContractToQueue().

    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {
        uint256 length = nodeDelegatorContracts.length;
        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
            revert MaximumNodeDelegatorCountReached();
        }


        for (uint256 i; i < length;) {
            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
            unchecked {
                ++i;
            }
        }
    }

The problem is that there is no possibility for removing them. I highly suggest adding this functionality for not having any further problems like a too large array that will end up making users and the protocol pay higher gas fees and maybe get transactions reverted.

Max approve to EigenLayer its dangerous

For approving EigenLayer's strategies to take tokens from the NodeDelegators, admins will call the maxApproveToEigenStrategyManager() function.

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

This will approve all the tokens that are and will be in the NodeDelegator contract to the EigenLayer's strategy. In my opinion this is a dangerous approach, since if EigenLayer gets hacked or compromised this will directly affect all the funds that the NodeDelegators hold also.

To mitigate this risk, I think that a safer approach could be approving only the amount that the contract is going to deposit into the strategy. This can be done by adding an approve to the depositAssetIntoStrategy() function.

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

        emit AssetDepositIntoStrategy(asset, strategy, balance);

      + // Add this line
      + IERC20(asset).approve(eigenlayerStrategyManagerAddress, balance);

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