Summary
The PriorityPool::withdraw function transfers tokens from the StakingPool to the PriorityPool. However, after transferring the remaining tokens, those tokens are still being queued for withdrawal. This leads to inconsistent user experience, where users expect immediate withdrawals but instead see their tokens queued. Users may get confused as they witness tokens both transferred and queued, causing delays in accessing their funds.
Vulnerability Detail
When a user requests to withdraw tokens, the function first tries to unqueue as many tokens as possible, then proceeds to transfer any remaining tokens from the StakingPool to the PriorityPool contract. However, after this transfer occurs, the PriorityPool contract still proceeds to queue the transferred tokens for withdrawal, which leads to a confusing and inefficient behavior. Users might expect these tokens to be immediately available but instead find them queued, which can delay their ability to access the funds.
This is especially problematic because it contradicts the intent of the transfer, which should ideally make the tokens available immediately. Instead, the system treats the transferred tokens as if they still need to go through the withdrawal queue, causing delays and potential frustration for the user.
Impact
The main impact is an inconsistent and confusing user experience. Users may believe their tokens have been successfully withdrawn when, in fact, the tokens are queued, leading to unnecessary delays in accessing their funds. This could also result in a lack of trust in the system if users encounter repeated delays.
Code Snippet
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/PriorityPool.sol#L271
function withdraw(
uint256 _amountToWithdraw,
uint256 _amount,
uint256 _sharesAmount,
bytes32[] calldata _merkleProof,
bool _shouldUnqueue,
bool _shouldQueueWithdrawal
) external {
if (_amountToWithdraw == 0) revert InvalidAmount();
uint256 toWithdraw = _amountToWithdraw;
address account = msg.sender;
if (_shouldUnqueue == true) {
_requireNotPaused();
if (_merkleProof.length != 0) {
bytes32 node = keccak256(
bytes.concat(keccak256(abi.encode(account, _amount, _sharesAmount)))
);
if (!MerkleProofUpgradeable.verify(_merkleProof, merkleRoot, node))
revert InvalidProof();
} else if (accountIndexes[account] < merkleTreeSize) {
revert InvalidProof();
}
uint256 queuedTokens = getQueuedTokens(account, _amount);
uint256 canUnqueue = queuedTokens <= totalQueued ? queuedTokens : totalQueued;
uint256 amountToUnqueue = toWithdraw <= canUnqueue ? toWithdraw : canUnqueue;
if (amountToUnqueue != 0) {
accountQueuedTokens[account] -= amountToUnqueue;
totalQueued -= amountToUnqueue;
toWithdraw -= amountToUnqueue;
emit UnqueueTokens(account, amountToUnqueue);
}
}
if (toWithdraw != 0) {
IERC20Upgradeable(address(stakingPool)).safeTransferFrom(
account,
address(this),
toWithdraw
);
@>>> toWithdraw = _withdraw(account, toWithdraw, _shouldQueueWithdrawal);
}
token.safeTransfer(account, _amountToWithdraw - toWithdraw);
}
Tool used
Manual Review
Recommendation
Ensure that once tokens are transferred from the staking pool to the contract, they are not placed back in the withdrawal queue. As bellow
function withdraw(
uint256 _amountToWithdraw,
uint256 _amount,
uint256 _sharesAmount,
bytes32[] calldata _merkleProof,
bool _shouldUnqueue,
bool _shouldQueueWithdrawal
) external {
if (_amountToWithdraw == 0) revert InvalidAmount();
uint256 toWithdraw = _amountToWithdraw;
address account = msg.sender;
if (_shouldUnqueue == true) {
_requireNotPaused();
if (_merkleProof.length != 0) {
bytes32 node = keccak256(
bytes.concat(keccak256(abi.encode(account, _amount, _sharesAmount)))
);
if (!MerkleProofUpgradeable.verify(_merkleProof, merkleRoot, node))
revert InvalidProof();
} else if (accountIndexes[account] < merkleTreeSize) {
revert InvalidProof();
}
uint256 queuedTokens = getQueuedTokens(account, _amount);
uint256 canUnqueue = queuedTokens <= totalQueued ? queuedTokens : totalQueued;
uint256 amountToUnqueue = toWithdraw <= canUnqueue ? toWithdraw : canUnqueue;
if (amountToUnqueue != 0) {
accountQueuedTokens[account] -= amountToUnqueue;
totalQueued -= amountToUnqueue;
toWithdraw -= amountToUnqueue;
emit UnqueueTokens(account, amountToUnqueue);
}
}
if (toWithdraw != 0) {
IERC20Upgradeable(address(stakingPool)).safeTransferFrom(
account,
address(this),
toWithdraw
);
toWithdraw = _withdraw(account, toWithdraw, _shouldQueueWithdrawal);
+ _amountToWithdraw += toWithdraw;
+ toWithdraw -= toWithdraw
}
- token.safeTransfer(account, _amountToWithdraw - toWithdraw);
+ token.safeTransfer(account, _amountToWithdraw );
}
