Lines of code

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&gt;

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&gt;

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&gt;

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&gt;

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

---

The text was updated successfully, but these errors were encountered:

All reactions