Description
_finalizeWithdrawals allows the codebase to update totalQueuedShareWithdrawals, queuedWithdrawals, indexOfNextWithdrawal and withdrawalBatches after a withdrawal.
In the case of a fully finalized withdrawal, it does not update queuedWithdrawals to set sharesRemaining and partiallyWithdrawableAmount to 0.
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)))
);
} else {
@> indexOfNextWithdrawal = i + 1;
@> withdrawalBatches.push(
@> WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
@> );
}
sharesToWithdraw = 0;
break;
}
assert(sharesToWithdraw == 0);
emit WithdrawalsFinalized(_amount);
}
This will cause the getters getWithdrawals and getWithdrawalIdsByOwner to return incorrect data:
function getWithdrawals(
uint256[] calldata _withdrawalIds
) external view returns (Withdrawal[] memory) {
Withdrawal[] memory withdrawals = new Withdrawal[]();
for (uint256 i = 0; i < _withdrawalIds.length; ++i) {
@> withdrawals[i] = queuedWithdrawals[_withdrawalIds[i]];
}
@> return withdrawals;
}
function getWithdrawalIdsByOwner(address _account) public view returns (uint256[] memory) {
uint256[] memory activeWithdrawals = new uint256[](
queuedWithdrawalsByAccount[_account].length
);
uint256 totalActiveWithdrawals;
for (uint256 i = 0; i < activeWithdrawals.length; ++i) {
uint256 withdrawalId = queuedWithdrawalsByAccount[_account][i];
Withdrawal memory withdrawal = queuedWithdrawals[withdrawalId];
@> if (withdrawal.sharesRemaining != 0 || withdrawal.partiallyWithdrawableAmount != 0) {
activeWithdrawals[i] = withdrawalId;
totalActiveWithdrawals++;
}
}
uint256[] memory withdrawalIds = new uint256[]();
uint256 withdrawalIdsAdded;
for (uint256 i = 0; i < activeWithdrawals.length; ++i) {
if (activeWithdrawals[i] != 0) {
@> withdrawalIds[withdrawalIdsAdded] = activeWithdrawals[i];
withdrawalIdsAdded++;
}
}
@> return withdrawalIds;
}
This will also corrupt getFinalizedWithdrawalIdsByOwner.
The biggest impact will occur in withdraw. Anyone can withdraw again any finalized withdrawal if there was a partiallyWithdrawableAmount in this withdrawal :
With the real batchId of your withdrawal to collect twice the full amount.
With the batchId to 0, to collect only the partiallyWithdrawableAmount amount again.
In both cases, it will pass those checks and enter that condition:
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);
}
Risk
Likelyhood: High
Impact: High
Recommended Mitigation
As in the withdrawal function, delete this information once the withdrawal is finalized.
+ delete queuedWithdrawals[i];
+ delete withdrawalOwners[i];
