merlin - DOS vulnerability in the _stake function in RsETHAdapter.sol

[sherlock-admin4]

merlin

medium

DOS vulnerability in the _stake function in RsETHAdapter.sol

Summary

There is a scenario where a user tries to deposit the minimum amount of ETH, which is accepted by LRTDepositPool, but the transaction simply fails.

Vulnerability Detail

LRTDepositPool has a minAmountToDeposit = 0.0001 ETH.
It's also important to pay attention to the check for minAmountToDeposit in the LRTDepositPool smart contract:

function _beforeDeposit(
        address asset,
        uint256 depositAmount,
        uint256 minRSETHAmountExpected
    )
        private
        view
        returns (uint256 rsethAmountToMint)
    {
-->     if (depositAmount == 0 || depositAmount < minAmountToDeposit) {
            revert InvalidAmountToDeposit();
        }
        /// ...
}

So, if depositAmount < minAmountToDeposit, the transaction will fail, but in RsETHAdapter, there is an incorrect check that will not allow depositing the minAmountToDeposit value:

if (stakeAmount <= RSETH_DEPOSIT_POOL.minAmountToDeposit()) revert MinAmountToDepositError();

I want to consider two cases:

1)If the user's stakeAmount is equal to minAmountToDeposit, the transaction will fail, although it should not. If the user specifies stakeAmount > minAmountToDeposit, the transaction will be successfully executed.
2)If stakeAmount > stakeLimit and stakeLimit = minAmountToDeposit, then stakeAmount will be set to minAmountToDeposit. This situation will prevent the user from making a deposit, and it can only be fixed by correcting the code:

uint256 stakeLimit = RSETH_DEPOSIT_POOL.getAssetCurrentLimit(Constants.ETH); // 0.0001 ETH
        if (stakeAmount > stakeLimit) { // 1 ETH > 0.0001 ETH
            // Cap stake amount
            stakeAmount = stakeLimit;  // stakeAmount = 0.0001 ETH
        }
        // Check LRTDepositPool minAmountToDeposit
        if (stakeAmount <= RSETH_DEPOSIT_POOL.minAmountToDeposit()) revert MinAmountToDepositError(); // tx revert

Impact

The user is unable to deposit the minimum amount of ETH that LRTDepositPool accepts.

Code Snippet

src/adapters/kelp/RsETHAdapter.sol#L77

Tool used

Manual Review

Recommendation

Consider modifying the check for minAmountToDeposit:

- if (stakeAmount <= RSETH_DEPOSIT_POOL.minAmountToDeposit()) revert MinAmountToDepositError();
+ if (stakeAmount < RSETH_DEPOSIT_POOL.minAmountToDeposit()) revert MinAmountToDepositError();

Duplicate of #46

[massun-onibakuchi]

I don't think this meet DoS requirements.

[z3s]

For Dos issue to be H/M:
The issue causes locking of funds for users for more than a week.
The issue impacts the availability of time-sensitive functions.

Here user needs to deposit +1 wei.

issue is valid Low/Informational.

[drynooo]

Escalate

I think this should be valid.

This has the same impact as #75, the stake function will revert.

[sherlock-admin3]

Escalate

I think this should be valid.

This has the same impact as #75, the stake function will revert.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/napierfi/napier-uups-adapters/pull/17

[z3s]

I disagree with the escalation. It doesn't have the same impact as #75.
Here, Watson defined two scenarios that could cause a DoS:

1. stakeAmount is equal to minAmountToDeposit: user can send +1 wei
2. stakeLimit = minAmountToDeposit: is very unlikely and RSETH is Trusted.

Issue is valid Low/Informational.

[merlin-up]

@z3s, you mention that stakeLimit could equal 0, leading to the _stake function failing, whereas when stakeLimit = minAmountToDeposit, this occurrence is highly unlikely. Essentially, you're indicating that the current stake limit in RSETH_DEPOSIT_POOL = 0 carries a medium severity, but when the current stake limit in RSETH_DEPOSIT_POOL = 0.0001 ETH is highly unlikely. There's inconsistency in your logic, as both scenario can occur.

