Kelp DAO | rsETH - Pechenite's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L109-L122 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L71-L89

Vulnerability details

Impact

Each asset in Kelp DAO has particular EigenLayer Strategy. When LRT Manager decide, he can transfer the deposited assets from NodeDelegator to the particular EigenLayer Strategy. The LRTDepositPool also enforce assets deposit limit tracked with depositLimitByAsset mapping. The current deposit for asset is sum of the LRTDepositPool, NodeDelegator and EigenLayer Strategy contract balances of this asset. Users cannot deposit more than this limit. The asset for particular EigenLayer Strategy can be changed over time via LRTConfig.sol#updateAssetStrategy() function.

The vulnerability is related to the way asset strategies are managed in the Kelp DAO protocol. When a EigenLayer Strategy for an asset is changed, there hasn't a mechanism in place to migrate user deposits from the old strategy to the new one. This can result in user funds becoming permanently locked in the old strategy, effectively rendering them inaccessible.

Additionally, the getAssetDistributionData() function, which is used to obtain information about the distribution of assets and used to enforce deposit limit during deposit, will not return the actual total current deposit of certain asset as it will have assets in the old strategies. The getAssetDistributionData() function will get into the calculation the latest Strategy that is set for the particular asset, even if asset. This will lead to inaccuracies in the asset distribution. This way users will also can deposit more asset than the set limit.

This issue in the Kelp DAO protocol has the following impact:

Risk of Permanent Loss of User Funds: After changing the strategy of a particular asset, user deposits made in the previous strategy can become permanently lost or locked within that strategy. This poses a significant risk to users, as they may be unable to recover their deposited assets for some period of time.

Exceeding the the assets deposit limit

Proof of Concept

The vulnerability can be observed in the following code snippets from the provided contracts:

1. Changing Strategy:

   • Contract: LRTConfig
   • Relevant Code: The contract allows changing the strategy associated with an asset through the updateAssetStrategy() function. However, there is no mechanism to ensure that user deposits in the old strategy are migrated to the new one, potentially leading to permanent loss of user funds.
   • GitHub Link: LRTConfig.sol: Lines 109-122

       function updateAssetStrategy(
           address asset,
           address strategy
       )
           external
           onlyRole(DEFAULT_ADMIN_ROLE)
           onlySupportedAsset(asset)
       {
           UtilLib.checkNonZeroAddress(strategy);
           if (assetStrategy[asset] == strategy) {
               revert ValueAlreadyInUse();
           }
           assetStrategy[asset] = strategy;
       }

2. Inaccurate Asset Distribution Information:

   • Contract: LRTDepositPool
   • Relevant Code: The getAssetDistributionData() function retrieves the latest strategy that holds the asset balance. If strategies have changed, this function may return inaccurate information about the distribution of assets. Actually I think that the function flow is correct, the whole issue arise after the of asset for particular EigenLayer Strategy.
   • GitHub Link: LRTDepositPool.sol: Lines 71-89

          function getAssetDistributionData(address asset)
           public
           view
           override
           onlySupportedAsset(asset)
           returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
       {
           // Question: is here the right place to have this? Could it be in LRTConfig?
           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;
               }
           }
       }

Tools Used

Manual Review

Recommended Mitigation Steps

There isn't an elegant way to fix this issue, but I would suggest the following mitigation steps:

First way: Implement a Migration Mechanism

• Description: Develop and implement a mechanism that allows users to migrate their deposits from the old strategy to the new one when strategy changes occur. This ensures that user funds are not permanently locked.

or

Second way: Update the updateAssetStrategy() function

• Description: Update the updateAssetStrategy() function to transfer user deposits from the old to the new Strategy after the new Strategy is set.

I recommend the second way.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L119-L144

Vulnerability details

Impact

This issue pertains to the depositAsset() function within the LRTDepositPool.sol contract, where certain critical checks are missing:

1. Minimum Minted Amount Check: Due to the highly volatile nature of the crypto and DeFi space, the function does not verify if the user is satisfied with the final minted rsETH amount.

2. Expiration Timestamp Check: The LRTDepositPool.sol#depositAsset() function lacks a check to ensure that the transaction has not been in the mempool for a very long time. This can result in the provided asset amount from the user becoming invalid at the time of execution. Without this check, users could receive outdated data (minted rsETH amount) without being satisfied with it.

In summary, the depositAsset() function lacks a minimum minted amount (a minimum amount specified by the user) and an expiration timestamp check, making it susceptible to potential issues. These issues include improper minting of rsETH tokens due to the high volatility of the crypto space.

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

Proof of Concept

Scenario 1:

1. A user deposits 100 tokens via the depositAmount() function for asset X. The price of ETH at the moment of the transaction is $2000, and let's assume the price of asset X is $10000.
2. Currently, the rsETH minted amount for the user is 500. | ((100 * 5) / 1) = 500
3. However, the user's transaction gets delayed, and due to the highly volatile nature of the DeFi and crypto space, the price of ETH can change significantly during this time. So, the price of ETH increases to $2500.
4. Now, the rsETH minted amount for the user is 400, but the user is not satisfied with this minted amount as it represents a 20% decrease from the expected value. | ((100 * 4) / 1) = 400

