Kelp DAO | rsETH - hals's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L151-L157 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L109

Vulnerability details

Impact

• In LRTDepositPool.depositAsset function : when a user deposits a supported LST asset in the deposit pool, he will be minted share rsETH tokens proportional to the deposited amount and the asset price.

• And as per the the price calculations, the depositor will almost get an amount of share tokens (rsETH) equivalent to the amount of the deposited asset: LRTDepositPool.getRsETHAmountToMint function/L109

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

  where lrtOracle.getRSETHPrice() is totalETHInPool / rsEthSupply.

• But this function is missing the slippage parameter (minimum amount of share tokens to be minted) that ensures the user gets a fair amount of shares (as the rsETH price calculation might be affected if the deposit pool receives a direct LST transfers without going through the depositing process-minting of rsETH).

Proof of Concept

LRTDepositPool._mintRsETH function

  function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
        (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

LRTDepositPool.getRsETHAmountToMint function/L109

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

Tools Used

Manual Review.

Recommended Mitigation Steps

In depositAsset function: add a _minShareAmount parameter as a slippage protection:

    function depositAsset(
        address asset,
        uint256 depositAmount,
+       uint256 _minShareAmount
    )
        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);
+       require(rsethAmountMinted > _minShareAmount,"calculated less than required by the user");

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Context

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

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

Findings Summary

Low

[L-01] NodeDelegator contract doesn't have a mechanism to remove LST asset approval from eigenStrategyManager contract <a id="l-01" ></a>

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);, but before that, the nodeDelegator should approve the eigenStrategyManager contract on the deposited amount.

• And for this approval action; the nodeDelegator contract manager calls NodeDelegator.maxApproveToEigenStrategyManager function, which will approve the strategy manager on the maximum LST amount.

• But if the eigenStrategyManager contract got compromised/hacked or started to act maliciously, the LST asset balance of the nodeDelegator contract will be at risk since there's no way to remove LST assets approvals for the comromised strategy manager contract (setting the LST approval/allowance to zero) in cases of emergencies.

Proof of Concept

NodeDelegator.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);
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Remove NodeDelegator.maxApproveToEigenStrategyManager function, and update NodeDelegator.depositAssetIntoStrategy function to approve the strategy manager contract on the deposited amount only before depositing in the strategy:

   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);
+       IERC20(asset).approve(eigenlayerStrategyManagerAddress, balance);
        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
    }

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

[L-02] LRTConfig.setRSETH function: updating the rsETH contract address will result in loss of users deposits <a id="l-02" ></a>

Impact

• The admin of the LRTConfig can change the address of the rsETH contract, and this will result in re-setting the rsETH.totalSupply to zero.

• So if the rsETH contract is updated; users can't redeem/withdraw their deposits as the totalSupply would be zero (would revert due to underflow if any value is deducted from zero), also the price of the rsETH share token will drop as it's calculated based on the totalSupply and the total value of ETH held by the protocol (deposited in the pool + in the node delegators + in the EigenLayer strategies):

  return totalETHInPool / rsEthSupply;

Proof of Concept

LRTConfig.setRSETH function

    function setRSETH(address rsETH_) external onlyRole(DEFAULT_ADMIN_ROLE) {
        UtilLib.checkNonZeroAddress(rsETH_);
        rsETH = rsETH_;
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Either prevent updating the stETH contract address or implement a mechanism to transfer users balances and totalSupply with the new contract.

--------------------------------

[L-03] LRTConfig contract: no mechanism implemented to remove supported LST assets <a id="l-03" ></a>

Impact

• The LRTConfig contract has a function to add LST supported assets to the protocol, but it doesn't have any function or mechanism to unsupport any of these assets.

• So if the protocol decided to remove any LST asset from being traded in the system due to financial risks, it will not be able to do so and users will keep depositing these assets and receive share tokens in return.

Proof of Concept

LRTConfig._addNewSupportedAsset function

 function _addNewSupportedAsset(address asset, uint256 depositLimit) private {
        UtilLib.checkNonZeroAddress(asset);
        if (isSupportedAsset[asset]) {
            revert AssetAlreadySupported();
        }
        isSupportedAsset[asset] = true;
        supportedAssetList.push(asset);
        depositLimitByAsset[asset] = depositLimit;
        emit AddedNewSupportedAsset(asset, depositLimit);
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Add a mechanism to remove/unsupport any of the added LST assets.

[L-04] LRTDepositPool.addNodeDelegatorContractToQueue function is accessed by the LRTAdmin while it should be accessed by the LRTManager <a id="l-04" ></a>

Impact

• addNodeDelegatorContractToQueue function is used to add nodeDelegator contracts to the system to act as a LST hoders until depositing these assets in EigenLayer protocol, as per the comments on the addNodeDelegatorContractToQueue function, it should be called only by the LRTManager:

  /// @dev only callable by LRT manager
  

• But the function has onlyLRTAdmin access modifier that only allows the admin from accessing it,; not the manager.

Proof of Concept

LRTDepositPool.addNodeDelegatorContractToQueue function/L159-L162

    /// @notice add new node delegator contract addresses
    /// @dev only callable by LRT manager
    /// @param nodeDelegatorContracts Array of NodeDelegator contract addresses
    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {

Tools Used

Manual Review.

Recommended Mitigation Steps

-   function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {

+   function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTManager {

[L-05] LRTDepositPool.transferAssetToNodeDelegator function doesn't check if the nodeDelegator exists before sending it LST assets <a id="l-05" ></a>

Impact

• The main purpose of 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.

• Whenever the deposit limit of a specific LST asset is reached, a new nodeDelegator contract is deployed and added to the protocol to overcome the deposit limit implemented by EigenLayer protocol.

• transferAssetToNodeDelegator function is intended to be called by the deposit pool manager to transfer LST assets to an approved nodeDelegator contract; and then these assets to be moved to EigenLayer strategy when the strategy is not paused.

• but this function doesn't check if the ndcIndex (the index of the nodeDelegator in the nodeDelegatorQueue array) is within the nodeDelegatorQueue array or not; which might lead to LST assets being transferred to address(0) if the nodeDelegator doesn't exist (ndcIndex is out of bounds).

• This will lead to loss of protocol funds and affecting the price of the rsETH share tokens as the total ETH locked in the protocol will be lowered without burning an equivalenT amount of shares (the price of rsETH will go up!).

Proof of Concept

LRTDepositPool.transferAssetToNodeDelegator function

    function transferAssetToNodeDelegator(
        uint256 ndcIndex,
        address asset,
        uint256 amount
    )
        external
        nonReentrant
        onlyLRTManager
        onlySupportedAsset(asset)
    {
        address nodeDelegator = nodeDelegatorQueue[ndcIndex];
        if (!IERC20(asset).transfer(nodeDelegator, amount)) {
            revert TokenTransferFailed();
        }
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update transferAssetToNodeDelegator function to check if the nodeDelegator address exists bfore transferring LST assets to it:

    function transferAssetToNodeDelegator(
        uint256 ndcIndex,
        address asset,
        uint256 amount
    )
        external
        nonReentrant
        onlyLRTManager
        onlySupportedAsset(asset)
    {
        address nodeDelegator = nodeDelegatorQueue[ndcIndex];
+       require(nodeDelegator != address(0),"nodeDelegator doesn't exist");
        if (!IERC20(asset).transfer(nodeDelegator, amount)) {
            revert TokenTransferFailed();
        }
    }