Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Incorrect Queueing Behavior in `PriorityPool.deposit()` Function

lin0x9a7a

Summary

The PriorityPool contract has a bug in the _deposit() function that causes incorrect queueing behavior when shouldQueue is true and totalQueued is zero. The issue arises because the logic prioritizes depositing tokens into the withdrawal pool and staking pool, neglecting to queue the remaining tokens as expected. This bug affects users by not properly accounting for their tokens in the queue, leading to discrepancies in the expected behavior of the contract.

Vulnerability Details

When the shouldQueue parameter is set to true and totalQueued is zero. The issue occurs because the logic prioritizes depositing tokens into the withdrawal pool and staking pool, even if shouldQueue is true, resulting in the remaining tokens not being queued as expected. PriorityPool.sol# https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/PriorityPool.sol#L607-L644

if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
​
uint256 toDeposit = _amount;
​
if (totalQueued == 0) {
uint256 queuedWithdrawals = withdrawalPool.getTotalQueuedWithdrawals();
if (queuedWithdrawals != 0) {
uint256 toDepositIntoQueue = toDeposit <= queuedWithdrawals
? toDeposit
: queuedWithdrawals;
withdrawalPool.deposit(toDepositIntoQueue);
toDeposit -= toDepositIntoQueue;
IERC20Upgradeable(address(stakingPool)).safeTransfer(_account, toDepositIntoQueue);
}
​
if (toDeposit != 0) {
uint256 canDeposit = stakingPool.canDeposit();
if (canDeposit != 0) {
uint256 toDepositIntoPool = toDeposit <= canDeposit ? toDeposit : canDeposit;
stakingPool.deposit(_account, toDepositIntoPool, _data);
toDeposit -= toDepositIntoPool;
}
}
}
​
// Vulnerable code block
if (toDeposit != 0) {
if (_shouldQueue) {
_requireNotPaused();
if (accountIndexes[_account] == 0) {
accounts.push(_account);
accountIndexes[_account] = accounts.length - 1;
}
accountQueuedTokens[_account] += toDeposit;
totalQueued += toDeposit;
} else {
token.safeTransfer(_account, toDeposit);
}
}
​
emit Deposit(_account, _amount - toDeposit, _shouldQueue ? toDeposit : 0);
}

The fact that the queueing logic is inside the if (totalQueued == 0) block. When totalQueued is zero, the code prioritizes depositing tokens into the withdrawal pool and staking pool, even if _shouldQueue is set to true. As a result, the remaining tokens (toDeposit) are not queued correctly when _shouldQueue is true and totalQueued is zero.

  1. When a user calls the deposit() function with shouldQueue set to true, it internally calls the _deposit() function.

  2. Inside _deposit(), there is a check for if (totalQueued == 0). If totalQueued is indeed 0, the function proceeds to attempt depositing tokens into the withdrawal pool and staking pool.

  3. If there is room for deposits in either the withdrawal pool or staking pool, the tokens are deposited accordingly, reducing the remaining amount to be queued.

  4. After the deposits, if there are any remaining tokens, they are not queued even though shouldQueue is true. This is because the queuing logic is inside the if (totalQueued == 0) block, and it is not executed when totalQueued is 0.

This bug affects users because when they call deposit() with shouldQueue set to true, expecting the remaining tokens to be queued, the tokens may not be queued if totalQueued is 0 and there is room for deposits in the withdrawal pool or staking pool. As a result, users' tokens may not be properly accounted for in the queue, leading to discrepancies in the expected behavior of the contract.

Examining the this trace, we can see the following sequence of calls:

  1. PriorityPool.deposit(1, true, []) is called with amount = 1, shouldQueue = true, and empty data.

  2. Inside deposit(), it calls the internal function _deposit() with the same parameters.

  3. In _deposit(), it checks the poolStatus and totalQueued.

  4. Since totalQueued is 0, it attempts to deposit into the withdrawal pool and staking pool.

  5. The remaining amount (1) is not queued even though shouldQueue is true.

To reproduce the issue, call deposit() with shouldQueue = true when totalQueued is 0 and there is room for deposits in the withdrawal pool or staking pool.

Impact

Users' tokens may not be properly accounted for in the queue when they call the deposit() function with shouldQueue set to true and totalQueued is zero. This can lead to discrepancies in the expected behavior of the contract, causing confusion and loss of funds for users who expect their tokens to be queued correctly.

Tools Used

Vs Code

Recommendations

The queueing logic should be moved outside the if (totalQueued == 0) block to ensure that the remaining tokens are always queued when _shouldQueue is true, regardless of the value of totalQueued.

diff --git a/contracts/core/priorityPool/PriorityPool.sol b/contracts/core/priorityPool/PriorityPool.sol
index ...
--- a/contracts/core/priorityPool/PriorityPool.sol
+++ b/contracts/core/priorityPool/PriorityPool.sol
@@ -...
function _deposit(
address _account,
uint256 _amount,
bool _shouldQueue,
bytes[] memory _data
) internal {
...
if (totalQueued == 0) {
...
}
- if (toDeposit != 0) {
- if (_shouldQueue) {
- _requireNotPaused();
- if (accountIndexes[_account] == 0) {
- accounts.push(_account);
- accountIndexes[_account] = accounts.length - 1;
- }
- accountQueuedTokens[_account] += toDeposit;
- totalQueued += toDeposit;
- } else {
- token.safeTransfer(_account, toDeposit);
+ if (_shouldQueue && toDeposit != 0) {
+ _requireNotPaused();
+ if (accountIndexes[_account] == 0) {
+ accounts.push(_account);
+ accountIndexes[_account] = accounts.length - 1;
}
+ accountQueuedTokens[_account] += toDeposit;
+ totalQueued += toDeposit;
+ } else if (toDeposit != 0) {
+ token.safeTransfer(_account, toDeposit);
}
...
}
...
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.