Kelp DAO | rsETH - 7siech's results

Lines of code

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

Vulnerability details

Impact

Users can receive significantly fewer rsETH than they should when depositing assets into a non-empty pool.

This happens because the user's funds are transferred to the LRTDepositPool before the rsETH is calculated taking into account the transferred funds and thus diluting the value of rsETH.

The more funds a user transfers in relation to the existing asset balance, the higher the dilution for the user thus punishing the most enthusiastic and trusting users of the protocol.

Proof of Concept

The depositAsset function first transfers the assets to the LRTDepositPool contract before calculating the amount of rsETH 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);
    }

The amount of rsETH to mint is calculated as the amount x assetPrice / rsETHPrice -

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

The rsETH price is calculated as essentially totalETHInPool / rsEthSupply -

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

When calculating the total amount of ETH, the oracle calls back into the pool to total the assets including the current balance which now includes the user's funds that have already been transferred to the pool thus inflating the total ETH in pool numerator resulting in a higher rsETH price and thus a lower amount of rsETH when calculating the mint amount.

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

Here is a unit test showing the issue -

    function testWrongPriceCalculation() external {
        uint supplyBefore = rseth.totalSupply();
        uint priceBefore = lrtOracle.getRSETHPrice();
        uint balanceBefore = rseth.balanceOf(alice);

        assertEq(supplyBefore, 0);
        assertEq(priceBefore, 1 ether);
        assertEq(balanceBefore, 0);

        // alice deposit 1 eth first time
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), 1 ether);
        lrtDepositPool.depositAsset(address(rETH), 1 ether);
        vm.stopPrank();

        assertEq(rseth.totalSupply(), 1 ether);
        assertEq(lrtOracle.getRSETHPrice(), 1 ether);
        assertEq(rseth.balanceOf(alice), 1 ether);

        // alice deposit another 1 eth
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), 1 ether);
        lrtDepositPool.depositAsset(address(rETH), 1 ether);
        vm.stopPrank();

        assertLt(rseth.totalSupply(), 2 ether, "total rsETH supply");
        assertGt(lrtOracle.getRSETHPrice(), 1 ether, "rsETH price");
        assertLt(rseth.balanceOf(alice), 2 ether, "alice rsETH balance");
    }

Link to fully functioning POC - https://gist.github.com/alpeware/c3f2e38ab258863ae27df88a1fa29dd3#file-depositpool-invariants-t-sol-L288

Tools Used

• Foundry invariant tests
• Manual review

Recommended Mitigation Steps

One simple option is to flip the order and first mint and then transfer the asset -

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Math