search

code-423n4 / 2023-06-stader-findings

1 stars 1 forks source link

Withdrawal system can be subject to DoS with a large amount of small requests #221

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L132-L151

Vulnerability details

Impact

An attacker can create a large number of small withdrawal requests in UserWithdrawalManager, which implements a push architecture for withdrawals.

This can cause a loss for other users, especially if the attacker doesn't finalize their withdrawals, as they will be forced to pay for the finalization if they want to receive their own funds.

Proof of Concept

The withdrawal system works in the following way:

  1. A user requests a withdrawal by calling requestWithdraw
  2. The request is put in a global queue to be finalized
  3. After some time, the request can be finalized by calling finalizeUserWithdrawalRequest
  4. The request can be finalized only if the prior requests are finalized, including other users' requests
  5. There is a maximum of finalizationBatchLimit requests that can be finalized with each request:
    uint256 maxRequestIdToFinalize = Math.min(nextRequestId, nextRequestIdToFinalize + finalizationBatchLimit) - 1; 
    ...
    for (requestId = nextRequestIdToFinalize; requestId <= maxRequestIdToFinalize; )

An attacker can simply split their withdrawal into a lot of small requestWithdraw that will clog this system, resulting in losses for other users, as they are forced to finalize the attacker's withdrawal requests.

Tools Used

Manual review

Recommended Mitigation Steps

Consider refactoring the system from a push to a pull system, where each user can finalize only their own requests.

Assessed type

Timing

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

manoj9april commented 1 year ago

We have a limit on maxNonRedeemedUserRequestCount, which makes sure that attacker can't request more than the limit https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#LL101C58-L101C88

c4-sponsor commented 1 year ago

manoj9april marked the issue as sponsor disputed

Picodes commented 1 year ago

@manoj9april to me the issue is still valid as DoS can still happen at the level of maxNonRedeemedUserRequestCount. See for example the duplicates https://github.com/code-423n4/2022-06-stader-findings/issues/224 or https://github.com/code-423n4/2022-06-stader-findings/issues/140.

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes marked the issue as partial-50

c4-judge commented 1 year ago

Picodes marked issue #140 as primary and marked this issue as a duplicate of 140

DadeKuma commented 1 year ago

I believe this is not a duplicate of #140 (note, I've submitted both issues, see #224)

Let me explain:

Picodes commented 1 year ago

@DadeKuma thanks for your comment. My initial reasoning was that both issues show that the whole idea of using a queue is very prone to griefing attacks, on both a user level as shown by #140, and at a protocol level as shown by #224.

Furthermore, I believe the fix you are proposing here is basically to stop using a FIFO queue to treat withdrawal requests. As long as there is no FIFO, there is no need to limit the number of requests per user, so the other issue is also fixed.

DadeKuma commented 1 year ago

@Picodes The main issue here is using a global/shared queue instead of a user-based one (the queue should not be removed, it can make sense, for example, to avoid bank runs), so the solution here was to have a queue for each user instead.

This was the solution I proposed:

Consider refactoring the system from a push to a pull system, where each user can finalize only their own requests.

This eliminates the problem of anyone being able to clog the global queue.

The issues are essentially different between the two cases because, even with the previous solution, an attacker can still clog the withdrawal queue of another user, as there are no restrictions on who can request a withdrawal (described in #140).

Furthermore, fixing #140 does not resolve this issue, as it remains vulnerable to the attack described in my previous comment.

Picodes commented 1 year ago

@DadeKuma sorry I am not sure to understand. How is a user-based queue useful to protect from bank runs? Unless I am missing something a user-based queue is like no queue at all. It seems to me that either you use a global queue, either it's useless. This being said, if I were the sponsor, I'd keep the current system and acknowledge the fact that a global queue can be clogged

DadeKuma commented 1 year ago

@Picodes

How is a user-based queue useful to protect from bank runs? Unless I am missing something a user-based queue is like no queue at all.

By limiting the amount of how much a user can withdraw at once. When requesting a withdrawal, there is a max amount:

if (assets < staderConfig.getMinWithdrawAmount() || assets > staderConfig.getMaxWithdrawAmount()) {
    revert InvalidWithdrawAmount();
}

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L98-L100

The user can't finalize their requests until a minimum delay has passed:

if (
    (ethToSendToFinalizeRequest + minEThRequiredToFinalizeRequest > pooledETH) ||
    (userWithdrawInfo.requestBlock + staderConfig.getMinBlockDelayToFinalizeWithdrawRequest() >
        block.number)
) {
    break;
}

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L137-L143

So in this specific period of time, the user is limited to withdrawing a maximum of maxNonRedeemedUserRequestCount * staderConfig.getMaxWithdrawAmount(), instead of everything.

The sponsor can monitor the number of withdrawal requests, and may decide to pause the contract before they are finalized, to prevent bank runs:

function finalizeUserWithdrawalRequest() external override nonReentrant whenNotPaused {

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L117

This won't be possible without the queue, as a max amount would not make sense (a user could simply send a single transaction with N withdrawal requests), so users would be able to withdraw the funds instantly, potentially causing a bank run.

Picodes commented 1 year ago

So here you're saying that the bank-run mitigation is the delay + the limit, not really the "queue", right? And you're saying that the bank-run scenario can be mitigated as the admin can pause.

I disagree with this take and the fact that this is a better solution. Here, the global queue is made to preserve the fairness of the system: the first to request a withdrawal will be processed first.

As long as you remove this global queue and you allow individual users to finalize their own requests, then you will for sure have front-running / bank-run issues because when the withdrawal flow is higher than what the eth exit queue can process, then non-advanced users will always be front-ran by more advanced players and they will effectively be facing a DoS has there will never be enough funds in the system for them to finalize their withdrawals.

So in my opinion all the possible safeguards (min / max withdrawal size, max withdrawal queue per user, max batch size) have already been implemented by stader's team and this issue is of NC severity

c4-judge commented 1 year ago

Picodes marked the issue as not a duplicate

Picodes commented 1 year ago

@sanjay-staderlabs tagging your for visibility

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)