User with large stacked ETH can deny other stacker from withdrawing.

[code423n4]

Lines of code

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

Vulnerability details

Description

The withdraw flow of Stader splitted in two steps, first the user has to requestWithdraw by passing his owned ETHx amount which add a new record to userWithdrawRequests[nextRequestId], second, finalizeUserWithdrawalRequest got called by any user to finalize the recorded requested in order it's got added, so requestID 1 should be finalized first and then 2 and so on.

The function finalizeUserWithdrawalRequest loop through each requestID and fetch the userWithdrawInfo.ethExpected;, then it calculate the minEThRequiredToFinalizeRequest and it checks if the minEThRequiredToFinalizeRequest + minEThRequiredToFinalizeRequest > SSPM ETH balance, the loop will break. The issue is that the function is designed to process requestIDs in order, if 1st request failed the loop break and no more iteration over other requestIds..

Hence, if the requestID 1 for example initiated by Alice who deposited large amount, the value of her minEThRequiredToFinalizeRequest could be more than the available balance of the poolManager so any withdraw requests will be temporary frozen even if the available balance can simply cover the other withdraws on the userWithdrawRequests mapping.

Impact

Either intentionally or unintentionally, attacker who deposited large amount can call requestWithdraw to lock other users funds until PoolManager receive large deposit either from other contracts on the system as part of the reward flow or another stacker in order to cover the funds requested by the attacker.

Proof of Concept

1. Let's look at the implementation of requestWithdraw https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L95-L110

   function requestWithdraw(uint256 _ethXAmount, address _owner) external override whenNotPaused returns (uint256) {
       if (_owner == address(0)) revert ZeroAddressReceived();
       uint256 assets = IStaderStakePoolManager(staderConfig.getStakePoolManager()).previewWithdraw(_ethXAmount);
       if (assets < staderConfig.getMinWithdrawAmount() || assets > staderConfig.getMaxWithdrawAmount()) {
           revert InvalidWithdrawAmount();
       }
       if (requestIdsByUserAddress[_owner].length + 1 > maxNonRedeemedUserRequestCount) {
           revert MaxLimitOnWithdrawRequestCountReached();
       }
       IERC20Upgradeable(staderConfig.getETHxToken()).safeTransferFrom(msg.sender, (address(this)), _ethXAmount);
       ethRequestedForWithdraw += assets;
       userWithdrawRequests[nextRequestId] = UserWithdrawInfo(payable(_owner), _ethXAmount, assets, 0, block.number);
       requestIdsByUserAddress[_owner].push(nextRequestId);
       emit WithdrawRequestReceived(msg.sender, _owner, nextRequestId, _ethXAmount, assets);
       nextRequestId++;
       return nextRequestId - 1;
   }

2. The function calculate the withdrawn ETH based on the passed _ethXAmount using previewWithdraw

uint256 assets = IStaderStakePoolManager(staderConfig.getStakePoolManager()).previewWithdraw(_ethXAmount);

3. Then it transfer the ETHx amount from the user to the contract, and then add new record into userWithdrawRequests mapping https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#LL106C9-L106C119

   userWithdrawRequests[nextRequestId] = UserWithdrawInfo(payable(_owner), _ethXAmount, assets, 0, block.number);

4. Supposed the following:

• Alice called this function first and the assets (amount of ETH to be withdrawn) is 100, now Alice request is 1
• Bob call the function == assets is 5 ETH, Bob request is 2
• Mike call the function == assets is 5 ETH, Mike request is 3

5. Later on, when finalizeUserWithdrawalRequest get called the function fetch the current balance of the pool manager at L131, let's say current balance is 30 ETH. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L131

