Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarketBase.sol#L676

Vulnerability details

Proof of Concept

When partially repaying an unpaid batch, the current availableLiquidity is passed as an argument to the _applyWithdrawalBatchPayment

  function _applyWithdrawalBatchPayment(
    WithdrawalBatch memory batch,
    MarketState memory state,
    uint32 expiry,
    uint256 availableLiquidity
  ) internal returns (uint104 scaledAmountBurned, uint128 normalizedAmountPaid) {
    uint104 scaledAmountOwed = batch.scaledTotalAmount - batch.scaledAmountBurned;

    // Do nothing if batch is already paid
    if (scaledAmountOwed == 0) return (0, 0);

    uint256 scaledAvailableLiquidity = state.scaleAmount(availableLiquidity);
    scaledAmountBurned = MathUtils.min(scaledAvailableLiquidity, scaledAmountOwed).toUint104();
    // Use mulDiv instead of normalizeAmount to round `normalizedAmountPaid` down, ensuring
    // it is always possible to finish withdrawal batches on closed markets.
    normalizedAmountPaid = MathUtils.mulDiv(scaledAmountBurned, state.scaleFactor, RAY).toUint128();

The way it works is that it turns the current availableLiquidity into a scaledAmount and uses the lower of it and the scaledAmountOwed.

The problem lies when the repayment made is partial and scaledAvailableLiquidity <= scaledAmountOwed.

In this case, it must be noted that state.scaleAmount rounds to the nearest number - meaning that if the equivalent scale amount usually should be X.5, it will be rounded to X + 1. While this would not usually a problem, it becomes a problem when the added 0.5 shares wei due to rounding up, make up for more than 1 normalized token wei. (Or in other words - when the scaling factor exceeds 2).

In this case, whenever a partial repayment is made and it is rounded up, the normalized amount added to the batch will be larger than what was really sent to the contract. This would cause a race condition between the lenders, as the last one would always not be able to withdraw, due to underflow.

This also breaks a key protocol invariant that the contract should always hold enough funds to cover for all the unclaimed paid withdraws.

Attaching PoC Below Note: in order to run, I've commented out the _checkState function in _deposit

  function test_roundingIssue() external {
    vm.prank(address(hooksFactory));
    market.setProtocolFeeBips(0);       // protocol rate to 0 for simplicity
    _deposit(alice, 10e18);

    vm.prank(borrower);
    market.setAnnualInterestAndReserveRatioBips(10000, 0);

    fastForward(119 weeks);             // forwarding 119 weeks so scale factor is around 3.

    vm.prank(alice);
    uint32 expiry = market.queueFullWithdrawal();

    fastForward(1 days + 1);          // skipping day + 1, so we can execute the withdraw
    vm.startPrank(borrower);
    asset.mint(borrower, 100e18);
    asset.approve(address(market), 100e18);

    market.repayAndProcessUnpaidWithdrawalBatches(5, 2);
    market.repayAndProcessUnpaidWithdrawalBatches(5, 2);

    vm.expectRevert();
    market.executeWithdrawal(alice, expiry);
  }

Impact

For markets where the scale factor exceeds 2, repaying unpaid batches will be broken and there will always be a user who cannot fully withdraw.

Recommended Mitigation Steps

add the following line:

normalizedAmountPaid = Math.min(normalizedAmountPaid, availableLiquidity);

Assessed type

Context

code-423n4 / 2024-08-wildcat-findings