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

Inconsistent deposit behaviour in paused state based on queue preference (`_shouldQueue`)

iamthesvn

Summary

The PriorityPool contract exhibits inconsistent behaviour when processing deposits in a paused state. When the contract is paused, deposits that don't require queueing are partially processed, while deposits that do require queueing are fully reverted. This inconsistency could lead to unexpected behaviour and potential exploitation.

Relevant links

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/PriorityPool.sol#L632-L644

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/PriorityPool.sol#L601-L647

Vulnerability Details

In the _deposit function of the PriorityPool contract, there's an inconsistency in how deposits are handled when the contract is paused:

function _deposit(
address _account,
uint256 _amount,
bool _shouldQueue,
bytes[] memory _data
) internal {
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;
}
}
}
​
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 issue arises specifically in this section of the function:

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);
}
}

When _shouldQueue is false:

  1. The function processes the deposit up to the amount of queued withdrawals.

  2. Any remaining amount is returned to the user.

  3. The transaction does not revert, even though the contract is paused.

When _shouldQueue is true:

  1. The function attempts to process the deposit up to the amount of queued withdrawals.

  2. When trying to queue the remaining amount, it calls _requireNotPaused().

  3. This causes the entire transaction to revert, undoing any processed withdrawals.

This inconsistency means that the paused state is only partially enforced, depending on the _shouldQueue parameter. This could lead to unexpected state changes even when the contract is supposed to be paused.

Impact

Likelihood of Exploitation:
High - The conditions required for this improper functioning are easily achievable. The inconsistent behaviour occurs in a common state (when the contract is paused) and can be triggered by standard user interactions (deposits with different queue preferences).

While user funds are not directly at risk, the protocol fails to function as intended:

  1. Inconsistent enforcement of the paused state

  2. Potential for unexpected partial deposits when paused

  3. Violation of the expected behaviour of a paused contract

This discrepancy between intended and actual functionality could lead to user confusion and potentially undermine trust in the protocol's reliability, even if it doesn't directly threaten user assets.

Tools Used

Manual review

Recommendations

Enforce consistent behaviour:
Move the _requireNotPaused() check to the beginning of the _deposit function.
This ensures that no deposits of any kind can be processed when the contract is paused.

Alternative approach:
If the intention is to allow fulfilling queued withdrawals even when paused, create a separate function for this purpose that can be called even when paused.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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