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

Incorrect Update of Total Queued Withdrawal Shares

aariiif

Summary

_finalizeWithdrawals function is called by the deposit function to process queued withdrawals when new tokens are deposited. However, _finalizeWithdrawal has a logic flaw in how it updates the totalQueuedShareWithdrawals variable.

Iit's possible that not all of these shares will actually be used to finalize withdrawals, if the total amount of queued withdrawals is less than sharesToWithdraw. In this case, totalQueuedShareWithdrawals will be decreased by more than the actual amount of shares used, causing it to become out of sync with the real total of queued withdrawals.

Vulnerability Details

The _finalizeWithdrawals function in the WithdrawalPool contract incorrectly updates the totalQueuedShareWithdrawals variable when processing queued withdrawals during a deposit. The function decreases totalQueuedShareWithdrawals by the full amount of shares corresponding to the deposited tokens, even if not all of these shares are actually used to finalize withdrawals.

This can happen when the total amount of queued withdrawals is less than the amount of shares corresponding to the deposited tokens. In such cases, totalQueuedShareWithdrawals will be decreased by more than the actual amount of shares used, causing it to become out of sync with the real total of queued withdrawals.

In WithdrawalPool.sol: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/WithdrawalPool.sol#L422-L426

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/WithdrawalPool.sol#L323-L327

function deposit(uint256 _amount) external onlyPriorityPool {
token.safeTransferFrom(msg.sender, address(this), _amount);
lst.safeTransfer(msg.sender, _amount);
_finalizeWithdrawals(_amount); // @audit calls _finalizeWithdrawals with full deposit amount
}
​
// ...
​
function _finalizeWithdrawals(uint256 _amount) internal {
uint256 sharesToWithdraw = _getSharesByStake(_amount);
uint256 numWithdrawals = queuedWithdrawals.length;
​
totalQueuedShareWithdrawals -= sharesToWithdraw; // @audit decreases by full amount regardless of usage
​
// ...
}

Proof of Concept Scenario

  1. A user queues a withdrawal of 100 shares by calling queueWithdrawal with an amount of LST tokens corresponding to 100 shares.

  2. Another user makes a deposit of tokens corresponding to 500 shares by calling deposit.

  3. The _finalizeWithdrawals function is called with sharesToWithdraw equal to 500. It immediately decreases totalQueuedShareWithdrawals by 500.

  4. However, when processing the queued withdrawals, it only uses 100 of these shares to finalize the one queued withdrawal.

  5. The remaining 400 shares are not used, but totalQueuedShareWithdrawals has already been decreased by the full 500.

  6. As a result, totalQueuedShareWithdrawals is now 400 less than the real total of queued withdrawal shares.

Impact

The discrepancy between totalQueuedShareWithdrawals and the real queued amount may grow over time as more deposits and withdrawals happen, exacerbating the impact.

Tools Used

Manual Review

Recommendations

_finalizeWithdrawals() should track the actual number of shares used to finalize withdrawals and only decrease totalQueuedShareWithdrawals by that amount.

We introduces a sharesUsed variable to track the actual number of shares used to finalize withdrawals. It increments sharesUsed as it processes each withdrawal, and then decreases totalQueuedShareWithdrawals by sharesUsed at the end of the function.

function _finalizeWithdrawals(uint256 _amount) internal {
uint256 sharesToWithdraw = _getSharesByStake(_amount);
uint256 numWithdrawals = queuedWithdrawals.length;
+ uint256 sharesUsed;
​
- totalQueuedShareWithdrawals -= sharesToWithdraw;
​
for (uint256 i = indexOfNextWithdrawal; i < numWithdrawals; ++i) {
uint256 sharesRemaining = queuedWithdrawals[i].sharesRemaining;
​
if (sharesRemaining < sharesToWithdraw) {
// fully finalize withdrawal
- sharesToWithdraw -= sharesRemaining;
+ sharesUsed += sharesRemaining;
continue;
}
​
if (sharesRemaining > sharesToWithdraw) {
// partially finalize withdrawal
queuedWithdrawals[i] = Withdrawal(
uint128(sharesRemaining - sharesToWithdraw),
uint128(
queuedWithdrawals[i].partiallyWithdrawableAmount +
_getStakeByShares(sharesToWithdraw)
)
);
indexOfNextWithdrawal = i;
withdrawalBatches.push(
WithdrawalBatch(uint128(i - 1), uint128(_getStakeByShares(1 ether)))
);
+ sharesUsed += sharesToWithdraw;
} else {
// fully finalize withdrawal
indexOfNextWithdrawal = i + 1;
withdrawalBatches.push(
WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
);
+ sharesUsed += sharesRemaining;
}
​
sharesToWithdraw = 0;
break;
}
​
+ totalQueuedShareWithdrawals -= sharesUsed;
​
// entire amount must be accounted for
assert(sharesToWithdraw == 0);
​
emit WithdrawalsFinalized(_amount);
}
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.