function finalizeUserWithdrawalRequest() external override nonReentrant whenNotPaused {
        if (IStaderOracle(staderConfig.getStaderOracle()).safeMode()) {
            revert UnsupportedOperationInSafeMode();
        }
        if (!IStaderStakePoolManager(staderConfig.getStakePoolManager()).isVaultHealthy()) {
            revert ProtocolNotHealthy();
        }
        address poolManager = staderConfig.getStakePoolManager();
        uint256 DECIMALS = staderConfig.getDecimals();
        uint256 exchangeRate = IStaderStakePoolManager(poolManager).getExchangeRate();
        uint256 maxRequestIdToFinalize = Math.min(nextRequestId, nextRequestIdToFinalize + finalizationBatchLimit) - 1;
        uint256 lockedEthXToBurn;
        uint256 ethToSendToFinalizeRequest;
        uint256 requestId;
        uint256 pooledETH = poolManager.balance;
        for (requestId = nextRequestIdToFinalize; requestId <= maxRequestIdToFinalize; ) {
            UserWithdrawInfo memory userWithdrawInfo = userWithdrawRequests[requestId];
            uint256 requiredEth = userWithdrawInfo.ethExpected;
            uint256 lockedEthX = userWithdrawInfo.ethXAmount;
            uint256 minEThRequiredToFinalizeRequest = Math.min(requiredEth, (lockedEthX * exchangeRate) / DECIMALS);
            if (
                (ethToSendToFinalizeRequest + minEThRequiredToFinalizeRequest > pooledETH) ||
                (userWithdrawInfo.requestBlock + staderConfig.getMinBlockDelayToFinalizeWithdrawRequest() >
                    block.number)
            ) {
                break;
            }
            userWithdrawRequests[requestId].ethFinalized = minEThRequiredToFinalizeRequest;
            ethRequestedForWithdraw -= requiredEth;
            lockedEthXToBurn += lockedEthX;
            ethToSendToFinalizeRequest += minEThRequiredToFinalizeRequest;
            unchecked {
                ++requestId;
            }
        }
        // at here, upto (requestId-1) is finalized
        if (requestId > nextRequestIdToFinalize) {
            nextRequestIdToFinalize = requestId;
            ETHx(staderConfig.getETHxToken()).burnFrom(address(this), lockedEthXToBurn);
            IStaderStakePoolManager(poolManager).transferETHToUserWithdrawManager(ethToSendToFinalizeRequest);
            emit FinalizedWithdrawRequest(requestId);
        }
    }

6. Then the function run a for loop starts from 1 since nextRequestIdToFinalize initialized to 1 and this is the first call. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L132

