Skip to content
This repository has been archived by the owner on Nov 24, 2024. It is now read-only.

no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos #46

Open
sherlock-admin3 opened this issue May 20, 2024 · 4 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Disputed The sponsor disputed this issue's validity

Comments

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented May 20, 2024

no

high

Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos

Summary

Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos

Vulnerability Detail

 function _stake(uint256 stakeAmount) internal override returns (uint256) {
        if (stakeAmount == 0) return 0;

        // Check LRTDepositPool stake limit
        uint256 stakeLimit = RSETH_DEPOSIT_POOL.getAssetCurrentLimit(Constants.ETH);
        if (stakeAmount > stakeLimit) {
            // Cap stake amount
            stakeAmount = stakeLimit;
        }
        // Check LRTDepositPool minAmountToDeposit
@>        if (stakeAmount <= RSETH_DEPOSIT_POOL.minAmountToDeposit()) revert MinAmountToDepositError();
        // Check paused of LRTDepositPool
        if (RSETH_DEPOSIT_POOL.paused()) revert ProtocolPaused();

        // Interact
        IWETH9(Constants.WETH).withdraw(stakeAmount);
        uint256 _rsETHAmt = RSETH.balanceOf(address(this));
        RSETH_DEPOSIT_POOL.depositETH{value: stakeAmount}(0, REFERRAL_ID);
        _rsETHAmt = RSETH.balanceOf(address(this)) - _rsETHAmt;

        if (_rsETHAmt == 0) revert InvariantViolation();

        return stakeAmount;
    }

The _stake will revert in the condition that the stakeAmount is less than RSETH_DEPOSIT_POOL.minAmountToDeposit(), which is 100000000000000. This could always happens. Because stakeAmount is not the user's input, it is calculate by this protocal.

function prefundedDeposit() external nonReentrant onlyTranche returns (uint256, uint256) {
        LSTAdapterStorage storage $ = _getStorage();

        uint256 bufferEthCache = $.bufferEth; // cache storage reads
        uint256 queueEthCache = $.totalQueueEth; // cache storage reads
        uint256 assets = IWETH9(WETH).balanceOf(address(this)) - bufferEthCache; // amount of WETH deposited at this time
        uint256 shares = previewDeposit(assets);

        if (assets == 0) return (0, 0);
        if (shares == 0) revert ZeroShares();

        // Calculate the target buffer amount considering the user's deposit.
        // bufferRatio is defined as the ratio of ETH balance to the total assets in the adapter in ETH.
        // Formula:
        // desiredBufferRatio = (totalQueueEth + bufferEth + assets - s) / (totalQueueEth + bufferEth + stakedEth + assets)
        // Where:
        // assets := Amount of ETH the user is depositing
        // s := Amount of ETH to stake at this time, s <= bufferEth + assets.
        //
        // Thus, the formula can be simplified to:
        // s = (totalQueueEth + bufferEth + assets) - (totalQueueEth + bufferEth + stakedEth + assets) * desiredBufferRatio
        //   = (totalQueueEth + bufferEth + assets) - targetBufferEth
        //
        // Flow:
        // If `s` <= 0, don't stake any ETH.
        // If `s` < bufferEth + assets, stake `s` amount of ETH.
        // If `s` >= bufferEth + assets, all available ETH can be staked in theory.
        // However, we cap the stake amount. This is to prevent the buffer from being completely drained.
        //
        // Let `a` be the available amount of ETH in the buffer after the deposit. `a` is calculated as:
        // a = (bufferEth + assets) - s
        uint256 targetBufferEth = ((totalAssets() + assets) * $.targetBufferPercentage) / BUFFER_PERCENTAGE_PRECISION;

        /// WRITE ///
        _mint(msg.sender, shares);

        uint256 availableEth = bufferEthCache + assets; // non-zero

        // If the buffer is insufficient or staking is paused, doesn't stake any of the deposit
        StakeLimitTypes.Data memory data = $.packedStakeLimitData.getStorageStakeLimitStruct();
        if (targetBufferEth >= availableEth + queueEthCache || data.isStakingPaused()) {
            /// WRITE ///
            $.bufferEth = availableEth.toUint128();
            return (assets, shares);
        }

        // Calculate the amount of ETH to stake
        uint256 stakeAmount; // can be 0
        unchecked {
    @>        stakeAmount = availableEth + queueEthCache - targetBufferEth; // non-zero, no underflow
        }
        // If the calculated stake amount exceeds the available ETH, simply assign the available ETH to the stake amount.
        // Possible scenarios:
        // - Target buffer percentage was changed to a lower value and there is a large withdrawal request pending.
        // - There is a pending withdrawal request and the available ETH are not left in the buffer.
        // - There is no pending withdrawal request and the available ETH are not left in the buffer.
        if (stakeAmount > availableEth) {
            // Note: Admins should be aware of this situation and take action to refill the buffer.
            // - Pause staking to prevent further staking until the buffer is refilled
            // - Update stake limit to a lower value
            // - Increase the target buffer percentage
    @>          stakeAmount = availableEth; // All available ETH
        }

        // If the amount of ETH to stake exceeds the current stake limit, cap the stake amount.
        // This is to prevent the buffer from being completely drained. This is not a complete solution.
        uint256 currentStakeLimit = StakeLimitUtils.calculateCurrentStakeLimit(data); // can be 0 if the stake limit is exhausted
        if (stakeAmount > currentStakeLimit) {
    @>          stakeAmount = currentStakeLimit;
        }
        /// WRITE ///
        // Update the stake limit state in the storage
        $.packedStakeLimitData.setStorageStakeLimitStruct(data.updatePrevStakeLimit(currentStakeLimit - stakeAmount));

        /// INTERACT ///
        // Deposit into the yield source
        // Actual amount of ETH spent may be less than the requested amount.
    @>      stakeAmount = _stake(stakeAmount); // stake amount can be 0

        /// WRITE ///
        $.bufferEth = (availableEth - stakeAmount).toUint128(); // no underflow theoretically

        return (assets, shares);
    }

