Summary
checkUpkeep function misses to validate a limit on the number of withdrawals processed.
And WithdrawalPool contract´s _finalizeWithdrawals function may revert due to missing to validate the max. number of withdrawals.
If many withdrawal requests accumulate, the gas required to process them all can exceed the block gas limit, causing the transaction to revert and halting the withdrawal process for users.
Vulnerability Details
When users request withdrawals that exceed the WithdrawalPool's available balance, their requests are queued in the queuedWithdrawals array.
The performUpkeep function is designed to process these queued withdrawals by calling _finalizeWithdrawals. And this one is triggered by querying checkUpKeep off-chain.
However checkUpkeep does not account for queue size:
Contract: WithdrawalPool.sol
333: function checkUpkeep(bytes calldata) external view returns (bool, bytes memory) {
334: if (
335: _getStakeByShares(totalQueuedShareWithdrawals) != 0 &&
336: priorityPool.canWithdraw(address(this), 0) != 0 &&
337: block.timestamp > timeOfLastWithdrawal + minTimeBetweenWithdrawals
338: ) {
339: return (true, "");
340: }
341: return (false, "");
342: }
So if above condition returns true, the CRON job can call performUpkeep to execute them and calls _finalizeWithdrawals at the end:
Contract: WithdrawalPool.sol
348: function performUpkeep(bytes calldata _performData) external {
349: uint256 canWithdraw = priorityPool.canWithdraw(address(this), 0);
350: uint256 totalQueued = _getStakeByShares(totalQueuedShareWithdrawals);
351: if (
352: totalQueued == 0 ||
353: canWithdraw == 0 ||
354: block.timestamp <= timeOfLastWithdrawal + minTimeBetweenWithdrawals
355: ) revert NoUpkeepNeeded();
356:
357: timeOfLastWithdrawal = uint64(block.timestamp);
358:
359: uint256 toWithdraw = totalQueued > canWithdraw ? canWithdraw : totalQueued;
360: bytes[] memory data = abi.decode(_performData, (bytes[]));
361:
362: priorityPool.executeQueuedWithdrawals(toWithdraw, data);
363: >>> _finalizeWithdrawals(toWithdraw);
364: }
However, _finalizeWithdrawals iterates over the entire queuedWithdrawals array without any limit, which can lead to excessive gas consumption if the array is large.
Contract: WithdrawalPool.sol
422: function _finalizeWithdrawals(uint256 _amount) internal {
423: uint256 sharesToWithdraw = _getSharesByStake(_amount);
424: uint256 numWithdrawals = queuedWithdrawals.length;
425:
426: totalQueuedShareWithdrawals -= sharesToWithdraw;
427:
428: for (uint256 i = indexOfNextWithdrawal; i < numWithdrawals; ++i) {
429: uint256 sharesRemaining = queuedWithdrawals[i].sharesRemaining;
430:
431: if (sharesRemaining < sharesToWithdraw) {
432:
433: sharesToWithdraw -= sharesRemaining;
434: continue;
435: }
436:
437: if (sharesRemaining > sharesToWithdraw) {
438:
439: queuedWithdrawals[i] = Withdrawal(
440: uint128(sharesRemaining - sharesToWithdraw),
441: uint128(
442: queuedWithdrawals[i].partiallyWithdrawableAmount +
443: _getStakeByShares(sharesToWithdraw)
444: )
445: );
446: indexOfNextWithdrawal = i;
447: withdrawalBatches.push(
448: WithdrawalBatch(uint128(i - 1), uint128(_getStakeByShares(1 ether)))
449: );
450: } else {
451:
452: indexOfNextWithdrawal = i + 1;
453: withdrawalBatches.push(
454: WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
455: );
456: }
457:
458: sharesToWithdraw = 0;
459: break;
460: }
461:
462:
463: assert(sharesToWithdraw == 0);
464:
465: emit WithdrawalsFinalized(_amount);
466: }
Impact
DOS
Tools Used
Manual Review
Recommendations
Modify the checkUpkeep for checking a threshold qeueu size.