Scenario 2:

1. A user deposits 100 tokens via the depositAmount() function for asset X. The price of ETH at the moment of the transaction is $2000, and let's assume the price of asset X is $10000.
2. Currently, the rsETH minted amount for the user is 500. | ((100 * 5) / 1) = 500
3. However, the user's transaction gets delayed, and due to the highly volatile nature of the DeFi and crypto space, the price of asset X can change significantly during this time. So, the price of asset X decreases to $8000.
4. Now, the rsETH minted amount for the user is 400, but the user is not satisfied with this minted amount as it represents a 20% decrease from the expected value. | ((100 * 4) / 1) = 400

These two examples illustrate how changes in ETH price or the price of assets deposited by the user can affect the minted rsETH amount. This highlights how the lack of a minimum minted amount (a minimum amount specified by the user) and an expiration timestamp check in the code could lead to inefficient minting calculations for users.

Tool Used

The vulnerability was identified through manual code inspection.

Recommended Mitigation Steps

Add the following:

Add:

1. Minimum Minted Amount Check: Add parameter for minimum amount of rsETH that the user specify.
2. Expiration Timestamp Check of the depositAsset() function if the transaction is in mempool for long period of time.

Assessed type

Error

QA Report - Kelp DAO Audit Contest | 10 Nov 2023 - 15 Nov 2023

---

Executive summary

Overview

Findings Summary

---

[L‑1] Do not allow the modification of the asset for particular EigenLayer Strategy after amount of the specific asset has already been deposited into the EigenLayer Strategy

Explanation

The LRTConfig contract has a vulnerability that allows the modification of the asset for a particular EigenLayer Strategy after an amount of the specific asset has already been deposited into the EigenLayer Strategy. This can potentially disrupt the logic of the system, leading to unintended consequences and security risks like permanently locked users funds.

The vulnerability arises from the ability to change the asset for a particular EigenLayer Strategy even after an amount of the specific asset has already been deposited into the EigenLayer Strategy. The sequence of events leading to this issue is as follows:

1. An EigenLayer Strategy is configured with a specific asset.

2. An amount of that asset is deposited into the EigenLayer Strategy.

3. The LRTConfig contract allows the asset for the same EigenLayer Strategy to be modified, potentially switching to a different asset.

4. This modification can disrupt the logic and expectations of the system, as it may not be designed to handle asset changes for an active EigenLayer Strategy.

Impact

Changing the asset for an active EigenLayer Strategy could result in the permanently lock/loss of funds or assets being incorrectly handled.

Recommendation

Implement a restriction that prevents the modification of the asset for a particular EigenLayer Strategy after any amount of the specific asset has been deposited into that Strategy. This can be achieved by adding a check in the setAsset() function to disallow changes when there is a non-zero balance of the asset in the EigenLayer Strategy.

---

[L‑2] A malicious LRT Manager can disrupt the deposit process for a particular asset by setting depositLimitByAsset[asset] to a value smaller than the actual total supply of this asset within contracts. Consequently, the depositing function will consistently revert due to underflow

Explanation

The LRTConfig contract has a vulnerability that allows a malicious LRT Manager to disrupt the deposit process for a particular asset by setting depositLimitByAsset[asset] to a value smaller than the actual total supply of this asset within the contracts (LRTDepositPool, NodeDelegator and EigenLayer Strategy contracts). This vulnerability can cause the depositing function to consistently revert due to underflow, effectively preventing legitimate deposits.

The vulnerability occurs due to the lack of proper checks and balances in the LRTConfig contract. The sequence leading to this issue is as follows:

1. The LRT Manager has the authority to set the depositLimitByAsset for a particular asset.

2. The LRT Manager sets depositLimitByAsset[asset] to a value smaller than the actual total supply of that asset within the contract.

3. Users attempt to deposit the asset into the contract.

4. Since the depositLimitByAsset is smaller than the total supply, the depositing function consistently reverts due to an underflow error when calculating available deposit limits.

5. Legitimate deposits are prevented, causing a disruption in the deposit process for the affected asset.

Impact

The impact of this vulnerability is severe, as it can disrupt the deposit process for a particular asset. Users attempting to deposit the asset will encounter consistent reverts, making it impossible to deposit funds as intended. This can lead to a breakdown in the functionality of the ecosystem.

Likelihood: Low

Recommendation

To mitigate this vulnerability, it is crucial to implement proper checks when setting the depositLimitByAsset to ensure that it cannot be set to a value smaller than the actual total supply of the asset within the contracts (LRTDepositPool, NodeDelegator and EigenLayer Strategy contracts).

---

[L‑3] Ownership Can Be Renounced

Explanation