for (requestId = nextRequestIdToFinalize; requestId <= maxRequestIdToFinalize; ) {
            UserWithdrawInfo memory userWithdrawInfo = userWithdrawRequests[requestId];
            uint256 requiredEth = userWithdrawInfo.ethExpected;
            uint256 lockedEthX = userWithdrawInfo.ethXAmount;
            uint256 minEThRequiredToFinalizeRequest = Math.min(requiredEth, (lockedEthX * exchangeRate) / DECIMALS);
            if (
                (ethToSendToFinalizeRequest + minEThRequiredToFinalizeRequest > pooledETH) ||
                (userWithdrawInfo.requestBlock + staderConfig.getMinBlockDelayToFinalizeWithdrawRequest() >
                    block.number)
            ) {
                break;
            }

7. Inside the loop the function fetch the withdrawRequest at requestId 1 which is Alice object.

UserWithdrawInfo memory userWithdrawInfo = userWithdrawRequests[requestId];

8. At L134, the function get the recorded ETH which is 100 ETH

9. At L136, the function select the smallest value between the requiredEth and (lockedEthX * exchangeRate) / DECIMALS and assign it to minEThRequiredToFinalizeRequest.

The second argument calculate the amount of ETH in exchnage for the LockedETHx at the time of execution which will not affect the final ETH amount too much, could be a little less.

10. At L137 it checks if the (minEThRequiredToFinalizeRequest which is 100 ETH + ethToSendToFinalizeRequest which is 0 as this is the first iteration) bigger than the pooledETH since Alice ETH requested is 100 and current balance of pool manager is 30 ETH, the function break the loop and no more iteration.

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

11. The IF condition at L153 will return false as requestID still 1 so the function end without finalizing the withdraw. In this case, Bob and Mike who just requested 5 ETH each which can be covered by the poolManager will not be able to withdraw. The withdraw flow will be broken for all future requests.

Tools Used

Manual

Recommended Mitigation Steps

Allow the user to call finalizeUserWithdrawalRequest passing the requestID returned from requestWithdraw function as an argument so you don't have to execute requests in order. This will require some changes to the function implementation as well.

Assessed type

DoS

[manoj9april]

Intentionally designed to cater FIFO requests.

[c4-sponsor]

manoj9april marked the issue as sponsor disputed

[Picodes]

I am not sure to understand why the suggested mitigation would be "fairer" and why FIFO is not the best way to go here. In your scenario why is it an issue to wait for Alice's request to be filled before filling Bob and Mike? Assuming the code prioritizes Bob and Mike then Alice would be the one who's facing a DoS

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[Co0nan]

Hi @Picodes, I really appreciate leaving feedback when closing the report.

I believe that following the FIFO concept during withdrawal will lead to DOS of the function for the following two reasons:

1. The function requestWithdraw doesn't enforce the max amount a user can request at once, a malicious attacker can request 1M ETH at a time and his request will be recorded with such a large amount.
2. There is no guarantee that the contract can secure such a large amount in a short time, the SSPM contract only gets funded if there is a new deposit and when collecting rewards.

why is it an issue to wait for Alice's request to be filled before filling Bob and Mike?

This is because Bob and Mike will have to wait for a long time may be weeks until the SSPM contract holds 1000 ETH as requested by a malicious attacker to get their ETH withdrawn.

Here is the attack scenario:

1. Attacker deposit 10000 ETH
2. Bob, and Mike deposit 5 ETH each.
3. Alice request to withdraw passing her full balance = 10000 ETH
4. SSPM contract collects some rewards and it now holds 10 ETH.
5. Bob and Mike request to withdraw
6. Even though the contract holds enough balance for Bob and Mike, due to the large amount requested by Alice the function will be in DoS state until it's get funded by 10000 ETH.

This attack will not cost the user anything, actually, he can earn rewards while he is waiting for his request to be finalized.

Assuming the code prioritizes Bob and Mike then Alice would be the one who's facing a DoS

Alice is the attacker here so no users will be impacted unless the attacker himself. Also, even if Alice is a normal user, she can call deleteRequestId and split her amount to a small value if needed.

I am not sure to understand why the suggested mitigation would be "fairer"

My suggestion is to allow the user to finalize their own requests directly, without having to wait for another withdrawer who is requesting large amounts. I want to suggest also enforcing a max amount users can request at a time. With this mitigation, no one will be able to DOS other users.

[Picodes]

Thanks for taking the time to give additional explanations @Co0nan.

On my end, I still think this is a suggestion for another design but not a security issue:

• Alice is the attacker in your scenario but could as well be just a normal user. Assuming the withdrawal process is not FIFO, it's very likely that she will just never be able to withdraw 10000 ETH at once. Furthermore, if users can finalize their own requests directly then there is a significant risk that "normal" users will just always be frontran by more advanced players that are able to use MEV to finalize their withdrawal requests as soon as there are enough funds available.
• I think deleteRequestId is internal and can't be called

[Co0nan]

@Picodes,

Totally respect your discussion, but I can't imagine how this can't be a DoS issue. I think it has a similar impact as #140 but with a different attack vector. As a user, why do I have to wait for a long time to get my ETH back cause an attacker requested a large amount? I mentioned 10k ETH but this can be more than that which stacks all funds for months, Should ain't the system secure my funds?

In my scenario Alice is the attacker that will request a large amount, this is clearly DoS another user.

if users can finalize their own requests directly then there is a significant risk that "normal" users will just always be frontran by more advanced players that are able to use MEV to finalize their withdrawal requests as soon as there are enough funds available.

I agree, I gave another suggestion that we check for a max value so combined with the currently implemented function regarding finalizationBatchLimit, this will be more applicable.

Would appreciate taking a more closely look at the possible attack here based on this design, and let me know your thoughts, if you believe this is not a DoS issue I will remain with your discussion. Thanks!

[Picodes]

@Co0nan my decision is final on this one. In your scenario Alice is the "attacker" but nothing differentiates her from the average user. She staked these ETH, which will be frozen during the attack scenario, so this isn't free for her either. Breaking the queue system is not a convincing solution as it leads to more important issues. Finally, I think -but haven't digged- that you could perform this attack at the validator exit queue level anyway if you had a lot of funds

Note furthermore that there are already withdrawal limits in the system.

[Co0nan]

Thanks @Picodes, you are correct. Appreciate your final discussion. I also discussed this with @sanjay-staderlabs and released that the system can exit validators to cover the requested amounts, so there will not be a DoS for a long time as I stated.