[z3s]

@z3s, you mention that stakeLimit could equal 0, leading to the _stake function failing, whereas when stakeLimit = minAmountToDeposit, this occurrence is highly unlikely. Essentially, you're indicating that the current stake limit in RSETH_DEPOSIT_POOL = 0 carries a medium severity, but when the current stake limit in RSETH_DEPOSIT_POOL = 0.0001 ETH is highly unlikely. There's inconsistency in your logic, as both scenario can occur.

After inspecting more, I think #75 could be low, too.
Reverting for any stakeAmount value lower than minAmountToDeposit, including 0, can be the correct behavior.
And the protocol can show the minimum amount users can stake in the UI.

[drynooo]

There are two things I don't know about the judging rules.

1. There seems to be no mention of possibility as a criterion in Sherlock’s rules?
2. Are problems that are easily avoidable through operation no longer a problem? Users don't know how to avoid this revert.

[merlin-up]

I would like to clarify that in prefundedDeposit, the stakeAmount does not depend on the user:

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

Therefore, if a user stakes an amount greater than minAmountToDeposit, there will be situations where the stakeAmount equals minAmountToDeposit.

In the previous audit, an issue #105 was accepted where if stakeAmount = 0, the _stake function would revert. (valid Medium severity)

In the current audit, an issue #64 was accepted, where if stakeAmount is sufficiently low (but not 0), the _stake function would revert. (valid Medium severity)

