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

Users would incur losses when they want to withdraw and they would receive less than they should

mrmorningstar

Summary

When users want to withdraw their tokens they can do that from withdraw function in PriorityPool.sol. Which looks like this:

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;
​
// attempt to unqueue tokens before withdrawing if flag is set
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);
}
}
​
// attempt to withdraw if tokens remain after unqueueing
if (toWithdraw != 0) {
IERC20Upgradeable(address(stakingPool)).safeTransferFrom(
account,
address(this),
toWithdraw
);
toWithdraw = _withdraw(account, toWithdraw, _shouldQueueWithdrawal);
}
​
token.safeTransfer(account, _amountToWithdraw - toWithdraw);
}

If full withdrawal amount cannot be satisfied after unqueuing, protocol transfer remaining amount from users account from StakingPool as we can see in this part here:

// attempt to withdraw if tokens remain after unqueueing
if (toWithdraw != 0) {
IERC20Upgradeable(address(stakingPool)).safeTransferFrom(
account,
address(this),
toWithdraw
);
toWithdraw = _withdraw(account, toWithdraw, _shouldQueueWithdrawal);
}

And calls _withdraw that looks like this :

function _withdraw(
address _account,
uint256 _amount,
bool _shouldQueueWithdrawal
) internal returns (uint256) {
if (poolStatus == PoolStatus.CLOSED) revert WithdrawalsDisabled();
​
uint256 toWithdraw = _amount;
​
if (totalQueued != 0) {
uint256 toWithdrawFromQueue = toWithdraw <= totalQueued ? toWithdraw : totalQueued;
​
totalQueued -= toWithdrawFromQueue;
depositsSinceLastUpdate += toWithdrawFromQueue;
sharesSinceLastUpdate += stakingPool.getSharesByStake(toWithdrawFromQueue);
toWithdraw -= toWithdrawFromQueue;
}
​
if (toWithdraw != 0) {
if (!_shouldQueueWithdrawal) revert InsufficientLiquidity();
withdrawalPool.queueWithdrawal(_account, toWithdraw);
}
​
emit Withdraw(_account, _amount - toWithdraw);
return toWithdraw;
}

If here also the full amount cannot be satisfied then the protocol will queue remaining amount in WithdrawalPool (if user set _shouldQueueWithdrawal to true otherwise it would revert) via queueWithdrawal function that looks like this:

function queueWithdrawal(address _account, uint256 _amount) external onlyPriorityPool {
if (_amount < minWithdrawalAmount) revert AmountTooSmall();
// the part below will never execute if _amount < minWithdrawalAmount
lst.safeTransferFrom(msg.sender, address(this), _amount);
​
uint256 sharesAmount = _getSharesByStake(_amount);
queuedWithdrawals.push(Withdrawal(uint128(sharesAmount), 0));
totalQueuedShareWithdrawals += sharesAmount;
​
uint256 withdrawalId = queuedWithdrawals.length - 1;
queuedWithdrawalsByAccount[_account].push(withdrawalId);
withdrawalOwners[withdrawalId] = _account;
​
emit QueueWithdrawal(_account, _amount);
}

Vulnerability Details

The issue here is that the queueWithdrawal function will revert if amount that are tried to be queued is less than minWithdrawalAmount, but the _withdraw function in PriorityPool does not check if queue was succesfull or not and it will return toWithdraw amount assuming queue was succesfull. That unsuccesfully queued amount would be deducted from users _amountToWithdraw , assuming it is queued and can be withdrawn latter by user when there is enough liquidity. But it would be not possible because queueWithdrawal reverted and user address is never mapped in WithdrawalPool as I mentioned in comment above. That amount would be stuck forever in PriorityPool and user would not he able to withdraw and will incur loss of funds because of that.

At first maybe it will be small amount, but the more this happens the users losses would increase more and more.

Impact

Users will incur loss of funds and lost amount would be stuck .

Tools Used

Manual Review

Recommendations

Redesign queueWithdrawal function to return bool, true if queue was succesfull and in PriorityPool::_withdraw check if queue is successful or not. Add additional mapping in PriorityPool that will store amount that is not queued in WithdrawalPool by each account and whenever PriorityPool try to queue in WithdrawalPool for the same user add that amount also. And if queue was not succesfull again add that additional amount to the previous and repeat next time until it was succesfully queued in WithdrawalPool. This will make sure users does not incur any loss of funds.

Updates

Lead Judging Commences

inallhonesty Lead Judge
9 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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