depositAsset should call transferFrom after _mintRsETH

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Depositors will get less rsETH than designed, especially when depositAmount is big and there are few assets in the protocol.

Proof of Concept

In depositAsset, we first call transferFrom to transfer the asset into the pool, and then call _mintRsETH to mint rsETH:

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

In _mintRsETH, the rsETH amount is calculated from getRsETHAmountToMint, which calls lrtOracle.getRSETHPrice():

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

In getRSETHPrice, is rsETH supply is not zero, we calculate the price using totalETHInPool / rsEthSupply where totalETHInPool is the total assets value in the protocol:

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

The assets value in the pool will also be taken into account:

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));     <----

So the problem is:

1. Users deposit assets into the pool to get rsETH.
2. Before the rsETH tokens are minted, the assets are transferred into the pool.
3. When minting rsETH, the amount is based on the assets' value divided by rsETH price, which is calculated by the protocol's value divided by rsETH supply.

So, the rsETH price will increase after the transfer of the assets and before the rsETH tokens are minted, which leads to fewer rsETH tokens for the depositor, especially when the deposit is big and there are few assets in the protocol, in which case the rsETH price will increase a lot.

Tools Used

Manual Review.

Recommended Mitigation Steps

--- a/src/LRTDepositPool.sol
+++ b/src/LRTDepositPool.sol
@@ -135,9 +135,6 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
             revert MaximumDepositLimitReached();
         }

-        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
-            revert TokenTransferFailed();
-        }
@@ -145,6 +142,10 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
         // interactions
         uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

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

Assessed type

Context

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