Malicious users can make depositors receive less RSETH tokens than expected

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTOracle.sol#L52-L79 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L95 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L151

Vulnerability details

Impact

Malicious users can make depositors receive less RSETH tokens than expected.

Proof of Concept

In the usual case, the user calls the depositAsset function, which calls _mintRsETH that mints RSETH tokens to the depositor. _mintRsETH calls getRsETHAmountToMint, which calculates the amount of RSETH tokens to be minted. getRsETHAmountToMint function calculates the number of tokens to be minted in the following way:

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

Notice, the denominator - 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;
    }

It returns totalETHInPool / rsEthSupply. So, in the calculation for rsethAmountToMint, totalETHInPool is inversely related to rsethAmountToMint. totalAssetAmt is given by the getTotalAssetDeposits, which uses getAssetDistributionData.

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

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

It can be seen that this function uses balanceOf to calculate the asset balance across the deposit pool, node delegators etc. This is where a malicious user can abuse the system.

A user calls the depositAsset function. A malicious user notices this transaction in the mempool and directly transfers the asset tokens using the ERC20 transfer function to the LRTDepositPool. This would increase the number of totalAssetAmt and thus the totalETHInPool amount based on the above discussion.

So, when the depositAsset executes, the original user receives fewer RSETH tokens than expected because totalETHInPool is inversely related to rsethAmountToMint.

Tools Used

Manual review

Recommended Mitigation Steps

It would be better to use internal accounting for functions like getAssetDistributionData to use, rather than depending on the balance of the tokens.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #168

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)