Summary
Withdrawing in the WithdrawalPool is done through the withdraw function:
function withdraw(uint256[] calldata _withdrawalIds, uint256[] calldata _batchIds) external {
address owner = msg.sender;
uint256 amountToWithdraw;
for (uint256 i = 0; i < _withdrawalIds.length; ++i) {
uint256 withdrawalId = _withdrawalIds[i];
Withdrawal memory withdrawal = queuedWithdrawals[_withdrawalIds[i]];
uint256 batchId = _batchIds[i];
WithdrawalBatch memory batch = withdrawalBatches[batchId];
if (withdrawalOwners[withdrawalId] != owner) revert SenderNotAuthorized();
if (
batchId != 0 && withdrawalId <= withdrawalBatches[batchId - 1].indexOfLastWithdrawal
) revert InvalidWithdrawalId();
if (
batchId != 0 &&
withdrawalId > batch.indexOfLastWithdrawal &&
withdrawal.partiallyWithdrawableAmount == 0
) revert InvalidWithdrawalId();
if (withdrawalId <= batch.indexOfLastWithdrawal) {
amountToWithdraw +=
withdrawal.partiallyWithdrawableAmount +
(uint256(batch.stakePerShares) * uint256(withdrawal.sharesRemaining)) /
1e18;
delete queuedWithdrawals[withdrawalId];
delete withdrawalOwners[withdrawalId];
} else {
amountToWithdraw += withdrawal.partiallyWithdrawableAmount;
queuedWithdrawals[withdrawalId].partiallyWithdrawableAmount = 0;
}
}
token.safeTransfer(owner, amountToWithdraw);
emit Withdraw(owner, amountToWithdraw);
}
It accepts two arrays:
However, the lengths of these two arrays are never checked and in the case in which _batchIds's length is larger than _withdrawalIds the execution of the transaction will be successful even though such execution will not make sense.
Vulnerability Details
The vulnerability comes from the fact that the whole function operates with the assumption that the two input arrays have the same length without requiring this anywhere.
Impact
The impact of such a faulty transaction not reverting would be that the batch ids will be not correct for the given withdrawal ids. This will lead to wrong checks based on the batch that was fetched through the given id.
Tools Used
Manual Review
Recommendations
Implement a check for array length:
function withdraw(uint256[] calldata _withdrawalIds, uint256[] calldata _batchIds) external {
+ require(_withdrawalIds.length == _batchIds.length, "Wrong input data");
address owner = msg.sender;
uint256 amountToWithdraw;
for (uint256 i = 0; i < _withdrawalIds.length; ++i) {
uint256 withdrawalId = _withdrawalIds[i];
Withdrawal memory withdrawal = queuedWithdrawals[_withdrawalIds[i]];
uint256 batchId = _batchIds[i];
WithdrawalBatch memory batch = withdrawalBatches[batchId];
if (withdrawalOwners[withdrawalId] != owner) revert SenderNotAuthorized();
if (
batchId != 0 && withdrawalId <= withdrawalBatches[batchId - 1].indexOfLastWithdrawal
) revert InvalidWithdrawalId();
if (
batchId != 0 &&
withdrawalId > batch.indexOfLastWithdrawal &&
withdrawal.partiallyWithdrawableAmount == 0
) revert InvalidWithdrawalId();
if (withdrawalId <= batch.indexOfLastWithdrawal) {
amountToWithdraw +=
withdrawal.partiallyWithdrawableAmount +
(uint256(batch.stakePerShares) * uint256(withdrawal.sharesRemaining)) /
1e18;
delete queuedWithdrawals[withdrawalId];
delete withdrawalOwners[withdrawalId];
} else {
amountToWithdraw += withdrawal.partiallyWithdrawableAmount;
queuedWithdrawals[withdrawalId].partiallyWithdrawableAmount = 0;
}
}
token.safeTransfer(owner, amountToWithdraw);
emit Withdraw(owner, amountToWithdraw);
}
