A Sanctioned Borrower Can Close Markets Gracefully in WildCatMarket

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Description

The WildcatMarket contract allows sanctioned entities to close markets, violating the sponsor's invariant that sanctioned accounts should not modify the market state. The closeMarket() function lacks a sanction check, contrary to other state-modifying functions like borrow().

Let's take a look at the closeMarket() implementations from WildcatMarket.sol:

 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);

    // If there is a pending withdrawal batch which is not fully paid off, set aside
    // up to the available liquidity for that batch.
    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);
      // Reduce liquidity available to next batch
      availableLiquidity -= normalizedAmountPaid;
    }

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

    _writeState(state);
    emit_MarketClosed(block.timestamp);
  }
}

Impact

This lack of restriction allows sanctioned entities to interact with the market and modify its state by closing it, effectively circumventing financial regulations. This violates a key invariant stated in the guidelines:

"Accounts which are flagged as sanctioned on Chainalysis should never be able to successfully modify the state of the market unless the borrower specifically overrides their sanctioned status in the sentinel."

Proof of Concept

Unlike the deposit() and borrow() functions, which include the necessary check, a sanctioned entity can bypass the sanction check by directly calling the closeMarket() function.

‹‹Test output logs›› :

[PASS] test_SanctionedBorrowerCanCloseMarket() (gas: 456671)
Logs:
  Sanctioned borrower successfully closed the market

Create a new NewTest.t.sol file under ./test to run the POC code below with forge test --match-path test/NewTest.t.sol -vvvv

// SPDX-License-Identifier: Apache-2.0
pragma solidity >=0.8.20;

import './BaseMarketTest.sol';

contract WildcatMarketSanctionedCloseTest is BaseMarketTest {
    uint256 constant DEPOSIT_AMOUNT = 1000e18;

    function setUp() public override {
        parameters.annualInterestBips = 1000; // 10% APR
        parameters.maxTotalSupply = 1000000e18;
        parameters.reserveRatioBips = 1000; // 10% reserve ratio

        super.setUp();
        setUpContracts(false);
    }

    function test_SanctionedBorrowerCanCloseMarket() public {
        // Step 1: Deposit to the market
        _deposit(alice, DEPOSIT_AMOUNT);

        // Step 2: Sanction the borrower
        sanctionsSentinel.sanction(borrower);
        assertTrue(sanctionsSentinel.isSanctioned(borrower, borrower), "Borrower should be sanctioned");

        // Step 3: Attempt to close the market as the sanctioned borrower
        vm.startPrank(borrower);
        market.closeMarket();
        vm.stopPrank();

        // Step 4: Verify that the market was closed
        bool isClosed = market.isClosed();

        assertTrue(isClosed, "Market should have been closed");

        console.log("Sanctioned borrower successfully closed the market");
    }
}

Tools used

Foundry

Recommendation:

Implement a sanction check in the closeMarket() function, similar to the check in the borrow() function:

  if (_isFlaggedByChainalysis(borrower)) {
    revert_CloseMarketWhileSanctioned();
  }

Assessed type

Access Control

[laurenceday]

https://github.com/code-423n4/2024-08-wildcat-findings/issues/70

This warden is just playing word-games and filing highs without caring if protocol functionality is actually impinged upon.

We have to allow this: the alternative is that funds are in the void with a sanctioned borrower in perpetuity, constantly gaining interest. It's a legal issue in the sense that funds returned are going to have re-originated from a tainted party, but we aren't interested in taking the position as a protocol that your money is gone with zero recourse: as the warden has pointed out elsewhere, a sanctioned party could just repay from a different address anyway.

Afuera.

[c4-judge]

3docSec marked the issue as duplicate of #70

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b