Lines of code

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

Vulnerability details

Users will receive less RSETH tokens when depositing into the contract using LRTDepositPool::deposit(...). The function transfers the deposit tokens to itself and then makes calculation for the amount of RSETH to be minted for the user. Because of this token transfer to self before the calculation for the mint, the users will get less tokens then actual amount.

Impact

The LRTDepositPool::deposit(...) function makes call to different functions from the same contract that again calls other contracts to calculate the amount of token to mint for the user. Here is a high level flow:

LRTDepositPool::deposit()
           |_ LRTDepositPool::_mintRsETH() 
                      |_ LRTDepositPool::getRsETHAmountToMint() 
                                |_ LRTOracle::getAssetPrice()
                                |_ LRTOracle::getRSETHPrice()
                                            |_ LRTOracle::getSupportedAssetList()
                                            |_ LRTOracle::getAssetPrice()
                                            |_ LRTDepositPool::getTotalAssetDeposits()
@>                                                          |_ LRTDepositPool::getAssetDistributionData()
                                                                   |_ NodeDelegator::getAssetBalance()

This function makes a call to LRTDepositPool::getAssetDistributionData()(indirectly) to fetch the total amount of tokens deposited in LRTDepositPool, NodeDelegator and Eigen Layer strategies. To fetch the assets deposited in LRTDepositPool this LRTDepositPool::getAssetDistributionData() function perform the following operation:

File: Breadcrumbs2023-11-kelp/src/LRTDepositPool.sol

        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

Github: 79

This just reads the balance of the asset that LRTDepositPool has. Now the problem is, when user calls LRTDepositPool::deposit(), this function first transfer the tokens to itself and then calls _mintRsETH() to calculate the amount of RSETH to mint to the user.

2023-11-kelp/src/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();
        }

        // this will increase the balance before any calculation
@>        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
@>        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Github: 136-138,141

But when a call reaches to LRTDepositPool::getAssetDistributionData(), the function calculate the assetLyingInDepositPool using the above operation that just reads the balance of token that is stored in LRTDepositPool using ERC20::balanceOf() function. But this balance now also stores the newly added tokens as they were transferred to this contract in the beginning. That means the data in assetLyingInDepositPool will be more than it should be. This amount will be returned back to LRTOracle::getRSETHPrice() after going through some other functions that will add this amount to balance of token stored in NodeDelegator and Eigen Layer strategies.


File: 2023-11-kelp/src/LRTOracle.sol

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        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];
            uint256 assetER = getAssetPrice(asset);

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

            unchecked {
                ++asset_idx;
            }
        }

        // expensive?
        return totalETHInPool / rsEthSupply;
    }

Github: 70-71

This totalAssetAmt contains all amount of a token that is stored in different contracts that also includes the newly deposited amount. Then this amount will be multiplied with corresponding ETH price feed and will be stored in totalETHInPool. Now when the RSETH price is returned from the function it will be in ratio of total eth deposited in pools to the total supply of RSETH. And since the RSETH supply will be less than the total deposits of eth stored in totalETHInPool because the corresponding RSETH has not been minted yet. the ratio returned will be more than expected. This ratio will increase if the amount provided to LRTDepositPool::deposit(...) is increased.

Now this ratio will be returned to the LRTDepositPool::getRsETHAmountToMint() and it will perform the following calculation:

File: 2023-11-kelp/src/LRTDepositPool.sol

    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
        // @audit what if the tokens has different decimals?

@>        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

Github: 95-110

This ratio returned is in the denominator of the calculation that means the bigger this ratio is and the lesser will be rsethAmountToMint. And this rsethAmountToMint is the actual amount of RSETH token that will be minted to the user. So bigger the deposit by the user, the lesser the tokens he will receive.

Proof of Concept

Test for Poc

    function test_depositToPoolMultipleTimes() public {
        uint256 amount = 500 ether;

        for (uint256 i; i < 5; i++) {
            // minting tokens to user to make sure he has enough tokens
            cbETH.mint(alice, amount);

            uint256 userBalanceBefore = IERC20(cbETH).balanceOf(alice);
            (uint256 rsEthMinted) = depositAssetToPool(address(cbETH), amount, alice, i + 1);
            uint256 userBalanceAfter = IERC20(cbETH).balanceOf(alice);

            assertEq(userBalanceBefore - userBalanceAfter, amount, "amount is not same");
        }
    }

