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

[WithdrawalPool.sol] Prevent efficient return of data in getBatchIds() by blocking updateWithdrawalBatchIdCutoff() update of newWithdrawalIdCutoff

jsondoge

Summary

[WithdrawalPool.sol] updateWithdrawalBatchIdCutoff() is used to update values withdrawalIdCutoff and withdrawalBatchIdCutoff . withdrawalBatchIdCutoff is used in getBatchIds and getFinalizedWithdrawalIdsByOwner view functions to more efficiently return data without iterating over all batches. By purposefully or by some innocent staker forgetting to call withdraw() on their queuedWithdrawal the updateWithdrawalBatchIdCutoff() will no longer update the withdrawalIdCutoff and withdrawalBatchIdCutoff because there will be a queuedWithdrawal with smaller index and still left with balance.

Vulnerability Details

updateWithdrawalBatchIdCutoff() is used for updating withdrawalIdCutoff and withdrawalBatchIdCutoff to increase view function efficiency. It has a loop:

...
// 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) { // <- breaks on first queuedWithdrawal with any balance
break;
}
}
...

Because it depends on queuedWithdrawals, it is possible to permanently halt the optimization by simply not finalizing own withdrawal.

The only place that clears queuedWithdrawals balance is the withdraw() function:

...
delete queuedWithdrawals[withdrawalId];
delete withdrawalOwners[withdrawalId];
} else {
amountToWithdraw += withdrawal.partiallyWithdrawableAmount;
queuedWithdrawals[withdrawalId].partiallyWithdrawableAmount = 0;
}
...

But before that it has a check:

if (withdrawalOwners[withdrawalId] != owner) revert SenderNotAuthorized(); // <- only true owner can call

Because only the true owner can finalize and clear the balance once the malicious actor or forgetful staker successfully queues their withdrawal and not clears it, it will cause the optimization function to stop increasing the stored cutoff variables.

Updated existing hardhat test for withdrawals and checked withdrawalBatchIdCutoff and withdrawalIdCutoff values

it('once queuedWithdrawal is not called with withdraw() it is the highest index for updateWithdrawalBatchIdCutoff()', async () => {
const { signers, accounts, withdrawalPool, token } = await loadFixture(deployFixture)
​
await withdrawalPool.queueWithdrawal(accounts[0], toEther(1000)) // <- will not be withdrawan - index 1 (because 0 is by default stored)
await withdrawalPool.queueWithdrawal(accounts[1], toEther(250))
await withdrawalPool.queueWithdrawal(accounts[0], toEther(500))
await withdrawalPool.deposit(toEther(1200))
​
await expect(withdrawalPool.withdraw([1, 2, 3], [1, 1, 0])).to.be.revertedWithCustomError(
withdrawalPool,
'SenderNotAuthorized()'
)
await expect(withdrawalPool.withdraw([1, 3], [1, 1])).to.be.revertedWithCustomError(
withdrawalPool,
'InvalidWithdrawalId()'
)
​
await withdrawalPool.deposit(toEther(550))
​
await expect(withdrawalPool.withdraw([1], [2])).to.be.revertedWithCustomError(
withdrawalPool,
'InvalidWithdrawalId()'
)
​
let startingBalance = await token.balanceOf(accounts[1])
await withdrawalPool.connect(signers[1]).withdraw([2], [2])
assert.equal(fromEther((await token.balanceOf(accounts[1])) - startingBalance), 250)
assert.deepEqual(
(await withdrawalPool.getWithdrawalIdsByOwner(accounts[1])).map((id) => Number(id)),
[]
)
assert.deepEqual(
(await withdrawalPool.getWithdrawals([2])).map((d: any) => [
fromEther(d[0]),
fromEther(d[1]),
]),
[[0, 0]]
)
​
startingBalance = await token.balanceOf(accounts[0])
await withdrawalPool.withdraw([3], [2]);
​
// account[0] did not withdraw its first queueWithdrawal
// updateWithdrawalBatchIdCutoff will not be able to update the cutoffs beyond it's index
​
await withdrawalPool.updateWithdrawalBatchIdCutoff()
​
console.log(await withdrawalPool.withdrawalBatchIdCutoff()) // <- returns 0
console.log(await withdrawalPool.withdrawalIdCutoff()) // <- returns 1 (matching leftover queuedWithdrawal index)
})

Impact

The getBatchIds and getFinalizedWithdrawalIdsByOwner view functions will increasingly consume more and more computational resources to be called due to increasing iteration count. If the project uses 3rd party nodes this will cause the qoutas to increase and after longer periods of time can reach limits. If own node is used it will cause increasing computational pressure.

Tools Used

Manual review + hardhat test.

Recommendations

Few possible workarounds:

  • Allow WithdrawalPool owner to forcefully withdraw a queuedWithdrawal if it's been pending for a long time

  • Refactor getBatchIds to not iterate through all withdrawalBatches, but rather use some form of mapping get constant performance

Notes

Although this issue is somewhat mentioned in the LightChaser as "permanent Denial of Service (DoS) vulnerability", it is likely that the developers were hoping to solve it by using these stored cutoff variables. Which as described here are still open to circumvention.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

M-1 Cyfrin not properly fixed - if someone forgets to withdraw the withdrawalBatches array is still ever increasing

Support

FAQs

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