Summary
The return value retFinalizedWithdrawals in the WithdrawalPool::getFinalizedWithdrawalIdsByOwner() function is incorrect, potentially leading to incomplete data being returned.
Vulnerability Details
The issue lies within the logic that handles the finalization of withdrawals, specifically in the way the function iterates over and returns finalized withdrawals. The problematic parts of the code are marked with @> in the following analysis:
The length of finalizedWithdrawals is equal to withdrawalIds.length.
The counter totalFinalizedWithdrawals tracks the number of valid finalized withdrawals, ensuring totalFinalizedWithdrawals <= finalizedWithdrawals.length. However, totalFinalizedWithdrawals should only be used to initialize the retFinalizedWithdrawals array, not for iterating over finalizedWithdrawals.
To correctly traverse the finalizedWithdrawals array and populate retFinalizedWithdrawals, the loop should use finalizedWithdrawals.length instead of totalFinalizedWithdrawals.
function getFinalizedWithdrawalIdsByOwner(
address _account
) external view returns (uint256[] memory, uint256) {
uint256[] memory withdrawalIds = getWithdrawalIdsByOwner(_account);
uint256[] memory batchIds = getBatchIds(withdrawalIds);
@> uint256[] memory finalizedWithdrawals = new uint256[]();
uint256 totalFinalizedWithdrawals;
uint256 totalWithdrawable;
for (uint256 i = 0; i < batchIds.length; ++i) {
Withdrawal memory withdrawal = queuedWithdrawals[withdrawalIds[i]];
if (batchIds[i] != 0 || withdrawal.partiallyWithdrawableAmount != 0) {
finalizedWithdrawals[i] = withdrawalIds[i];
@> totalFinalizedWithdrawals++;
totalWithdrawable += withdrawal.partiallyWithdrawableAmount;
if (batchIds[i] != 0) {
totalWithdrawable +=
(uint256(withdrawalBatches[batchIds[i]].stakePerShares) *
uint256(withdrawal.sharesRemaining)) /
1e18;
}
} else {
break;
}
}
uint256[] memory retFinalizedWithdrawals = new uint256[]();
uint256 withdrawalsAdded;
@> for (uint256 i = 0; i < totalFinalizedWithdrawals; ++i) {
@> uint256 withdrawalId = finalizedWithdrawals[i];
if (withdrawalId != 0) {
retFinalizedWithdrawals[withdrawalsAdded] = withdrawalId;
withdrawalsAdded++;
}
}
return (retFinalizedWithdrawals, totalWithdrawable);
}
Code Snippet
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/WithdrawalPool.sol#L213-L253
Impact
The incorrect use of totalFinalizedWithdrawals in the iteration results in incomplete data being returned in retFinalizedWithdrawals. This could lead to users receiving incomplete information about their finalized withdrawals, causing issues with fund visibility or tracking.
Tools Used
Manual Review
Recommendations
To fix this issue, the loop responsible for populating retFinalizedWithdrawals should iterate over the entire finalizedWithdrawals array, not just up to totalFinalizedWithdrawals, to ensure that all potential entries are processed.
function getFinalizedWithdrawalIdsByOwner(
address _account
) external view returns (uint256[] memory, uint256) {
uint256[] memory withdrawalIds = getWithdrawalIdsByOwner(_account);
uint256[] memory batchIds = getBatchIds(withdrawalIds);
uint256[] memory finalizedWithdrawals = new uint256[]();
uint256 totalFinalizedWithdrawals;
uint256 totalWithdrawable;
for (uint256 i = 0; i < batchIds.length; ++i) {
Withdrawal memory withdrawal = queuedWithdrawals[withdrawalIds[i]];
if (batchIds[i] != 0 || withdrawal.partiallyWithdrawableAmount != 0) {
finalizedWithdrawals[i] = withdrawalIds[i];
totalFinalizedWithdrawals++;
totalWithdrawable += withdrawal.partiallyWithdrawableAmount;
if (batchIds[i] != 0) {
totalWithdrawable +=
(uint256(withdrawalBatches[batchIds[i]].stakePerShares) *
uint256(withdrawal.sharesRemaining)) /
1e18;
}
} else {
break;
}
}
uint256[] memory retFinalizedWithdrawals = new uint256[]();
uint256 withdrawalsAdded;
- for (uint256 i = 0; i < totalFinalizedWithdrawals; ++i) {
+ for (uint256 i = 0; i < finalizedWithdrawals.length; ++i) {
uint256 withdrawalId = finalizedWithdrawals[i];
if (withdrawalId != 0) {
retFinalizedWithdrawals[withdrawalsAdded] = withdrawalId;
withdrawalsAdded++;
}
}
return (retFinalizedWithdrawals, totalWithdrawable);
}
