Queued withdrawal execution fails when not preceded by an adequate number of "processUnpaidWithdrawalBatch" calls

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L84 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L190

Vulnerability details

Whenever a WildcatMarket receives a withdrawal request it cannot process, it will enqueue it in the unpaidBatches FIFO queue.

Any lender who queued a withdrawal request will see their withdrawal execution fail, unless batches prior to theirs get processed, possibly at their own expense via repeated processUnpaidWithdrawalBatch calls.

Since there is no lower limit to how much a lender can request to withdraw, once the market has no funds to cover withdrawals, a lender could enqueue requests of withdrawing as little as 1 wei. If the market is configured with short enough withdrawal periods, these requests can make the unpaidBatches queue grow arbitrarily large.

Impact

There are two impacts on this finding:

• when there is batch backlog, executeWithdrawals will fail despite being covered, unless the lender repeatedly calls processUnpaidWithdrawalBatch to crunch the backlog
• the above requirement creates the opportunity for gas grieving attacks i.e. via a large number of batches of 1 wei requests

Proof of Concept

The below PoC shows how a covered batch fails to be executed without a leading array of processUnpaidWithdrawalBatch calls:

    function testQueuedWithdrawal() public {
        // we start with a market with 0 reserve ratio
        // so it's easy to queue up withdrawal requests
        // and 0 as withdrawal batch duration

        // lender deposits
        vm.startPrank(lender);
        uint256 amt = 10e18;

        token.mint(lender, amt);
        token.approve(address(market), amt);

        market.deposit(amt);

        // borrower borrows everything
        vm.startPrank(borrower);
        market.borrow(amt);

        // lenders try to withdraw, obviously building up a queue
        // note that the lenders below may be different entities
        // and the queue may be arbitrarily long
        vm.startPrank(lender);
        uint t0 = block.timestamp;
        market.queueWithdrawal(1e18);

        vm.warp(t0+1);
        market.queueWithdrawal(1e18);

        vm.warp(t0+2);
        market.queueWithdrawal(1e18);

        // now the borrower gives back enough tokens to cover the first two batches
        token.mint(address(market), 2e18);

        // but the lender still can't withdraw batch 2
        vm.expectRevert();
        market.executeWithdrawal(lender, uint32(t0+1));

        // ... unless they take charge of the backlog processing
        market.processUnpaidWithdrawalBatch();
        market.processUnpaidWithdrawalBatch();

        market.executeWithdrawal(lender, uint32(t0+1));
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider mitigating the problem on two fronts:

• limiting the practically attainable batch queue size by:
  • adding a maximum rate and/or minimum amount and/or a symbolic fee to withdrawal requests available to lenders
  • adding a global minimum of i.e. 1 day for the length of withdrawal batches
• embedding a processUnpaidWithdrawalBatch loop in execution requests, so, once the queue processing cost is at bay, it becomes an automatic action

Assessed type

DoS

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-sponsor]

d1ll0n (sponsor) acknowledged

[MarioPoneder]

Since there is a queue per market where the participating parties are legally bound by an off-chain contract, therefore the risk of gas griefing negligible. Other than that, there are no malicious impacts.
Nevertheless, this heavily impedes the UX. An excellent QA finding.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b

[c4-judge]

MarioPoneder marked the issue as grade-a

[3docSec]

Hi @MarioPoneder I would like to ask you to kindly reconsider the downgrading of this finding:

Since there is a queue per market where the participating parties are legally bound by an off-chain contract, therefore the risk of gas griefing negligible.

There are two aspects I would bring into consideration:

• if we stay on the technical level, the impact on availability to withdrawal is real, and reporting it in our technical report seems fair territory to me for this reason. Business considerations on off-chain agreements are a solid argument for the sponsor's decision to accept or mitigate the risk though. Issue 497 is an example where the technical finding (being able to lower the interest) took precedence over business considerations (how much a >90% reserve rate can make sense?)
• if we consider the business level, too, we can't really say that lenders are completely trusted, because of the sanctions mechanism that is in place just for the very nasty ones among them

Thanks!

[MarioPoneder]

Thank you for reaching out, those are interesting aspects.
Considering only the technical level: The present griefing attack cannot be done by anyone except lenders that do hold market tokens, therefore the likelihood & impact is minimized.
Considering only the business level: Although unlikely, a reserve rate > 90% could be legitimated by an off-chain contract (concerning the comparison with #497), while griefing is not a valid business case. Furthermore, sanctions are a risk borrower & lender parties have to bear and the protocol offers an escrow mechanism to mitigate it.

Therefore, QA status is maintained, but definitely grade A.
Thanks for your understanding.