Liquid Staking

Summary

executeQueuedWithdrawals() takes the StakingPool.sol funds that can be withdrawn + totalQueued tokens in exchange for LST, but in executeQueuedWithdrawals(), nowhere is the accounting for the totalQueued to be seen, which will give PriorityPool.sol the false impression that there are LINK tokens being queued, but this won't be possible.

Vulnerability Details

The vulnerability starts in WithdrawalPool::performUpkeep() in the calculation for withdrawing funds,which is in PriorityPool::canWithdraw() and this is the first part:

The account's personal amount and the totalQueued and it takes the smaller value https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/priorityPool/PriorityPool.sol#L215-L217, which will always result in zero because there is no way for the withdraw pool's address to have a key value pair in the mapping of accountQueuedTokens .

But the second part is the interesting one -> we make the comparison between the LST in WithdrawalPool.sol and StakingPool::canWithdraw() + totalQueued - canUnqueue (canUnqueue will be zero, I've explained why in the above section)

Let's say we have 100 LST (totalQueuedShareWithdrawals) in WithdrawalPool.sol and we have in 75 LINK tokens available in StakingPool::canWithdraw() + 25 LINK tokens from totalQueued.

Due to that, stLINKCanWithdraw will be 100 and stLINKCanWithdraw + canUnqueue is going to be equal to 100. Remember that canUnqueue will always be zero, that's why.

Let's see why is this a problem in my attack scenario:

1. We have a user which calls WithdrawalPool::performUpkeep()

2. The calculation stated above will return 100 LINK tokens (and the external conditions to be the same or similar are very likely) -> https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/priorityPool/WithdrawalPool.sol#L349

3. This comparison is going to result in 100 tokens as well -> https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/priorityPool/WithdrawalPool.sol#L359

4. executeQueuedWithdraw() will be called with _amount equal to 100, we'll call StakingPool::withdraw() which will send us the 75 LINK Tokens to the PriorityPool.sol _(remember, __amount is equal to 100 from the 25 tokens from totalQueued + 75 tokens from the StakingPool.sol), which will result in sending the 100 LINK tokens to WithdrawalPool.sol-> https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/priorityPool/PriorityPool.sol#L530 , without accounting it in totalQueued or somewhere in the PriorityPool.sol (which account tokens should exactly be used in the whole operation, etc.).

Impact

This will render all of the functions dependent on totalQueued unusable to some extend in functionality like: not having the possibility to unqueue or withdraw tokens that are queued.

I will provide a non-exclusive list to show you some

1. Let's say the attack scenario has happened, Alice wants to unqueue her tokens using PriorityPool::unqueueTokens() but the function will revert because there are no LINK tokens left in the PriorityPool.sol after the specified scenario.

2. The first point above can be repeated basically the same with PriorityPool::withdraw() with _shouldUnqueue being evaluated to true.

3. Future withdrawals using the same steps as in the attack scenario will not be possible due to totalQueued being of a higher value than the actual tokens queued in the contract.

Tools Used

Manual Review

Recommendations

Account for totalQueued when running executeQueuedWithdrawal