The contract goes back to being in a Delinquent state 1 seconds after repaying all DelinquentDebt because of a wrong repayment flow

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L186-L191 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L202-L214 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L226-L287

Vulnerability details

Impact

In the current implementation of the contract's repayOutstandingDebt() function, when a borrower repays their delinquent debt, the contract enters a delinquent state again just seconds after repayment due to improper handling of batched withdrawals. The repayment logic does not process pending withdrawals, resulting in the borrower incurring more debt despite having repaid their outstanding balance. This is due to batched and expired withdrawals not being accounted for during repayment, leading to the accumulation of additional debt.

The closeMarket() function, on the other hand, correctly handles pending withdrawals by updating the market state and processing withdrawals before marking the market as closed. This behavior should be replicated in the debt repayment function.

Proof of Concept

The contract does not account for batched withdrawals during debt repayment, as seen in this test:

 function repayDelinquentDebt() external nonReentrant sphereXGuardExternal {
    MarketState memory state = _getUpdatedState();
    uint256 delinquentDebt = state.liquidityRequired().satSub(totalAssets());
    _repay(state, delinquentDebt, 0x04);
    _writeState(state);
  }

The issue arises because pending withdrawals and fees are pay immediately the borrower repaid. After calling _repay(), the system continues to accumulate debt from these unprocessed batches. This is handled correctly in the closeMarket() function, where the contract processes all pending and expired withdrawals before finalizing the market closure.

function liquidityRequired(
    MarketState memory state
  ) internal pure returns (uint256 _liquidityRequired) {
 @audit uses new scale factor because it is not yet paid>>    uint256 scaledWithdrawals = state.scaledPendingWithdrawals;
    uint256 scaledRequiredReserves = (state.scaledTotalSupply - scaledWithdrawals).bipMul(
      state.reserveRatioBips
    ) + scaledWithdrawals;
    return

@audit uses new scale factor because it is not yet paid>>       state.normalizeAmount(scaledRequiredReserves) +
      state.accruedProtocolFees +
      state.normalizedUnclaimedWithdrawals;
  }

 function closeMarket() external onlyBorrower nonReentrant sphereXGuardExternal {
    MarketState memory state = _getUpdatedState();

    if (state.isClosed) revert_MarketAlreadyClosed();

    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from borrower
      uint256 remainingDebt = totalDebts - currentlyHeld;
      _repay(state, remainingDebt, 0x04);
      currentlyHeld += remainingDebt;
    } else if (currentlyHeld > totalDebts) {
      uint256 excessDebt = currentlyHeld - totalDebts;
      // Transfer excess assets to borrower
      asset.safeTransfer(borrower, excessDebt);
      currentlyHeld -= excessDebt;
    }
    hooks.onCloseMarket(state);
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 10000;
    // Ensures that delinquency fee doesn't increase scale factor further
    // as doing so would mean last lender in market couldn't fully redeem
    state.timeDelinquent = 0;

    // Still track available liquidity in case of a rounding error
    uint256 availableLiquidity = currentlyHeld -
      (state.normalizedUnclaimedWithdrawals + state.accruedProtocolFees);

@audit >> update withdrawal after transfer>>     // If there is a pending withdrawal batch which is not fully paid off, set aside
@audit >>    // up to the available liquidity for that batch.
@audit >>    if (state.pendingWithdrawalExpiry != 0) {
      uint32 expiry = state.pendingWithdrawalExpiry;
      WithdrawalBatch memory batch = _withdrawalData.batches[expiry];
      if (batch.scaledAmountBurned < batch.scaledTotalAmount) {
        (, uint128 normalizedAmountPaid) = _applyWithdrawalBatchPayment(
          batch,
          state,
          expiry,
          availableLiquidity
        );
        availableLiquidity -= normalizedAmountPaid;
        _withdrawalData.batches[expiry] = batch;
      }
    }

    uint256 numBatches = _withdrawalData.unpaidBatches.length();
@audit >>    for (uint256 i; i < numBatches; i++) {
      // Process the next unpaid batch using available liquidity
      uint256 normalizedAmountPaid = _processUnpaidWithdrawalBatch(state, availableLiquidity);
      // Reduce liquidity available to next batch
      availableLiquidity -= normalizedAmountPaid;
    }

    if (state.scaledPendingWithdrawals != 0) {
      revert_CloseMarketWithUnpaidWithdrawals();
    }

@audit >>update now ok to write>>    _writeState(state);
    emit_MarketClosed(block.timestamp);
  }

Tool Used

Manual Code analysis

Mitigation Steps

Suggested Fix The repayment function should follow the same flow as the closeMarket() function to ensure that all batched withdrawals are processed before marking the debt as cleared. This prevents the borrower from incurring more debt after repayment.

Fix: Refactor repayOutstandingDebt() to process pending withdrawals

function repayOutstandingDebt() external nonReentrant sphereXGuardExternal {
    MarketState memory state = _getUpdatedState();  // Fetch the current state

    uint256 outstandingDebt = state.totalDebts().satSub(totalAssets());
    _repay(state, outstandingDebt, 0x04);  // Repay the debt

    uint256 availableLiquidity = totalAssets();

    // Process pending withdrawal batches
    if (state.pendingWithdrawalExpiry != 0) {
        uint32 expiry = state.pendingWithdrawalExpiry;
        WithdrawalBatch memory batch = _withdrawalData.batches[expiry];
        if (batch.scaledAmountBurned < batch.scaledTotalAmount) {
            (, uint128 normalizedAmountPaid) = _applyWithdrawalBatchPayment(
                batch,
                state,
                expiry,
                availableLiquidity
            );
            availableLiquidity -= normalizedAmountPaid;
            _withdrawalData.batches[expiry] = batch;
        }
    }

    uint256 numBatches = _withdrawalData.unpaidBatches.length();
    for (uint256 i; i < numBatches; i++) {
        // Process the next unpaid batch using available liquidity
        uint256 normalizedAmountPaid = _processUnpaidWithdrawalBatch(state, availableLiquidity);
        availableLiquidity -= normalizedAmountPaid;
    }

    // Update market state to ensure no additional debt accrues
    _writeState(state);

    // Emit repayment event
    emit DebtRepaid(msg.sender, outstandingDebt);
}

Key Changes:

1. Pending Withdrawals Processing: The function now processes all pending and batched withdrawals, reducing the available liquidity accordingly.
2. Available Liquidity: The total assets held by the contract are used to settle any pending withdrawal batches.
3. State Update: The market state is updated once all withdrawals are processed, preventing the borrower from incurring additional debt.

By processing pending withdrawals in the repayOutstandingDebt() function, the contract ensures that the borrower does not incur additional debt after repayment. This follows the correct logic already present in the closeMarket() function, thereby aligning the two processes and preventing any inconsistency in the debt repayment flow.

Assessed type

Error

[d1ll0n]

This is a valid comment, but it isn't something we're even able to include in this function due to size limits.

I consider this low/informational as it doesn't impact the functionality of the protocol in any way (other than to make this function not very useful in a lot of cases). If the borrower has unpaid batches they need to pay off, they'll use the repayAndProcess function. Also even without any unpaid or pending withdrawal batches, if the APR is >0 the market will immediately go into delinquency one second after repayDelinquentDebt if even 1 wei of interest accrues. To me this is more an argument to just delete these functions than a vulnerability.

Appreciate the notes!

[3docSec]

Marking as QA because the reported scenario assumes the borrower calls the wrong function - repayOutstandingDebt instead of repayAndProcess

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b