Liquid Staking

Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Submission Details
Severity: high
Invalid

Due to incorrect design in `WithdrawalPool::_finalizeWithdrawals` fully finalize withdrawals will not be done

Summary

There are two types of withdrawals in the protocol full and partial withdrawals. Full are when amount of shares to withdraw are greater than queued shares remaining and when it is otherwise around then it is partial withdrawal.

This is done in the first check the function _finalizeWithdrawals function does as we can see here:

if (sharesRemaining < sharesToWithdraw) {
// fully finalize withdrawal
sharesToWithdraw -= sharesRemaining;
continue;
}

The shares are deducted and sharesTowithdraw have new deducted value now.

The issue here is that then function does not proceed and do full finalized withdrawal accounting , instead it checks if sharesRemaining are bigger than new updated sharesToWithdraw and based on outcome of that second check it will decide if it will do full or partial withdrawal as we can see here:

if (sharesRemaining < sharesToWithdraw) {
// fully finalize withdrawal
sharesToWithdraw -= sharesRemaining;
continue;
}
if (sharesRemaining > sharesToWithdraw) {
// partially finalize withdrawal
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 {
// fully finalize withdrawal
indexOfNextWithdrawal = i + 1;
withdrawalBatches.push(
WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
);
}

Vulnerability Details

In order to see why this is issue lets check the following PoC:

For the sake of simplicity I will use following values:

sharesToWithdraw = 10

sharesRemaining = 7

First it check if sharesRemaining < sharesToWithdraw which in our case it is true because 7 < 10.

This should mean that withdrawal should be accounted as full withdrawal.

Then it deduct that amount and sharesToWithdraw now have value of 3 (10 - 7 = 3).

Instead of do the full finalized withdrawal as it should instead it do another check if sharesRemaining > sharesToWithdraw which now would be true also because 7 > 3 and it will proceed executing with partial withdrawal instead of full withdrawal as it should have done.

So instead of someone receiving full amount as it should it will receive way less (only 3 shares in our example), but the full amount of 7 shares would be deducted from him as we could see in the example above. Which in the end will make users incur losses and not be able to withdraw correct amount due to incorrect accounting of the protocol.

The _finalizeWithdrawals is used by performUpkeep and by deposit which is called from PriotityPool, so in the end whole protocol can be impacted due to incorrect accounting and wrong data can be passed.

Impact

Incorrect implementation of _finalizeWithdrawals could make users incur losses and not be able to withdraw correct amount due to incorrect accounting and wrong data can be passed to the protocol.

Tools Used

Manual Review

Recommendations

Do a full finalized withdrawal accounting if sharesRemaining are less than sharesToWithdraw immediatelly after deducting shares like this:

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) {
// fully finalize withdrawal
sharesToWithdraw -= sharesRemaining;
- continue;
+ indexOfNextWithdrawal = i + 1;
+ withdrawalBatches.push(
+ WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
+ );
+ } else {
- if (sharesRemaining > sharesToWithdraw) {
// partially finalize withdrawal
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 {
- // fully finalize withdrawal
- indexOfNextWithdrawal = i + 1;
- withdrawalBatches.push(
- WithdrawalBatch(uint128(i), uint128(_getStakeByShares(1 ether)))
- );
- }
sharesToWithdraw = 0;
break;
}
// entire amount must be accounted for
assert(sharesToWithdraw == 0);
emit WithdrawalsFinalized(_amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
9 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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