Depositor receives less rsETH than they should

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

When a user mints rsETH, the price per share is determined by calculating the total value of all the assets held by the system. Before the user's shares are calculated, the DepositPool contract transfers the user's deposit amount to itself. That causes the asset amount to be inflated when the price per share is calculated which in turn decreases the shares the user receives.

Proof of Concept

When a user deposits assets into DepositPool, it'll first transfers those assets and then calculate the shares to mint:

    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)) {
            revert TokenTransferFailed();
        }

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

To calculate the share price it uses the price of the asset and the price of rsETH which is the total value of all the underlying assets divided by the total supply of rsETH:

    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
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

    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;
    }

Given that the DepositPool holds 100 stETH, has minted 99 rsETH, and 1 stETH = 1 ETH, then rsETH price = 100e18 * 1e18 / 99e18 = 1.010101e18

If a user now wants to deposit 1 stETH they should get: 1e18 * 1e18 / 1.010101e18 = 9.9000001e17.

But, because the deposit pool has first transferred the user's funds to itself, the rsETH price changes. Instead of holding 100 stETH it now holds 101 stETH: 101e18 * 1e18 / 99e18 = 1.020202e18. Thus, the user will receive 1e18 * 1e18 / 1.020202e18 = 9.8019804e17 rsETH.

The higher the deposit amount the higher the difference in rsETH minted.

Tools Used

none

Recommended Mitigation Steps

Calculate the user's rsETH shares before transferring the underlying asset to the deposit pool.

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)