The stakeAmount could be any small value. The users deposit right value using Tranche, but could revert, and they don't konw why.

Impact

The users deposit right value using Tranche, but could revert, and they don't konw why.

Code Snippet

https://github.com/sherlock-audit/2024-05-napier-update/blob/main/napier-uups-adapters/src/adapters/kelp/RsETHAdapter.sol#L77-L77

Tool used

Manual Review

Recommendation

 function _stake(uint256 stakeAmount) internal override returns (uint256) {
        if (stakeAmount == 0) return 0;

        // Check LRTDepositPool stake limit
        uint256 stakeLimit = RSETH_DEPOSIT_POOL.getAssetCurrentLimit(Constants.ETH);
        if (stakeAmount > stakeLimit) {
            // Cap stake amount
            stakeAmount = stakeLimit;
        }
        // Check LRTDepositPool minAmountToDeposit
-        if (stakeAmount <= RSETH_DEPOSIT_POOL.minAmountToDeposit()) revert MinAmountToDepositError();
+        if (stakeAmount <= RSETH_DEPOSIT_POOL.minAmountToDeposit()) return 0;
        // Check paused of LRTDepositPool
        if (RSETH_DEPOSIT_POOL.paused()) revert ProtocolPaused();

        // Interact
        IWETH9(Constants.WETH).withdraw(stakeAmount);
        uint256 _rsETHAmt = RSETH.balanceOf(address(this));
        RSETH_DEPOSIT_POOL.depositETH{value: stakeAmount}(0, REFERRAL_ID);
        _rsETHAmt = RSETH.balanceOf(address(this)) - _rsETHAmt;

        if (_rsETHAmt == 0) revert InvariantViolation();

        return stakeAmount;
    }
@massun-onibakuchi
Copy link

Such DoS doesn't meat requirements. This is because

  • The revert doesn't always happen. It can happens when depositing small amount.
  • It's unlikely that users deposit such small amount, wasting gas cost.
  • stakeAmount can be changed by changing maxStakeLimit on adapter and we can definitely avoid such revert if needed

@sherlock-admin3 sherlock-admin3 added Sponsor Disputed The sponsor disputed this issue's validity Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed and removed Sponsor Disputed The sponsor disputed this issue's validity Sponsor Confirmed The sponsor acknowledged this issue is valid labels May 21, 2024
@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label May 22, 2024
@sherlock-admin2
Copy link

1 comment(s) were left on this issue during the judging contest.

z3s commented:

Low/Info; For an issue to be a valid Denial of Service (DoS), it must meet one of these criteria: 1. The issue causes locking of funds for users for more than a week. 2. The issue impacts the availability of time-sensitive functions. but The stakeAmount can be modified by changing the maxStakeLimit.

@0502lian
Copy link
Collaborator

0502lian commented May 23, 2024

it's not because It can happens when depositing small amount. It's because stakeAmount is calculated by prefundedDeposit . User deposit a large amount, stakeAmount can still be small amount (even zero) in _stake()

@sherlock-admin2 sherlock-admin2 changed the title Great Seafoam Reindeer - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos May 24, 2024
@sherlock-admin2 sherlock-admin2 added the Non-Reward This issue will not receive a payout label May 24, 2024
@sherlock-admin3 sherlock-admin3 removed the Will Fix The sponsor confirmed this issue will be fixed label May 27, 2024
@WangSecurity WangSecurity added the Has Duplicates A valid issue with 1+ other issues describing the same vulnerability label Jun 1, 2024
@WangSecurity
Copy link

After the discussions on escalation on #54, this report will be the main issue of a new family.

@Evert0x Evert0x added the Medium A valid Medium severity issue label Jun 1, 2024
@Evert0x Evert0x reopened this Jun 1, 2024
@sherlock-admin2 sherlock-admin2 added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Jun 1, 2024
@sherlock-admin4 sherlock-admin4 removed the Excluded Excluded by the judge without consulting the protocol or the senior label Jun 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Disputed The sponsor disputed this issue's validity
Projects
None yet
Development

No branches or pull requests

7 participants