Kelp DAO | rsETH - adeolu's results

Lines of code

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

Vulnerability details

Vulnurebility Detail

Bug Description

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

in checking if current limit for an asset has been reached via getAssetCurrentLimit(), the function getTotalAssetDeposits() is called and it calls getAssetDistributionData() which sums up all the Node delegators asset balances in EigenLayer. It gets the EigenLayer balance by called the EigenLayer Strategy contract's userUnderlyingView() function.

LRTDepositPool.getTotalAssetDeposits() -> LRTDepositPool.getAssetDistributionData() -> NodeDelegator.getAssetBalance() -> EigenLayerStrategy.userUnderlyingView()

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

                 return IStrategy(strategy).userUnderlyingView(address(this));

The problem here is that the eigen layer function userUnderlyingView() returns the amount of assets accrued to the user based on amount of shares the user has. The eigen layer contract is a yield bearing contract very similar to erc-4626 vaults, in the event that there is yield, the assets returned will not be the same as the one initially deposited by users via kelp Dao. So the getAssetCurrentLimit() function is actually checking if deposit limit is reached by subtracting the set assets deposited + yield accrued from the set max Deposit limit for the asset

return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);

The above can be further explained to be lrtConfig.depositLimitByAsset(asset) - assets + yield.

Yield amount should not be in this permutation. So if there is a scenario where by the depositLimit per asset is 100, 80 assets was deposited by users and 80 assets goes to eigen layer, eigen layer assigns 80 shares to node delegator. Later the 80 shares in eigen layer increases in value and becomes worth 110 assets. The function userUnderlyingView() will return 110 as assets for the node delegator. getAssetCurrentLimit() will use it as actual amount of deposits and proceed to do the math. This will become 100 - 110 and cause a revert.

Getting the total amount of assets deposited by calling userUnderlyingView() is not the best approach as it returns the asset value of the shares the caller is holding, not the initial amount of assets the caller deposited. See the notice tag in the snippet below.

https://github.com/Layr-Labs/eigenlayer-contracts/blob/75e59432d079c6f90d48d4e950a68c15867c82b2/src/contracts/strategies/StrategyBase.sol#L245C1-L251C6

    /**
     * @notice convenience function for fetching the current underlying value of all of the `user`'s shares in
     * this strategy. In contrast to `userUnderlying`, this function guarantees no state modifications
     */
    function userUnderlyingView(address user) external view virtual returns (uint256) {
        return sharesToUnderlyingView(shares(user));
    }

110 assets were'nt deposited but 80, so it will be better to track the number of user depoits in the contract storage than utilize the function userUnderlyingView()

Impact

userUnderlyingView() returing the asset value of shares held may mean that assets can be worth more than intially deposited. Using this value to calculate if the deposit limit of an asset is reached in getAssetCurrentLimit() can give errored results since the assets + yield is not the same as the assets deposited. getAssetCurrentLimit() may revert or return 0, even before the users deposits have actually gotten to the set deposit limit.

Tools Used

manual review

Recommended Mitigation Steps

1. save the deposit amount in contract storage and increment it each time a user deposits with the amount deposited.

    mapping (address => uint256) private amountDepositedPerAsset;


    /// @notice helps user stake LST to the protocol
    /// @param asset LST asset address to stake
    /// @param depositAmount LST asset amount to stake
    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);
        amountDepositedPerAsset[asset] += depositAmount;

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

2. use the new mapping value amountDepositedPerAsset in getAssetCurrentLimit()

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

Assessed type

Error