Kelp DAO | rsETH - Shaheen's results

Lines of code

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

Vulnerability details

In the NodeDelegator.sol contract, the function depositAssetIntoStrategy is utilized to allocate users' assets into the Eigenlayer Strategy. However, it lacks a mechanism to ensure that the expected shares are returned, which can result in loss of shares for the protocol and users.

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

        ///@audit-ok check the return value `shares` is greater than zero to make sure successful deposit - this is internally checked by eigenlayer
        ///@audit-issue M a slippage protection should be added because "share rate can be massively inflated" ~ EigenLayer
        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
    }

The EigenLayer's depositIntoStrategy() function is susceptible to griefing attacks, as highlighted in their C4 audit. While EigenLayer acknowledges this issue, their mitigation approach is to verify that the returned shares are not zero, considering it as a worst-case scenario when the share rate is "massively inflated"

->        // checks to ensure correctness / avoid edge case where share rate can be massively inflated as a 'griefing' sort of attack
          require(newShares != 0, "StrategyBase.deposit: newShares cannot be zero");

EigenLayer does not implement slippage protection within this function for not so "massive" inflation attacks, leaving it to the discretion of strategy designers. Strategy designers are advised to incorporate slippage protection into their implementations, as discussed in their audit findings:

This is good information for Strategy designers - EigenLayer Dev

Impact

• Loss of Shares

Proof of Concept

1. https://github.com/code-423n4/2023-04-eigenlayer/blob/398cc428541b91948f717482ec973583c9e76232/src/contracts/strategies/StrategyBase.sol#L103
2. https://github.com/code-423n4/2023-04-eigenlayer-findings/issues/343

Tools Used

Shaheeniyat 🦅

Recommended Mitigation Steps

To enhance security for the users'/protocol's funds, it is recommended to add a slippage protection mechanism using minShares:

    function depositAssetIntoStrategy(address asset, uint256 minShares) 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);

        uint256 shares = IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
        require(shares >= minShares, "Slippage Protected")
    }

Assessed type

Context

Lines of code

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

Vulnerability details

The protocol allows users to deposit LSTs (cbETH, stETH, rETH) and mint RSETH tokens. The RSETH amount to mint & RSETH's price highly dependent on total assets deposit into the protocol. The total deposited assets can be in the LRTDepositPool, NodeDelegator & EigenLayer's Strategy. The are summed to get the total assets deposited:

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

The protocol uses ERC20.balanceOf() to determine the deposited assets in the LRTDepositPool & NodeDelegator, which can be dangerous.

    function getAssetDistributionData(address asset) public view override onlySupportedAsset(asset) returns (.....) {
        ///@audit-issue use of balanceOf can be dangerous for the rsETH price.
->      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;
            }
        }
    }

The protocol anticipates a parallel increase in total deposited assets and the RSETH total supply. This assumption is rooted in the belief that assets will only enter the protocol through users invoking depositAsset() and minting a fair share of RSETH tokens. Consequently, the protocol utilizes contract token balances to compute the getRSETHPrice().

Contrary to this expectation, the total assets within the contracts can expand without a corresponding minting of RSETH, thereby affecting the RSETH price. An attacker could potentially inject assets directly into these contracts, disrupting the equilibrium between the actual deposited assets and the contract balance.

Impact

• Price manipulation of RSETH

Tools Used

Shaheeniyat 🦅

Recommended Mitigation Steps

Avoid using the contract balance balanceOf() method in price calculations. Instead, maintain a totalAssetsDeposited variable that is updated when assets enter or exit the contract. This approach ensures a more accurate representation of the deposited assets, mitigating the risk of price manipulation.

Assessed type

ERC20