Liquid Staking

Summary

withdraw function deletes the queuedWithdrawals entry entirely if the withdrawal is fully finalized (withdrawalId <= batch.indexOfLastWithdrawal). This effectively decreases totalQueuedShareWithdrawals when it shouldn't, because those shares were already accounted for in _finalizeWithdrawals when the withdrawal was finalized by deposit. WithdrawPool.sol# https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/WithdrawalPool.sol#L270-L295

In the if block where withdrawalId <= batch.indexOfLastWithdrawal. It incorrectly deletes the entire queuedWithdrawals entry using delete queuedWithdrawals[withdrawalId], which has the side effect of decreasing the totalQueuedShareWithdrawals value when it shouldn't, because those shares were already accounted for when the withdrawal was finalized.

Vulnerability Details

In the withdraw function, when a withdrawal is fully finalized (withdrawalId <= batch.indexOfLastWithdrawal), the entire queuedWithdrawals entry is incorrectly deleted using delete queuedWithdrawals[withdrawalId]. This deletion decreases the totalQueuedShareWithdrawals value, which keeps track of the total queued LST shares.

However, the shares for finalized withdrawals were already accounted for and deducted from totalQueuedShareWithdrawals when the withdrawal was finalized by the deposit function calling _finalizeWithdrawals. Therefore, withdraw should not modify totalQueuedShareWithdrawals for finalized withdrawals.

This bug connects to the _finalizeWithdrawals internal function, which is called by deposit to finalize withdrawals and correctly updates totalQueuedShareWithdrawals.

Proof of Concept

1. Queue a withdrawal by calling queueWithdrawal(account1, 1000). This will increase totalQueuedShareWithdrawals by 1000.

2. Finalize the withdrawal by calling deposit(1000). This will call _finalizeWithdrawals(1000), which will decrease totalQueuedShareWithdrawals by 1000.

3. Call withdraw([withdrawalId], [batchId]) for the finalized withdrawal.

4. Check that totalQueuedShareWithdrawals has decreased from the original value (it should be 0 after steps 1-3, but the bug causes it to go to -1000 after withdraw).

Impact

The bug causes totalQueuedShareWithdrawals to become inconsistent and no longer accurately reflect the total queued LST shares.

Users may be indirectly affected if this inconsistency causes any malfunctions or incorrect behavior in other parts of the system that rely on totalQueuedShareWithdrawals being accurate.

The bug will occur whenever a user withdraws a finalized withdrawal (i.e., whenever withdraw is called with a withdrawalId that has been finalized by a previous deposit). So it's quite likely to happen in normal usage of the protocol, as withdrawing finalized withdrawals is an expected regular operation.

Tools Used

Manual Review

Recommendations

The withdraw should only delete the partiallyWithdrawableAmount, similar to the else case, rather than the entire queuedWithdrawals entry. This ensures totalQueuedShareWithdrawals stays consistent.