If the owner renounces their ownership in LRTConfig.sol, all ownable contracts will be left without an owner. As a result, any function guarded by the onlyOwner modifier will no longer be executable. Additionally, the DEFAULT_ADMIN_ROLE can be renounced or revoked.

Recommendation

Confirm whether this is the intended behavior. If not, override and disable the renounceOwnership() and revokeRole() functions in the affected contracts. For added security, consider implementing a two-step process when transferring ownership of the contract (e.g., using Ownable2Step from OpenZeppelin).

---

[L‑4] Possible DoS and Out of Gas errors

Description

The LRTDepositPool contract contains nodeDelegatorQueue array, which can lead to uncontrolled growth of the array over time. This uncontrolled growth can result in increased gas costs and potential out-of-gas errors during function calls that read or modify the array. Additionally, it may expose the contract to a denial-of-service (DoS) by causing transactions to run out of gas.

Impact

• Increased gas costs: As the nodeDelegatorQueue array grows, the gas required to execute functions that modify the array (such as addNodeDelegatorContractToQueue and transferAssetToNodeDelegator) also increases, potentially leading to higher transaction fees for users.

• Possible out-of-gas errors: If the nodeDelegatorQueue array becomes too large, it may exceed the gas limit for Ethereum transactions, resulting in out-of-gas errors and failed transactions.

• Denial-of-Service (DoS) potential: An attacker could exploit the uncontrolled growth of the nodeDelegatorQueue to launch a DoS attack by repeatedly calling functions that modify the array, causing legitimate transactions to fail due to high gas costs.

Explanation

The vulnerability arises from the fact that the nodeDelegatorQueue array is never reduced in size by removing elements. The contract allows NodeDelegator contract addresses to be added to the queue via the addNodeDelegatorContractToQueue function but does not provide a mechanism to remove addresses. As a result, the array can grow indefinitely.

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 above function allows NodeDelegator contract addresses to be added to the nodeDelegatorQueue, but there is no corresponding function to remove addresses from the queue when they are no longer needed or have fulfilled their purpose.

Proof of Concept

This vulnerability can be demonstrated by deploying the LRTDepositPool contract and repeatedly adding NodeDelegator contract addresses to the nodeDelegatorQueue using the addNodeDelegatorContractToQueue function. As addresses are added, the array will grow without limit.

// Deploy LRTDepositPool contract
// ...

// Repeatedly add NodeDelegator contract addresses to the queue
for (uint256 i = 0; i < 1000; i++) {
    lrtDepositPool.addNodeDelegatorContractToQueue([nodeDelegatorAddress]);
}

As a result, the nodeDelegatorQueue will continue to grow, potentially leading to increased gas costs and potential out-of-gas errors during the addition of addresses.

Tools Used

Manual code review and analysis.

Recommended Mitigation Steps

To address this vulnerability, it is recommended to implement a mechanism to remove or "pop" elements from the nodeDelegatorQueue when NodeDelegator contract addresses are no longer needed or have fulfilled their purpose. This can be achieved by adding a function to remove addresses from the queue or pop elements in the end of transferAssetToNodeDelegator() function when the asset is already transferred (the second possible solution is just an example mitigation step).

---

---

---

[NC‑1] Implement a check to determine if the NodeDelegator already exists within the LRTDepositPool.sol#addNodeDelegatorContractToQueue() function

Explanation

In the LRTDepositPool.sol contract, specifically within the addNodeDelegatorContractToQueue() function, there is currently no check to determine if a NodeDelegator contract address already exists in the nodeDelegatorQueue before adding it. This omission could lead to the unintended addition of duplicate addresses to the queue, potentially causing issues when processing deposits or transfers to these contracts.

Recommendation:

It is advisable to implement a check within the addNodeDelegatorContractToQueue function to ensure that duplicate NodeDelegator addresses are not added to the nodeDelegatorQueue.

Here is an example of how the function could be modified to include the check:

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

        for (uint256 i; i < length; ) {
+           if (nodeDelegatorQueue[nodeDelegatorContracts[i]] != address(0)) (
+               revert();
+           )

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

By implementing this check, the addNodeDelegatorContractToQueue function will prevent the addition of duplicate NodeDelegator contract addresses to the nodeDelegatorQueue, ensuring the integrity of the queue and avoiding potential issues that may arise from duplicates.

---

[NC‑2] A malicious user can disrupt the logic of the LRTDepositPool.sol contract by directly transferring assets to the LRTDepositPool, NodeDelegator or EigenLayer Strategy contracts

Explanation

This will disrupt the intended deposit() functioning of the contract and the associated ecosystem, as the contract relies on controlled depositing procedures to ensure proper asset management.

---

[NC‑3] Grant the LRT Manager permissions during the initialization of the LRTDepositPool contract

---

[NC‑4] Grant MINTER_ROLE and BURNER_ROLE permissions during the initialization of the RSETH contract

---

[NC‑5] Implement a function for removing assets from supportedAssets only if the asset has not already been transferred to the NodeDelegator contract

Explanation

This will prevent potential out of gas errors.

---