`rsEth` TOKEN, MINT AMOUNT CALCULATION, IS BROKEN DUE TO FEE-ON-TRANSFER TOKEN DEPOSITS HAPPEN VIA `LRTDepositPool.depositAsset` FUNCTION

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L136-L138 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L141 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L109 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L66-L78

Vulnerability details

Impact

The LRTDepositPool.depositAsset function is used to allow user to stake a particular asset amount to the protocol. After the initial input validation checks the asset transfer is executed from the msg.sender to the deposit pool as shown below:

    if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) { //@audit-info - transfer the deposit amount to this address
        revert TokenTransferFailed(); //@audit-info - revert if the transfer failed
    }

Then the depositAmount is used to calculate the rsEth (receipt token) amount to mint for the msg.sender as shown below:

    uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

The issue here is that the asset transfer in the LRTDepositPool.depositAsset function does not account for fee-on-transfer. Since the kelp product is expected to work with multiple ERC20 tokens as assets there could be certain assets which charge a fee on transfers. But in the actual asset amount transfer from the msg.sender to the deposit pool the fee-on-transfer scenario is not considered.

As a result more rsethAmountMinted amount could be minted for a msg.sender who deposits a fee-on-transfer ERC20 token asset amount (depositAmount) to the protocol. But the actual deposited asset amount to the LRTDepositPool contract would be less than the depositAmount due to the fee charged on the transfer.

This could lead to the rsethAmountToMint calculation in the LRTDepositPool.getRsETHAmountToMint erroneous as shown below:

1. getRsETHAmountToMint function calculates the rsethAmountToMint as shown below:

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

2. Here the lrtOracle.getRSETHPrice() calculation uses the total asset deposits in the protocol and converts those asset amounts into Eth amount as shown below:

   for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
       address asset = supportedAssets[asset_idx];
       uint256 assetER = getAssetPrice(asset); //@audit-info - get the asset price for each of the assets in the supported asset list
   
       uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset); //@audit-info - get the total asset amount from all three contracts
       totalETHInPool += totalAssetAmt * assetER; //@audit-info - increment the total asset amount in the contract via the ETH amount (because it is common denominator)
   
       unchecked {
           ++asset_idx;
       } //@audit-info - increment the index by one
   }

3. Then the rsEth price is calculated by dividing the total Eth amount in the protocol by the total supply of rsEth as shown below:

   return totalETHInPool / rsEthSupply;

4. The issue here is, if the fee on transfer asset tokens were deposited using the LRTDepositPool.depositAsset function the rsEthSupply does not correctly represent the totalEthInPool since the actual asset amount in the LRTDepositPool contract would be less in due to fee charged on transfer.

5. As a result the totalETHInPool / rsEthSupply value returned from the lrtOracle.getRSETHPrice will be less since being divided by a larger value (denominator) than it should have.

6. As a result the rsethAmountToMint will result in a larger value since the returned denominator lrtOracle.getRSETHPrice() will be smaller than the correct value.

7. This issue will be aggravated as more fee-on-transfer tokens are deposited into the protocol via the LRTDepositPool.depositAsset function thus breaking the rsethAmountToMint calculation.

8. As a result the receipt token amount minted to the depositors will be larger than the correct amount which indicate there is not enough underlying assets in the protocol to match the minted rsEth tokens.

9. As a result if the asset prices remain the same the later depositors will get minted more rsEth tokens than the early depositors for the same asset amount deposited.

10. Hence this breaks the rsEth accounting system completely and puts the early depositors of the protocol at a disadvantage.

Proof of Concept

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

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

        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

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

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

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

        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;

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L66-L78

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to update the asset transfer logic of the LRTDepositPool.depositAsset function by accounting for the before and after, transfer balance amount of an asset in the LRTDepositPool contract, as shown below:

function depositAsset(
    address asset,
    uint256 depositAmount
)
    external
    whenNotPaused
    nonReentrant
    onlySupportedAsset(asset)
{

    ...
    ...

    uint256 beforeTansfer = IERC20(asset).balanceOf(address(this));

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

    uint256 afterTransfer = IERC20(asset).balanceOf(address(this));

    uint256 transferredAmount = afterTransfer - beforeTransfer ;

    uint256 rsethAmountMinted = _mintRsETH(asset, transferredAmount);

    emit AssetDeposit(asset, transferredAmount, rsethAmountMinted); 

}

Assessed type

Token-Transfer

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

M-03 from the bot

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid