Lack of slippage protection when withdrawing order in UserWithdrawalManager

[code423n4]

Lines of code

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

Vulnerability details

Inside UserWithdrawalManager.sol, users can make a request to withdraw a specific amount of ethX from the vault by calling requestWithdraw.

    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();
        }
        //@audit lock in _ethXAmount ETHx
        IERC20Upgradeable(staderConfig.getETHxToken()).safeTransferFrom(msg.sender, (address(this)), _ethXAmount);
        ethRequestedForWithdraw += assets;

        //@audit create the UserWithdrawInfo struct and add it to the userWithdrawRequests map
        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;
    }

The request are finalized by calling finalizeUserWithdrawalRequest. This function finalizes the submitted requests in batches. The issue is that finalizeUserWithdrawalRequest calls getExchangeRate to get the exchange rate between ETH and ETHx. There could be a larger than expected value between when the user first submitted the request and when the request is processed. This is further an issue since there is a minimum of 600 blocks that must pass before the request is processed.

  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();

        //@audit here the exchange rate is calculated from getExchangeRate
        uint256 exchangeRate = IStaderStakePoolManager(poolManager).getExchangeRate();
        uint256 maxRequestIdToFinalize = Math.min(nextRequestId, nextRequestIdToFinalize + finalizationBatchLimit) - 1;
        uint256 lockedEthXToBurn;
        uint256 ethToSendToFinalizeRequest;
        uint256 requestId;
        uint256 pooledETH = poolManager.balance;
        //@audit loop through the batch of requests
        for (requestId = nextRequestIdToFinalize; requestId <= maxRequestIdToFinalize; ) {
            UserWithdrawInfo memory userWithdrawInfo = userWithdrawRequests[requestId];
            uint256 requiredEth = userWithdrawInfo.ethExpected;
            uint256 lockedEthX = userWithdrawInfo.ethXAmount;

            //@audit eth to be sent is calculated
            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;
            }
        }
        //@audit mark as finalized
        //..

    }

Impact

The user may suffer unexpected losses when withdrawing ETH. This is further exacerbated by the minimumm 600 block wait time between request submission and finalization.

Proof of Concept

1. User submits a request to withdraw 10e18 ethX through requestWithdraw in order to receive an expected 10 eth which is the number of shares that is calculated by previewWithdraw the UserWithdrawInfo struct is created with values

   UserWithdrawInfo {
   owner: user 
   ethXAmount: 10e18
   ethExpected: 10e18
   ethFinalized: 0
   requestBlock: block.number
   }

2. finalizeUserWithdrawalRequest is then called and the minimum of ethExpected and ethXAmount*exchangeRate/DECIMALS is calculated. If the exchange rate is currently less than the user expects then the user will receive less than the ethExpected figure

 uint256 minEThRequiredToFinalizeRequest = Math.min(requiredEth, (lockedEthX * exchangeRate) / DECIMALS);

if the exchange rate was .90*1e18 then the final ethereum received will be 9e18 which is 1e18 ETH less than expected.

minEthRequiredToFinalizeRequest = min(10e18, (10e18*.90e18)/1e18)
 = min(10e18, 9e18)
 = 9e18

Tools Used

VIM

Recommended Mitigation Steps

Withdrawers should have a way to specify the amount of slippage that is accepted. This can be done inside requestWithdraw when users first submit their request. If the slippage is not within the accepted range then consider adding functionality inside finalizeUserWithdrawalRequest that cancels their order and refunds their deposited ETHx

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #222

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)