Kelp DAO | rsETH - Bauer's results

Lines of code

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

Vulnerability details

Impact

Attacker can first deposit small amount of asset token to get shares, and front-run other depositors' transactions and inflate shares price by large "donation", thus attacker may withdraw more loan tokens than he initially owned.

Proof of Concept

User can get share by depositing asset to Lending vault, the amount of minted shares is calculated as:

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

Let's assume: 1.Alice wants to invoke the depositAsset() function to deposit 1.5 ether worth of steth. 2.Bob sees Alice's transaction in mempool and front-runs by first depositing 1 wei to the LRTDepositPool and then get 1 share; 3.Bob then transfers 1 ether worth of steth directly to the pool, inflates rsETH price to (1 ether worth of steth + 1); 4.Alice's deposit transaction gets confirmed and Alice get 1 share; 5.If there is a withdrawal feature, Bob can withdraw from the pool and receive 1.5 ether worth of steth, resulting in a profit of 0.5 ether worth of steth.

Tools Used

Recommended Mitigation Steps

Consider minting a minimal amount of pool tokens during the first deposit and sending them to zero address, this increases the cost of the attack. Uniswap V2 uses the value 1000 as it is small enough to don't hurt the first minter, while still increasing the cost of this attack by 1000x. https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L121

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The current implementation poses a risk where depositors may experience unfavorable outcomes when depositing assets to receive rseth. This risk is associated with the reliance on oracle-reported prices during the calculation of rsethAmountMinted. If the oracle's price is manipulated, depositors may receive a significantly lower amount of rseth than expected, leading to potential financial losses.

Proof of Concept

The LRTDepositPool.depositAsset() function allows users to deposit a specified amount of a supported asset into the smart contrac.Inside the fuction, it calculates rsethAmountMinted based on the price retrieved from an oracle. If the oracle's reported price is manipulated, it poses a significant risk to users.

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

In such a scenario, users might receive an unfairly low amount of assets in return for their deposit , resulting in potential financial loss.

Tools Used

Vscode

Recommended Mitigation Steps

Function depositAsset() should have an input parameter uint256 minimumOutputTokens and the function should revert if require(rsethAmountMinted >= minimumOutputTokens, "Too little rseth received.");

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L51-L68

Vulnerability details

Impact

The absence of slippage protection within the depositAssetIntoStrategy function poses a significant risk to the protocol. This oversight leaves the protocol vulnerable to potential financial losses, as malicious actors can manipulate share prices during deposit transactions. The impact extends beyond financial implications, as the efficiency of the protocol in managing assets within the strategy pool is compromised.

Proof of Concept

The function NodeDelegator.depositAssetIntoStrategy() is used to deposit a specified asset into its associated strategy. Within the function, the protocol utilizes the eigenlayerStrategyManagerAddress.depositIntoStrategy() function to deposit funds into the strategy pool, acquiring corresponding shares. However, a crucial concern arises as the function lacks protection against slippage. In the absence of safeguards, malicious actors manipulating the price of shares can cause the protocol to receive fewer shares than anticipated, resulting in financial losses for the protocol.

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

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

Tools Used

Vscode

Recommended Mitigation Steps

Function depositAssetIntoStrategy() should have an input parameter uint256 minimumOutputShares and the function should revert if require(shares >= minimumOutputTokens, "Too little shares received.");

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The protocol faces a critical issue as it lacks a removal mechanism for NodeDelegatorContracts in the nodeDelegatorQueue. The absence of this feature presents two main concerns: firstly, the continuous accumulation of contracts in the array may lead to escalated gas consumption and potential out-of-gas scenarios. Secondly, the protocol is exposed to security risks, as compromised NodeDelegatorContracts cannot be promptly removed, putting users' assets at potential risk.

Proof of Concept

The protocol lacks a mechanism for removing NodeDelegatorContracts from the queue, leading to a continuous accumulation of data in the nodeDelegatorQueue array. This absence of a removal feature can result in potential issues:

 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 ever-growing nodeDelegatorQueue array, without the ability to remove contracts, may lead to increased gas consumption during contract interactions. Transaction costs could escalate, and users might face challenges interacting with the protocol due to potential gas limits.

The inability to remove NodeDelegatorContracts poses a security risk, as a compromised or malicious NodeDelegatorContract could persist indefinitely in the queue. In the event of a security vulnerability or breach in a NodeDelegatorContract, users' assets may be at risk, and the protocol's integrity could be compromised. Protocol Scalability:

Tools Used

Vscode

Recommended Mitigation Steps

It is advisable to implement a corresponding removal or deactivation mechanism for NodeDelegatorContracts to address these concerns.

Assessed type

Other