Kelp DAO | rsETH - 0xmystery's results

Lines of code

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

Vulnerability details

Impact

The identified issue in the LRTDepositPool and LRTOracle contracts presents a significant vulnerability in the calculation of the RSETH token minting rate. The core of the problem lies in the variable returns of the getRSETHPrice function, which leads to inconsistent RSETH minting rates for different stakers based on the timing of their deposits. This inconsistency can result in an unfair advantage for an early first staker, who may receive a disproportionately larger amount of RSETH for their deposits compared to later stakers. Such a discrepancy could undermine the trust in the protocol's fairness, potentially leading to reputational damage and loss of user confidence. Additionally, it poses a risk of economic imbalance within the protocol, which could be exploited for financial gain, thereby affecting the overall stability of the system.

Proof of Concept

Here's the scenario:

1. First Staker (100 ETH, i.e. stETH, rETH or cbETH Deposit):

• Assuming lrtOracle.getAssetPrice(asset) is equal to the price of ETH and initially there is no RSETH supply, the function getRSETHPrice would return 1 ETH.

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

        if (rsEthSupply == 0) {
            return 1 ether;
        }

• Therefore, rsethAmountToMint for the first staker depositing 100 ETH would be (100 ETH * 1 ETH) / 1 ETH = 100e18 RSETH tokens, assuming lrtOracle.getAssetPrice(asset) returns a value in wei (1 ETH = 1e18 wei.)

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

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

2. Second Staker (100 ETH Deposit):

• After the first deposit, the total ETH in the pool increases. The getRSETHPrice now will return a value based on the total ETH in the pool divided by the total RSETH supply.
• The total ETH in the pool is considered to be 200 ETH (100 ETH from the first staker and 100 ETH from the second staker because the asset is transferred before minting RSETH).

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

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

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

• With the total RSETH supply being 100 RSETH (from the first staker), the exchange rate would be (200 ETH * getAssetPrice(asset)) / 100 RSETH = 2 ETH.
• Therefore, rsethAmountToMint for the second staker would be (100 ETH * 1 ETH) / 2 ETH = 50e18 RSETH tokens.

Hence, rsEthSupply is now 150e18. If the first staker were to withdraw the asset, he/she would be entitled to redeem 100e18 RSETH tokens for (100e18 / 150e18) * 200e18 = 133.33e18 ETH, making an instant 33.33% gain at the loss of the second staker.

Tools Used

Manual

Recommended Mitigation Steps

Reversing the order of asset transfer and RSETH minting is one possible solution, but this is not a good check-and-effect practice despite the nonReentrant function visibility of depositAsset().

Alternatively, extending ERC-4626, the Ethereum token standard for yield-bearing vaults, to include a feature for previewing share amounts can be a beneficial enhancement to your DeFi protocol, particularly in the context of your scenario involving the minting of RSETH tokens in depositAsset(). This will nonetheless require some extensive amount of code refactoring.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The current implementation of the depositAsset function in the DeFi protocol assumes that the prices of assets like stETH, rETH, and cbETH are either stable or appreciating relative to ETH. This assumption overlooks the potential volatility and price drops of these assets against ETH. In the absence of slippage protection, users could be exposed to significant financial risk, especially in scenarios where the asset value declines sharply after initiating a deposit but before the transaction is executed. This gap can lead to unfavorable RSETH minting rates and potential economic losses for users. The lack of slippage protection could undermine user confidence in the protocol’s robustness and its ability to safeguard user interests under volatile market conditions.

This vulnerability is rated medium severity because, while it may not directly lead to immediate large-scale financial losses or exploits, it exposes users to significant risk in volatile market conditions and can impact the trust and reliability of the protocol.

Proof of Concept

Illustrative Scenario:

• A user initiates a deposit of stETH.

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

    /// @notice helps user stake LST to the protocol
    /// @param asset LST asset address to stake
    /// @param depositAmount LST asset amount to stake
    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);
    }

• Between the initiation and execution of the transaction, Lido's stETH's price drops significantly against ETH due to unforeseen slashings. LRTOracle.getRSETHPrice() returns a comparatively higher rsETHPrice than getAssetPrice(asset) because the prices of rETH and cbETH remain unaffected:

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

        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;

• As a result, the user receives a substantially lower amount of RSETH than expected.

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

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

Tools Used

Manual

Recommended Mitigation Steps

Introduce a slippage tolerance mechanism in the depositAsset function. For instance, users could specify their minimum share of RSETH minted, and transactions falling below this threshold would be reverted.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

The absence of a deadline or time limit in the depositAsset function exposes users to risks associated with transaction delays on the Ethereum blockchain, such as heavy congestion or gas fee hikes. In volatile market conditions, the price obtained from lrtOracle.getAssetPrice(asset) could significantly change during the delay, potentially leading to unfavorable exchange rates at the time of execution.

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

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

This scenario can result in economic losses for users who expect a certain amount of RSETH based on the asset price at the time of initiating the transaction. Implementing a deadline within which the transaction must be executed can mitigate this risk by providing users with control over the maximum allowable time for their transactions, enhancing the security and reliability of the protocol.

This issue is rated as medium severity due to its potential to cause financial losses for users in specific scenarios of network congestion and price volatility, impacting user trust and the protocol's perceived reliability.

Proof of Concept

Illustrative Scenario:

• A user initiates a transaction to deposit assets using depositAsset().

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

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

• Due to network congestion, the transaction is delayed.
• In the interim, the asset's price decreases. By the time the transaction is executed, LRTOracle.getRSETHPrice() returns a comparatively higher rsETHPrice than getAssetPrice(asset) because the prices of other supported LST's remain unaffected:

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

        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;

• Consequently, the user receives a significantly lower amount of RSETH than expected based on the asset price at the initiation time.

Tools Used

Manual

Recommended Mitigation Steps

Introduce Transaction Deadline: Modify the depositAsset function to include a deadline parameter. Transactions that are not executed before this deadline should be automatically reverted. User-Defined Deadlines: Allow users to specify their own deadlines based on their risk tolerance and expectations regarding network conditions.

Assessed type

Timing