Submitted by 0xCiphky
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L137
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L77
The Wildcat protocol utilizes a withdrawal cycle where lenders call   queueWithdrawals which then goes through a set amount of time (withdrawal duration period) before a withdrawal can be executed (if the protocol has enough funds to cover the withdrawal). Withdrawal requests that could not be fully honored at the end of their withdrawal cycle are batched together, marked as expired withdrawals, and added to the withdrawal queue. These batches are tracked using the time of expiry, and when assets are returned to a market with a non-zero withdrawal queue, assets are immediately routed to the unclaimed withdrawals pool and can subsequently be claimed by lenders with the oldest expired withdrawals first.
The withdrawalBatchDuration can be set to zero so lenders do not have to wait before being able to withdraw funds from the market; however, this can cause issues where lenders in a batch can withdraw more than their pro-rata share of the batchs paid assets.
A lender calls queueWithdrawal first to initiate the withdrawal; this will place it in a batch respective to its expiry:
Now once the withdrawalBatchDuration has passed, a lender can call executeWithdrawal to finalize the withdrawal. This will grab the batch and let the lender withdraw a percentage of the batch if the batch is not fully paid or all funds if it is fully paid.
Lets look at how this percentage is determined: the newTotalWithdrawn function determines a lenders available withdrawal amount by multiplying the normalizedAmountPaid with the scaledAmount and dividing the result by the batchs scaledTotalAmount. This ensures that each lender in the batch can withdraw an even amount of the available funds in the batch depending on their scaledAmount.
This works fine when withdrawalBatchDuration is set over zero, as the batch values (except normalizedAmountPaid) are finalized. However, when set to zero, we can end up with lenders in a batch being able to withdraw more than normalizedAmountPaid in that batch, potentially violating protocol invariants.
Consider the following scenario:
There is only 5 tokens available to burn.
Lender A calls queueWithdrawal with 5 and executeWithdrawal instantly.
Lender A was able to fully withdraw.
Lender B comes along and calls queueWithdrawal with 5 and executeWithdrawal instantly in the same block. This will add to the same batch as lender A as it is the same expiry.
Now lets look at newTotalWithdrawn for Lender B:
Lets see what the batch looks like now:
This will break the following invariant in the protocol:
It will also mean that funds reserved for other batches may not be able to be fulfilled even if the batchs normalizedAmountPaid number shows that it should be able to.
For the following test, make sure you use the following parameters in ExpectedStateTracker:
Foundry
Review the protocols withdrawal mechanism and consider adjusting the behaviour of withdrawals when withdrawalBatchDuration is set to zero to ensure that lenders cannot withdraw more than their pro-rata share of the batchs paid assets.
d1ll0n (Wildcat) confirmed and commented:
laurenceday (Wildcat) commented:
