Kelp DAO | rsETH - Yanchuan's results

Lines of code

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

Vulnerability details

Impact

There is a deposit limit for every kind of asset, and the amount of the deposited asset cannot exceed this limit. Before a user deposits an asset, they need to call the getTotalAssetDeposits function to determine the current limit of asset deposit. This value is equal to the capacity limit minus the amount of the asset already deposited. Assets are stored in three locations: depositPool, NodeDelegators, and EigenLayer. The amount of the deposited asset is the sum of the assets in these three locations. https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L47-L51

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

    function getAssetCurrentLimit(address asset) public view override returns (uint256) {
        return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
    }

There are multiple NodeDelegators in the system, and each NodeDelegator can stake assets in EigenLayer. When calculating the amount of assets, it involves iterating through all NodeDelegators and summing up the assets. https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L71-L89

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

Only the admin can add NodeDelegators to the system, and this is accomplished through the addNodeDelegatorContractToQueue function. However, when the admin adds a NodeDelegator to the system, there is no check for whether the NodeDelegator is a duplicate. https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L162-L176

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

This introduces a vulnerability. If a NodeDelegator is added redundantly, the getAssetDistributionData function will redundantly calculate the amount of assets in both the NodeDelegator and EigenLayer, resulting in an overestimation of the total asset amount. As a consequence, the amount of asset that users can deposit cannot reach the deposit limit..

Proof of Concept

Here is a test case: (1) The admin invokes the addNodeDelegatorContractToQueue function to add three NodeDelegators, but all three NodeDelegators are nodeDelegatorContractOne. (2) Manager sets the deposit limit to 10e18 (3) Alice deposits 2e18 rETH into the depositPool. (4) The Manager transfers all rETH from the depositPool to the NodeDelegator. (5) Alice deposits an additional 5e18 rETH into the depositPool.

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { BaseTest } from "./BaseTest.t.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";

import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

import { console } from "forge-std/Test.sol";

contract LRTOracleMock {
    function getAssetPrice(address) external pure returns (uint256) {
        return 1e18;
    }

    function getRSETHPrice() external pure returns (uint256) {
        return 1e18;
    }
}

contract MockNodeDelegator {   

    function getAssetBalance(address) external view returns (uint256) {
        return 0;
    }
}


contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;

    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(proxyAdmin),
            ""
        );

        lrtDepositPool = LRTDepositPool(address(contractProxy));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));
        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(new LRTOracleMock()));

        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        vm.stopPrank();
    }
}


contract LRTDepositPoolAddDupDelegators is LRTDepositPoolTest {

    address public nodeDelegatorContractOne;
    address[] public nodeDelegatorQueueProspectives;

    function setUp() public override {
        super.setUp();

        // initialize LRTDepositPool
        lrtDepositPool.initialize(address(lrtConfig));

        nodeDelegatorContractOne = address(new MockNodeDelegator());

        nodeDelegatorQueueProspectives.push(nodeDelegatorContractOne);
        nodeDelegatorQueueProspectives.push(nodeDelegatorContractOne);
        nodeDelegatorQueueProspectives.push(nodeDelegatorContractOne);

        // add manager role within LRTConfig
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();

        // add node delegator contracts to queue
        vm.startPrank(admin);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueueProspectives);
        vm.stopPrank();
    }

    function test_AddDupDelegators() external {

        vm.prank(manager);
        lrtConfig.updateAssetDepositLimit(address(rETH), 10 ether);
        console.log("Asset current limit:  %s", lrtDepositPool.getAssetCurrentLimit(address(rETH)));

        // deposit 2 ether rETH
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), 2 ether);
        vm.stopPrank();

        console.log("Asset current limit before delegator:  %s", lrtDepositPool.getAssetCurrentLimit(address(rETH)));

        // transfer 2 ether rETH to node delegator contract one
        vm.startPrank(manager);
        lrtDepositPool.transferAssetToNodeDelegator(0, address(rETH), 2 ether);
        vm.stopPrank();

        console.log("Asset current limit after delegator  %s", lrtDepositPool.getAssetCurrentLimit(address(rETH)));

        vm.prank(alice);
        lrtDepositPool.depositAsset(address(rETH), 5 ether);   
    }

}

From the test results, it can be observed that after the Manager deposits rETH into the NodeDelegator, the value of the asset current limit becomes 4e18. When Alice attempts to deposit an additional 5e18 rETH into the depositPool, the transaction reverts.

Running 1 test for test/LRTDepositPoolTest.t.sol:LRTDepositPoolAddDupDelegators
[FAIL. Reason: MaximumDepositLimitReached()] test_AddDupDelegators() (gas: 309055)
Logs:
  Asset current limit:  10000000000000000000
  Asset current limit before delegator:  8000000000000000000
  Asset current limit after delegator  4000000000000000000

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 7.96ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/LRTDepositPoolTest.t.sol:LRTDepositPoolAddDupDelegators
[FAIL. Reason: MaximumDepositLimitReached()] test_AddDupDelegators() (gas: 309055)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

In the addNodeDelegatorContractToQueue function, check the newly added NodeDelegator for (1) duplicates within the nodeDelegatorContracts itself and (2) presence in the nodeDelegatorQueue. If there are duplicate NodeDelegators, revert the transaction.

Assessed type

Context