`NodeDelegator.depositAssetIntoStrategy` function doesn't check the returned shares amount by the strategy (lacks slippage control)

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

• The main purpose of the node delegators is to act as temporary LST holders before depositing in EigenLayer protocol (3rd party); mainly to prevent disabling the kelp protocol if the EigenLayer paused the deposit operations.

• First the LST assets are transferred to a nodeDelegator contract by the deposit pool manager, then transferred to eigenStrategyManager contract by calling NodeDelegator.depositAssetIntoStrategy function that will in turn calls the IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);.

• The IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance) function deposits balance of token into the specified strategy, with the resultant shares credited to msg.sender which is the nodeDelegator contract that initiated the call.

• And this function will return the amount of new shares in the strategy created as part of the action.

• But it was noted that this returned share value is not checked against any minimum value determined by the node delegator nor against zero, which might result in protocol losing from its LST balance without getting enough/equivalent shares to withdraw from EigenLayer protocol.

Proof of Concept

NodeDelegator.depositAssetIntoStrategy function/L67

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

Tools Used

Manual Review.

Recommended Mitigation Steps

In depositAssetIntoStrategy function: add a _minShareAmount parameter as a slippage protection, and check this value against the returned shares by the strategy manager:

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

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

+       uint256 shares=IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);

+        require(shares >= _minShareAmount);
    }

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #230

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[DevHals]

Hi @fatherGoose1,

this issue was invalidated based on the fact that Eigen layer has an implementation to mitigate inflation attack: as per the sponsors reply on the primary issue:

Please check the eigen layer latest code here. They have implemented proper mitigation for inflated shares attack.

while checking the StrategyBase.deposit function, it was noticed that the check made for the minted shares (newShares) is to be != 0 :

require(newShares != 0, "StrategyBase.deposit: newShares cannot be zero");

but this doesn't fully protect the nodeDelegator contract against slippage, as the minted shares by the strategy could be very low, and NodeDelegator.depositAssetIntoStrategy function doesn't have any minimum value determined by the manager to check the minted shares against, nor does it have a deadline to prevent executing the transaction after specific time- that might affect the minted shares as it will fluctuates with time (since the protocol is going to be deployed on the mainnet which suffers from congestion and delays of transactions executions at some times).

I kindly ask if you could take another look at the specifics of this issue and provide a re-evaluation?

Thanks!

[c4-judge]

fatherGoose1 marked the issue as duplicate of #148

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[DevHals]

Hi @fatherGoose1 , this issue is not a duplicate of 148, as the vulnerability introduced here is a result of not validating the slippage introduced by integrating with a 3rd party, and it has nothing to do with the shares minted by the Kelp protocol!

also to add: issue 148 introduces a loss for the users only (the protocol will not be adversely affected) while this issue here introduces a loss of protocol funds (which will affect users in return if the protocol becomes insolvent),

Could you please have a second look?

Thanks!

[TradMod]

I agree with @DevHals that this issue is not a dup of 148, specifically because fixing this issue will not fix 148 & 148's fix will not fix this issue. The only dup of this issue is #230. Pls re-evaluate Sir. Thanks!

[fatherGoose1]

@DevHals @DevABDee I understand the difference in root causes of slippages between this report and #148. While the issues originate in different parts of the code and for different protocol functions entirely, I am leaving this report and #230 as duplicates of #148, simply because slippage will most likely not cause issue to any significant degree through this implementation. It is a necessary check to have to ensure the safety of Kelp's users and Kelp's held assets when referring to shares minted by Eigenlayer, but it is not likely that these stable assets will incur more than minimal slippages.

[DevHals]

@fatherGoose1,, the reason provided to duplicate this issue with 148 is not convincing (LSTs are not stable assets to assume that minimal slippage might be incurred), an example of Lido Staked ETH (stETH/ETH) price fluctuation all time as per https://coinmarketcap.com/currencies/steth/steth/eth/ (where it hit 0.9394 on 20th of June/2022)

but the final say is for the judge no matter what!

anyway, I have another submitted issue #409 that is marked as a duplicate of 148; so if you see that this issue is still a duplicate of 148; then this issue should be nullified,

Thanks for your time!

[TradMod]

Hey Sir @fatherGoose1, I wanted to kindly request your reconsideration of our escalation following the validation of issue #584. You initially marked this issue & #230, as a duplicate of #148 due to concerns about LSTs stability. However, with the acceptance of #584, it seems evident that LSTs may not be as stable as initially perceived, aligning with the observations highlighted by @DevHals in his comment above. Both of us maintain the perspective that this is a medium issue and not a duplicate of #148. We will appreciate your final re-evaluation of this issue. Thanks!