Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
howlbot-integration bot opened this issue Sep 20, 2024 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-b primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_68_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

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

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_68_group AI based duplicate group recommendation bug Something isn't working edited-by-warden primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 20, 2024
howlbot-integration bot added a commit that referenced this issue Sep 20, 2024
@d1ll0n
Copy link

d1ll0n commented Sep 30, 2024

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!

@d1ll0n d1ll0n added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Sep 30, 2024
@3docSec
Copy link

3docSec commented Oct 3, 2024

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

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 3, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2024

3docSec changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2024

3docSec marked the issue as grade-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-b primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_68_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants