Summary
Finalize withdrawal will revert if the first queued withdrawal is higher than shares to withdraw
Vulnerability Details
In priority pool contract, priority is determined in following order WithdrawalPool -> Staking Pool -> Local Queue ( Merkle ). We can investigate it from deposit function in PP.
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);
}
}
If the withdrawal pool has queued withdrawal, it will choose withdrawal pool in first priority and it will continue execution in withdrawal pool.
function deposit(uint256 _amount) external onlyPriorityPool {
token.safeTransferFrom(msg.sender, address(this), _amount);
lst.safeTransfer(msg.sender, _amount);
_finalizeWithdrawals(_amount);
}
It simply swap the token with queued withdrawal and deposited amount in this logic. But in _finalizeWithdrawals() function it will revert the deposit if it's the first queued withdrawal and queued withdrawal is higher than currently deposited amount. Because it will substract 1 from iis equal to 0 ( because indexOfNextWithdrawal is fresh and equal to 0 ) and it will underflowed.
function _finalizeWithdrawals(uint256 _amount) internal {
uint256 sharesToWithdraw = _getSharesByStake(_amount);
uint256 numWithdrawals = queuedWithdrawals.length;
totalQueuedShareWithdrawals -= sharesToWithdraw;
for (uint256 i = indexOfNextWithdrawal; i < numWithdrawals; ++i) {
uint256 sharesRemaining = queuedWithdrawals[i].sharesRemaining;
if (sharesRemaining < sharesToWithdraw) {
sharesToWithdraw -= sharesRemaining;
continue;
}
if (sharesRemaining > sharesToWithdraw) {
queuedWithdrawals[i] = Withdrawal(
uint128(sharesRemaining - sharesToWithdraw),
uint128(
queuedWithdrawals[i].partiallyWithdrawableAmount +
_getStakeByShares(sharesToWithdraw)
)
);
indexOfNextWithdrawal = i;
&> withdrawalBatches.push(
WithdrawalBatch(uint128(i - 1), uint128(_getStakeByShares(1 ether)))
);
Impact
Deposits will be impossible in this situation until deposit is higher or equal than queued withdrawal. It will also cause DoS for any function which use finalize withdrawals.
It's also open to attack path in the future:
Attacker can flash loan LINK token from any DeFi
Deposit them and get LST from the protocol, he can swap that amount ( In the future )
Repay the flash loan and queue withdrawal
Queued withdrawal will be really high and this amount will cause very a certain time of DoS.
Tools Used
Manual Review
Recommendations
Avoid underflow issue in indexing
