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

The `withdrawalBatchIdCutoff` value is incorrectly updated in the `WithdrawalPool::updateWithdrawalBatchIdCutoff()` function, leading to potential discrepancies in tracking completed withdrawal batches.

joicygiore

Summary

The withdrawalBatchIdCutoff value is incorrectly updated in the WithdrawalPool::updateWithdrawalBatchIdCutoff() function, leading to potential discrepancies in tracking completed withdrawal batches.

Vulnerability Details

As shown in the following code snippet, when the @>1 code line is established and the loop is jumped out, the @>2 code line will not be executed. At this time, the value of newWithdrawalBatchIdCutoff is actually equal to i - 1. According to the function annotation, newWithdrawalBatchIdCutoff -> withdrawalBatchIdCutoff should point to the next batch index after all withdrawal requests have been fully completed, so the value of this index should be i.

In the current implementation of the WithdrawalPool::updateWithdrawalBatchIdCutoff() function, as highlighted in the code snippet below, when the condition at line @>1 is met and the loop is exited, the code at line @>2 is not executed. This results in newWithdrawalBatchIdCutoff retaining the value of i - 1 instead of i. According to the function's documentation, newWithdrawalBatchIdCutoff (which is eventually assigned to withdrawalBatchIdCutoff) should point to the batch index where all withdrawal requests have been fully processed. Therefore, the value of newWithdrawalBatchIdCutoff should be updated to i before breaking the loop.

@> // all batches before this index have had all withdrawal requests fully withdrawn
uint128 public withdrawalBatchIdCutoff;
// all withdrawal requests before this index have been fully withdrawn
uint128 public withdrawalIdCutoff;
​
// WithdrawalPool::updateWithdrawalBatchIdCutoff()
function updateWithdrawalBatchIdCutoff() external {
uint256 numWithdrawals = queuedWithdrawals.length;
uint256 newWithdrawalIdCutoff = withdrawalIdCutoff;
​
​
// find the first withdrawal that has funds remaining
for (uint256 i = newWithdrawalIdCutoff; i < numWithdrawals; ++i) {
newWithdrawalIdCutoff = i;
​
​
Withdrawal memory withdrawal = queuedWithdrawals[i];
if (withdrawal.sharesRemaining != 0 || withdrawal.partiallyWithdrawableAmount != 0) {
break;
}
}
​
​
uint256 numBatches = withdrawalBatches.length;
uint256 newWithdrawalBatchIdCutoff = withdrawalBatchIdCutoff;
​
​
// find the last batch where all withdrawals have no funds remaining
for (uint256 i = newWithdrawalBatchIdCutoff; i < numBatches; ++i) {
@>1 if (withdrawalBatches[i].indexOfLastWithdrawal >= newWithdrawalIdCutoff) {
break;
}
​
​
@>2 newWithdrawalBatchIdCutoff = i;
}
​
​
withdrawalIdCutoff = uint128(newWithdrawalIdCutoff);
withdrawalBatchIdCutoff = uint128(newWithdrawalBatchIdCutoff);
}

Code Snippet

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

Impact

This issue causes the withdrawalBatchIdCutoff to be updated incorrectly, leading to an inaccurate representation of the last fully completed withdrawal batch. If not addressed, this could affect the efficiency and correctness of withdrawal data, potentially impacting the proper tracking of withdrawal batches and the return of outdated batch data.

Tools Used

Manual Review

Recommendations

To ensure newWithdrawalBatchIdCutoff correctly reflects the batch where all withdrawals have been completed, update its value to i before breaking the loop, as shown in the proposed code modification below:

function updateWithdrawalBatchIdCutoff() external {
uint256 numWithdrawals = queuedWithdrawals.length;
uint256 newWithdrawalIdCutoff = withdrawalIdCutoff;
​
​
// find the first withdrawal that has funds remaining
for (uint256 i = newWithdrawalIdCutoff; i < numWithdrawals; ++i) {
newWithdrawalIdCutoff = i;
​
​
Withdrawal memory withdrawal = queuedWithdrawals[i];
if (withdrawal.sharesRemaining != 0 || withdrawal.partiallyWithdrawableAmount != 0) {
break;
}
}
​
​
uint256 numBatches = withdrawalBatches.length;
uint256 newWithdrawalBatchIdCutoff = withdrawalBatchIdCutoff;
​
​
// find the last batch where all withdrawals have no funds remaining
for (uint256 i = newWithdrawalBatchIdCutoff; i < numBatches; ++i) {
if (withdrawalBatches[i].indexOfLastWithdrawal >= newWithdrawalIdCutoff) {
+ newWithdrawalBatchIdCutoff = i;
break;
}
​
​
- newWithdrawalBatchIdCutoff = i;
}
​
​
withdrawalIdCutoff = uint128(newWithdrawalIdCutoff);
withdrawalBatchIdCutoff = uint128(newWithdrawalBatchIdCutoff);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

newWithdrawalBatchIdCutoff

Support

FAQs

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