Currently, the situation where stakeAmount (which does not depend on the user's deposit amount) equals minAmountToDeposit (revert) is marked as Low/Informational, but I cannot understand why.

Reverting for any stakeAmount value lower than minAmountToDeposit, including 0, can be the correct behavior.
However, reverting when stakeAmount = minAmountToDeposit is incorrect behavior.

[pronobis4]

It's not the same as #75 but the end result will be the same, _stake will do the revert. However, in the case of 75, since the limit is zero, there should be no revert and return a zero.
In this case, because there is an error and the minimum deposit should go through, but it doesn't.

In my opinion there is an obvious problem with the core function of the protocol.
I agree with the escalation, it should be valid.

[WangSecurity]

I believe this report and #75 are not the same. #75 is about not being able to stake when the stake limit is 0, which is not expected and intended. This report is about not being able to stake when stake limit == minimum to stake. The question is in the report the watson shows the deposit function from LRTDepositPool contract, where you can deposit if deposit amount == minimum deposit. Firstly, I don't see it in the in-scope contracts and can't find it in the contest repo at all. Can someone forward me to it?

And the second question, is it the main assumption that you should be able to stake when stake amount == stake limit == minimum to stake because you're able to deposit when deposit amount == deposit limit? Or there are other reasons for it as well?

[drynooo]

For the first question, "the check for minAmountToDeposit in the LRTDepositPool smart contract:" mentioned above can be found here.

[merlin-up]

RSETH_DEPOSIT_POOL reverts the transaction if the stakeAmount is less than the minAmountToDeposit value. As stated in the report, RsETHAdapter incorrectly reverts the transaction when the stakeAmount is equal to the minAmountToDeposit.

1. The prefundedDeposit function calculates the stakeAmount to pass to the internal _stake function, meaning it does not depend on the amount of WETH specified by the user.
2. Even if prefundedDeposit passes a higher stakeAmount, if RSETH_DEPOSIT_POOL.getAssetCurrentLimit(Constants.ETH) = minAmountToDeposit, then the stakeAmount will be equal to the minAmountToDeposit.

[WangSecurity]

After additional discussion with the sponsor, indeed the _stake function shouldn't revert, even if the deposited amount is less than minAmountDeposit. Hence, I believe it's a valid bug with a medium severity.

Planning to accept the escalation and validate the issue with medium severity. The duplicates are the following: #59 and #82

[WangSecurity]

UPD: another two duplicated were flagged to me: #74 and #46. Since it's a new issue family, please flag other potential missing duplicates.

[pronobis4]

UPD: another two duplicated were flagged to me: #74 and #46. Since it's a new issue family, please flag other potential missing duplicates.

These don't look like duplicates.
In this case it's about incorrect condition checking, in these two it's about whether it should return zero or do a revert.
Regardless of whether it is revert or zero, the minimum deposit requirement will still be checked incorrectly.

[z3s]

After additional discussion with the sponsor, indeed the _stake function shouldn't revert, even if the deposited amount is less than minAmountDeposit. Hence, I believe it's a valid bug with a medium severity.

Planning to accept the escalation and validate the issue with medium severity. The duplicates are the following: #59 and #82

only #74 and #46 mentioned _stake function shouldn't revert, #54 and it's duplicates are saying it shouldn't revert only when stakeAmount == minAmountToDeposit. these are not dups. #54 and it's duplicates are either Low/Info or separate family.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.

[WangSecurity]

Thank you for pointing it out and indeed, #74 and #46 are slightly different and correctly identify a case when the there shouldn't be revert at any value, even if it's smaller than the minimum to deposit. And I believe these two has to be be valid medium severity.

Regarding #54 and duplicates. They correctly identify the correct scenario that the staking shouldn't revert when stake amount == minimum amount to deposit.

It's a very hard and borderline case. Both find the way how staking will revert due to incorrect checks and the problem is in the same line with #74. The problem is in different conditions. The fix of #74 will also fixes this issue, but not vice versa. So here's my analysis and decision:

There's a core vulnerability -> the staking reverts when the staking amount <= minimum to deposit, which shouldn't happen and users should be able to stake any amount at any time. The first group (#74 and #46) correctly identify that there shouln't be a revert at any value. The second group (this and dups) correctly identify the case when it reverts at staking amount == minimum to deposit, which shouldn't happen.

On the one hand, they both identified the core vulnerability that stake shouldn't revert at this line. On the other hand, one set of Watsons understood the problem is that the protocol should allow to stake any amount, when the second set of Watsons didn't understand. Hence, I see four outcomes:

1. zzykxx - Kelp adapter won't allow users to deposit in some situations #74 and no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos #46 are valid, and this report and it duplicates are valid duplicates of zzykxx - Kelp adapter won't allow users to deposit in some situations #74 and no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos #46.
2. zzykxx - Kelp adapter won't allow users to deposit in some situations #74 and no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos #46 are valid, and this report and it duplicates are invalid duplicates of zzykxx - Kelp adapter won't allow users to deposit in some situations #74 and no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos #46, cause they don't show the Watsons actually understood the protocol.
3. zzykxx - Kelp adapter won't allow users to deposit in some situations #74 and no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos #46 are valid and merlin - DOS vulnerability in the _stake function in RsETHAdapter.sol #54 and its duplicates are a different valid family.
4. zzykxx - Kelp adapter won't allow users to deposit in some situations #74 and no - Checking RSETH_DEPOSIT_POOL.minAmountToDeposit() in RsETHAdapter::_stake() causes Dos #46 are valid and merlin - DOS vulnerability in the _stake function in RsETHAdapter.sol #54 and its duplicates are a different invalid family (low severity).

I believe the fair outcome the first one, cause #54 and its duplicates correctly identify the case that the code shouldn't revert when stake amount == minimum deposit, when #74 and #46 take it further and identify that it shouldn't revert when staking amount is both < and == to minimum deposit. I believe that #54 and its duplicates are sufficient enough to say that they identify the vulnerability, yet the fix is incorrect. Hope for your understanding.

Planning to accept the escalation, make a new medium issue family with #46 as main and #74, #54, #59 and #82 as duplicates.

[Evert0x]

Result:
Medium
Duplicate of #46

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Scorpiondeng: accepted