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

Creation Of `Multiple Duplicate Batches` With Same `indexOfLastWithdrawal` and different `StakePerShare` On Subsequent Partial Withdrawal Finalization

j1v9

Summary

In withdrawalPool, when queued withdrawals receive deposited tokens to be finalized, the _finalizeWithdrawals function handles finalizing these withdrawals partialy or completely.
When there are not enough shares to satisy a queued withdrawal completely, it is partially finalized and then a batch is created for all the finalized withdrawals before it.

The issue arises when the leftover partially finalized withdrawal on the next call to _finalizeWthdrawals, if the withdrawal with the shares amount sent to finalize the withdrawal is still not enough to completely finalize it, it would create a duplicate of the previous batch(i.e having the same indexOfLastWithdrawal) but may have varying stakePerShare depending on the stake per share rate when the call is made.

Vulnerability Details

This is the functionality here in the _finalizeWithdrawals in the WithdrawalPool contract, with some of my comments in the code...

function _finalizeWithdrawals(uint256 _amount) internal {
// ... some code here ...
​
for (uint256 i = indexOfNextWithdrawal; i < numWithdrawals; ++i) {
uint256 sharesRemaining = queuedWithdrawals[i].sharesRemaining;
​
// if can fully finalize withdrawal id, continue
if (sharesRemaining < sharesToWithdraw) {
sharesToWithdraw -= sharesRemaining;
continue;
}
​
-- // if it cant fully finalize the withdrawal id, creates a batch with the batch's
-- // lastIndexOfWithdrawal as the last finalized withdrawal id
-- // issue arises when the already partially finalized withdrawal cannot still be fully finalized on subsequent calls
-- // this in turn creates duplicate batches with the same `lastIndexOdWithdrawal` but different stake per share for the batches
-- // based on the current share rate used to satisfy the unfinalized withdrawal.
if (sharesRemaining > sharesToWithdraw) {
queuedWithdrawals[i] = Withdrawal(
uint128(sharesRemaining - sharesToWithdraw),
uint128(
queuedWithdrawals[i].partiallyWithdrawableAmount +
_getStakeByShares(sharesToWithdraw)
)
);
​
indexOfNextWithdrawal = i;
-- // creates the duplicate batch here with the different stake per share
-- // if the previous partially finalized withdrawal cannot still be fully finalizes
withdrawalBatches.push(
WithdrawalBatch(uint128(i - 1), uint128(_getStakeByShares(1 ether)))
);
} else {
indexOfNextWithdrawal = i + 1;
withdrawalBatches.push(
WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
);
}
​
sharesToWithdraw = 0;
break;
}
​
//... some code here ....
}

If you take a look at the function closely you can see that when the withdrawal cannot be completely finalized it creates a batch for the previously fully finalized withdrawals.
The issue arises when the previously partially finalized queued withdrawal still cannot be fully finalized in subsequent calls.


E.g withdrawal id's 1 - 5 are batched together because withdrawal id 6 cannot be fully finalized with the available sharesToWithdraw. So withdrawal Id 6 is partially finalized and a batch is created for withdrawal Id's 1 -5.

On the next call to finalize withdrawals if the sharesToWithdraw is still not enought to fully finalize the withdrawal id 6, we end up creating a duplicate withdrawal batch of batch 1-5, this time the new batch may have a different sharePerStake due to the when the _finallizeWithdrawals the share per stake migh have changed (higher or lower or same).

// ... some code here....
​
// conditions here would be true since withdrawal id 6 still cannot be fully finalized
if (sharesRemaining > sharesToWithdraw) {
queuedWithdrawals[i] = Withdrawal(
uint128(sharesRemaining - sharesToWithdraw),
uint128(
queuedWithdrawals[i].partiallyWithdrawableAmount +
_getStakeByShares(sharesToWithdraw)
)
);
​
indexOfNextWithdrawal = i;
-- // creates the duplicate batch of the previous batch here with the different stake per share
-- // depending on what the value of the current stake per share is
withdrawalBatches.push(
WithdrawalBatch(uint128(i - 1), uint128(_getStakeByShares(1 ether)))
);
}
// .... some code here....

These duplicate batches with different stake per shares would continue to be created so far the partially finalized withdrawal Id cannot be finalized for batching with the sharesToWithdrawused.

Recommendations

Keep track of the value of indexOfLastWithdrawalof the new batch to be created and if it matches the previous batch indexOfLastWithdrawal then dont create the batch, only finalize the partial withdrawal id.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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