rsETH amount to mint calculation when depositing an asset in the LRTDepositPool::depositAsset() is incorrect, leading to an immediate loss of value

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L136
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L141
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L151-L152

Vulnerability details

Bug description

In this protocol, users deposit underlying assets and get the corresponding amount of rsETH for their deposits. How many rsETH will be minted is calculated based on the underlying asset's price and the current rsETH price, which are fetched from the LRTOracle contract.
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L109

file: LRTDepositPool.sol 
        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

In the LRTOracle contract:

• asset price is returned directly from the ChainLink oracle. (Ref)

• rsETH price is calculated based on the total deposited ETH value in the pool and the total supply of the minted rsETH. (Ref)

Everything is as expected up to this point. Now, let's check the depositAsset() function:
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L119C1-L144C6

file: LRTDepositPool.sol
    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)) { //@audit Transfer is alreay made before calculating how many tokens to mint. 
            revert TokenTransferFailed();
        }

        // interactions
-->     uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount); //@audit amountToMint is calculated here. rsETH price is already increased when the transaction got here due to the transfer above. 

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

As we can see above, the calculation to decide how many rsETH to mint is performed after the token transfer is completed. At that point, the rsETH price has already been increased, which causes users to get less rsETH.

When the transaction is executed completely, rsETH supply will increase and the price will fall down. The impact depends on how many tokens are deposited.

Down below you can see a coded PoC showing a scenario where

• the user deposited 100 ether worth of assets when the rsETH price is equal to 1 ether (total asset balance in the pool was 100, rsETH supply was 100)

• rsETH's are minted based on the price of 2 ether during the transaction (total balance increased to 200 but the rsETH supply was still 100 during getRsETHAmountToMint() function)

• The user got 50 rsETH

• Transaction finalized and the last price of rsETH is 1.33 ether (total balance in the pool 200, rsETH supply 150)

• The user immediately lost 1/3 of his/her deposit value (deposited 100 ether worth of tokens, got 66.6666 ether worth of tokens)

Impact

• rsETH price will be calculated higher than its actual value during the transaction
• Depositors get less rsETH than expected due to this higher calculated price
• The rsETH price will decrease after the transaction is completed due to increased supply.
• At the end, users will immediately lose some of their deposited value depending on how much they deposited.

Proof of Concept

Coded PoC

Note: This PoC uses a mock LRTOracle similar to the protocol's other tests. In this mock contract, the rsETH price calculation is exactly the same as the code in scope. The only difference is that the asset price is fixed at 1 ether.

Create a new file in the test folder and copy and paste the snippet to this file. The setup of this file is the same setup in the protocol's other test files.

To run it: forge test --match-test test_MintAmount_WhenDepositing_Is_Incorrect -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 { IRSETH } from "src/interfaces/IRSETH.sol";

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

import "forge-std/console.sol";
import "forge-std/console2.sol";


// This is a mock oracle that mimics rsETH calculation exactly the same as LRTOracle contract.
// Only difference is getAssetPrice function always returs 1e18 instead of fetching it from the Chainlink  
contract LRTOracleMock {
    ILRTConfig public lrtConfig;

    function initialize(address _lrtConfig) public {
        lrtConfig = ILRTConfig(_lrtConfig);
    }

    // This will be the underlying asset price and it will be equal to 1 ether for this test.
    function getAssetPrice(address) external view returns (uint256) {
        return 1e18;
    }

    // This is the same function in the LRTOracle Contract, the only difference is underlying asset price is always 1e18
    function getRSETHPrice() external view returns (uint256) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        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];
            // The price of asset will be 1 ether
            uint256 assetER = 1e18;

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

            unchecked {
                ++asset_idx;
            }
        }
        return totalETHInPool / rsEthSupply;
    }
}

// This will return 0 as we assume there is no staked balance in EigenLayer
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 and initialize it the config and asset.
        LRTOracleMock oracle = new LRTOracleMock();
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(oracle));
        oracle.initialize(address(lrtConfig));

        // add deposit pool contract address to LRT config
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));

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

        vm.stopPrank();
    }
}

contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
    address public oracle;

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

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

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

        oracle = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
    }

    function test_MintAmount_WhenDepositing_Is_Incorrect() external {
        // We will prepare the situation first.
        // Initially we'll deposit 100 rETH tokens to set the scene.
        address randomAddress = makeAddr("random");
        rETH.mint(randomAddress, 100 ether);

        vm.startPrank(randomAddress);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), 100 ether);
        vm.stopPrank();

        // After initial minting 
        // Total deposits 100, total minted rsETH 100, rsETH price 1
        assertEq(lrtDepositPool.getTotalAssetDeposits(address(rETH)), 100 ether);
        assertEq(rseth.totalSupply(), 100 ether);
        assertEq(LRTOracleMock(oracle).getRSETHPrice(), 1 ether);
        
        //------------------------------------- A USER WANTS TO DEPOSIT ANOTHER 100 ETHER --------------------------
        // Normally user should be able to deposit 100 rETH and mint corresponding 100 rsETH since the price is still 1 ether.

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

        // Transfer is made before the calculation of how many tokens will be minted
        // During the deposit transaction, rsETH price is calculated as 2 ether instead of 1 (total deposits increased to 200, rsETH supply remained at 100) --> 50 rsETH will be minted to Alice instead of 100
        // Right after the transaction is completed:
        //       total supply of rsETH will be 150 - total asset deposits will be 200
        //       price of the rsETH will be 1.33  (200/150)
        //       Alice's 50 rsETH will be worth 66.6 eth ---> Alice will lose 1/3 of her deposit immediately
        assertEq(lrtDepositPool.getTotalAssetDeposits(address(rETH)), 200 ether);
        assertEq(rseth.totalSupply(), 150 ether);
        assertEq(rseth.balanceOf(alice), 50 ether);
        assertEq(LRTOracleMock(oracle).getRSETHPrice(), 1333333333333333333);        
    }

After running the tests, the result is like below:

Running 1 test for test/DepositPriceTest.t.sol:LRTDepositPoolDepositAsset
[PASS] test_MintAmount_WhenDepositing_Is_Incorrect() (gas: 317292)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.71ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, foundry

Recommended Mitigation Steps

rsETH amount to mint calculation should be done before transferring these tokens to the pool.

It might be implemented in different ways. For an example:

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // skipped for brevity

+        // Calculate the mint amount first.
+       uint256 rsethamountToMint = getRsETHAmountToMint(asset, depositAmount);

+       // Transfer after
        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
+       // This function does not need to take two arguments anymore. Use already calculated rsethamountToMint in here
+       _mintRsETH(rsethamountToMint);

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

+   // This function just mints necessary amount, doesn't return any value
+   function _mintRsETH(amount) private {
-       (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);
        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

Assessed type

Math

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #62

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[c4-judge]

fatherGoose1 changed the severity to 3 (High Risk)