Link to the original test: Test

Output

 ------------------------ Run 1:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 0
  LRTDepositPool Balance After Deposit: 500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 500000000000000000000
  TotalSupply of RSETH: 500000000000000000000

  ------------------------------------------------------------------------

  ------------------------ Run 2:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 500000000000000000000
  LRTDepositPool Balance After Deposit: 1000000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 750000000000000000000

  ------------------------------------------------------------------------

  ------------------------ Run 3:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 1000000000000000000000
  LRTDepositPool Balance After Deposit: 1500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 1000000000000000000000

  ------------------------------------------------------------------------

  ------------------------ Run 4:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 1500000000000000000000
  LRTDepositPool Balance After Deposit: 2000000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 1250000000000000000000

  ------------------------------------------------------------------------

  ------------------------ Run 5:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 2000000000000000000000
  LRTDepositPool Balance After Deposit: 2500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 1500000000000000000000

  ------------------------------------------------------------------------

As you can see the RSETH minted was equal to the token deposited in the first deposit only because the RSETH supply was 0 at that time and LRTOracle::getRSETHPrice(...) function has this check that ensures that the ratio will be 1 ether in case the supply of RSETH is zero:

File: 2023-11-kelp/src/LRTOracle.sol

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

Github: 56

So the ratio of token minted by token deposited in second transaction becomes = 250 / 500 = 0.5. Or we can say for each token deposit we got back 0.5 RSETH.

The mint amount get lesser when bigger amount of tokens are deposited. Here is a test that proves that:

    function test_depositToPoolMultipleTimesDifferentAmount() public {
        uint256 amount = 500 ether;
        uint256 amount2 = 20_000 ether;

        // minting tokens to user to make sure he has enough tokens
        cbETH.mint(alice, amount + amount2);

        uint256 userBalanceBefore = IERC20(cbETH).balanceOf(alice);
        (uint256 rsEthMinted) = depositAssetToPool(address(cbETH), amount, alice, 1);
        uint256 userBalanceAfter = IERC20(cbETH).balanceOf(alice);
        assertEq(userBalanceBefore - userBalanceAfter, amount, "amount is not same");

        userBalanceBefore = IERC20(cbETH).balanceOf(alice);
        (rsEthMinted) = depositAssetToPool(address(cbETH), amount2, alice, 2);
        userBalanceAfter = IERC20(cbETH).balanceOf(alice);

        assertEq(userBalanceBefore - userBalanceAfter, amount2, "amount is not same");
    }

Link to original Test: 373

Output:

PS E:\kelp-audit> forge test --mt test_depositToPoolMultipleTimesDifferentAmount -vv  
[⠰] Compiling...
No files changed, compilation skipped

Running 1 test for test/Integration.t.sol:BaseIntegrationTest
[PASS] test_depositToPoolMultipleTimesDifferentAmount() (gas: 413133)    
Logs:

  ------------------------ Run 1:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 0
  LRTDepositPool Balance After Deposit: 500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 500000000000000000000
  TotalSupply of RSETH: 500000000000000000000

  ------------------------------------------------------------------------

  ------------------------ Run 2:  Deposit Info  ------------------------

  Deposit Amount: 20000000000000000000000
  LRTDepositPool Balance Before Deposit: 500000000000000000000
  LRTDepositPool Balance After Deposit: 20500000000000000000000
  Change in Balance of LRTDepositPool: 20000000000000000000000
  RSETH Minted to User: 487804878048780487804
  TotalSupply of RSETH: 987804878048780487804

  ------------------------------------------------------------------------

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.11ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
PS E:\kelp-audit>

As you can can see for 500 tokens we got 500 RSETH initially and for 20000 tokens we got only 487 RSETH approx. So the ratio of token minted by token deposited becomes = 487 / 20000 = 0.02 approx. Or we can say for each token deposit we got 0.02 RSETH.

Tools Used

Manual Review
Foundry

Recommended Mitigation Steps

It is recommended to add the following check after the minting function:

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

        // this will increase the balance before any calculation
-        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
-            revert TokenTransferFailed();
-        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Other

code-423n4 / 2023-11-kelp-findings

Less `RSETH` tokens will be minted to the users when depositing tokens using `LRTDepositPool::deposit(...)` because of incorrect placement of token transfer statement / operation in the function #256