Summary
The WithdrawalPool::getBatchIds function returns the wrong batch ID for partially finalized withdrawals, preventing users from retrieving the correct batch ID needed for withdrawal.
Vulnerability Details
The WithdrawalPool::getBatchIds function is crucial for users to determine which batch a fully or partially finalized withdrawal ID belongs to. This batch ID is required to successfully withdraw using the WithdrawalPool::withdraw function. However, the current implementation of getBatchIds consistently returns the wrong batch ID when the withdrawal ID is only partially finalized.
The test below shows two implementations of getBatchIds: my recommended fix and the current, faulty implementation.
Please add the recommended getBatchIds fix from the "Recommendations" section as getBatchIdsMIne in the WithdrawalPool contract, then add the following test to test/core/priorityPool/withdrawal-pool.test.ts:
it('MINE withdraw should work correctly', async () => {
const { signers, accounts, withdrawalPool, token } = await loadFixture(deployFixture)
await withdrawalPool.queueWithdrawal(accounts[0], toEther(500))
await withdrawalPool.queueWithdrawal(accounts[1], toEther(200))
await withdrawalPool.queueWithdrawal(accounts[0], toEther(500))
await withdrawalPool.queueWithdrawal(accounts[1], toEther(300))
console.log(
'account0 WithdrawalIDs:',
await withdrawalPool.getWithdrawalIdsByOwner(accounts[0])
)
console.log(
'account1 WithdrawalIDs:',
await withdrawalPool.getWithdrawalIdsByOwner(accounts[1])
)
await withdrawalPool.deposit(toEther(500))
await withdrawalPool.deposit(toEther(500))
await withdrawalPool.deposit(toEther(100))
await withdrawalPool.deposit(toEther(100))
await withdrawalPool.deposit(toEther(100))
console.log('account0 BatchIds:', await withdrawalPool.getBatchIds([1, 3]))
console.log('account0 getBatchIdsMIne:', await withdrawalPool.getBatchIdsMIne([1, 3]))
console.log('account1 getBatchIds:', await withdrawalPool.getBatchIds([2, 4]))
console.log('account1 getBatchIdsMIne:', await withdrawalPool.getBatchIdsMIne([2, 4]))
let startingBalance = await token.balanceOf(accounts[0])
await withdrawalPool.withdraw([1], [1])
assert.equal(fromEther((await token.balanceOf(accounts[0])) - startingBalance), 500)
let startingBalance = await token.balanceOf(accounts[1])
await withdrawalPool.connect(signers[1]).withdraw([2], [2])
assert.equal(fromEther((await token.balanceOf(accounts[1])) - startingBalance), 200)
let startingBalance = await token.balanceOf(accounts[1])
await withdrawalPool.connect(signers[1]).withdraw([4], [5])
assert.equal(fromEther((await token.balanceOf(accounts[1])) - startingBalance), 100)
})
Run:
yarn test --grep "MINE withdraw should work correctly"
Here are the logs:
$ npx hardhat test --network hardhat --grep 'MINE withdraw should work correctly'
Compiled 1 Solidity file successfully (evm target: london).
WithdrawalPool
account0 WithdrawalIDs: Result(2) [ 1n, 3n ]
account1 WithdrawalIDs: Result(2) [ 2n, 4n ]
account0 BatchIds: Result(2) [ 1n, 4n ]
account0 getBatchIdsMIne: Result(2) [ 1n, 4n ]
account1 getBatchIds: Result(2) [ 2n, 0n ]
account1 getBatchIdsMIne: Result(2) [ 2n, 5n ]
✔ MINE withdraw should work correctly (10076ms)
1 passing (10s)
Done in 22.47s.
In the test, I created two queued withdrawals for account0 and two for account1, totaling 1,500 tokens in queued withdrawals. Then, I made deposits of up to 1,300 tokens, leaving the last queued withdrawal only partially finalized. The current implementation incorrectly returns a default batch ID of 0, while the fixed version correctly returns a batch ID of 5.
Impact
Users will be unable to retrieve batch IDs for partially finalized queued withdrawals, making it nearly impossible to withdraw from these partially finalized withdrawal Ids.
Tools Used
Manual Review
Recommendations
Update the current WithdrawalPool::getBatchIds function to:
function getBatchIds(
uint256[] memory _withdrawalIds
) public view returns (uint256[] memory) {
uint256[] memory batchIds = new uint256[]();
for (uint256 i = 0; i < _withdrawalIds.length; ++i) {
uint256 batchId;
uint256 withdrawalId = _withdrawalIds[i];
for (uint256 j = withdrawalBatchIdCutoff; j < withdrawalBatches.length; ++j) {
uint256 indexOfLastWithdrawal = withdrawalBatches[j].indexOfLastWithdrawal;
if (withdrawalId <= indexOfLastWithdrawal) {
batchId = j;
break;
}
if (
indexOfLastWithdrawal + 1 >= withdrawalId &&
queuedWithdrawals[withdrawalId].partiallyWithdrawableAmount != 0
) {
batchId = j;
continue;
}
}
batchIds[i] = batchId;
}
return batchIds;
}
