code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

If gas prices is too high, then no one will call finalizeUserWithdrawRequest() because the function will loop 50 times minimum and will cost a lot of money. Although max loops can be changed by the manager, the user may just wait for other's to finalize the withdrawal from him, which leads to a process of inaction.

Proof of Concept

UserWithdrawalManager#finalizeUserWithdrawalRequest is extremely dependent on the goodwill of users.

Let's say if there is a lot of withdraw requests happening and nextRequestId is at the thousands (2000). However, nextRequestIdToFinalize has not caught up and is still at the hundreds (100). maxRequestIdToFinalize will be calculated as such:

uint256 maxRequestIdToFinalize = Math.min(nextRequestId, nextRequestIdToFinalize + finalizationBatchLimit) - 1;
uint256 maxRequestIdToFinalize = Math.min(2000, 100 + 50) - 1;
uint256 maxRequestIdToFinalize = 150 - 1 = 149;

There will always be 50 loops if nextRequestId > nextRequestIdToFinalize + finalizationBatchLimit unless the limit is changed.

for (requestId = nextRequestIdToFinalize; requestId <= maxRequestIdToFinalize; )
for (requestId = 100 ; requestId <= 149; )

Code Snippet:

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

If the gas prices is too high, then no one will call finalizeUserWithdrawalRequest(), especially if their request is not within 50 places from nextRequestIdToFinalize. Even if gas prices is low, it would take the goodwill of someone to call finalizeUserWithdrawalRequest(). Also, if the person's withdraw request is the next up to be finalized, he has to call for 49 different people, which is pretty undesirable. Everybody would probably just wait for someone else to help them finalize their withdrawal.

Setting as Medium:

The user cannot control how many loops to go through, and its set by the manager. Users may just wait on others to help them finalize their withdrawals.
If the manager is not actively monitoring the situation (gas prices, spikes, gas limits etc) and changing the number of max loops accordingly, then the disparity between nextRequestId and nextRequestIdToFinalize will grow bigger, which is undesirable for the protocol because people have to wait an extremely long time just to get their funds out.
If there is too many loops, it may result in OOG error.

Tools Used

Manual Review

Recommended Mitigation Steps

It's noted that the finalizationBatchLimit amount can be updated by the manager through the updateFinalizationBatchLimit, but it may not be enough because of the aforementioned medium problems. It will be good if the user can input the number of loops he wants instead and not be dependent on the manager changing the variable.