The Wildcat Protocol - QiuhaoLi's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L154

Vulnerability details

Impact

closeMarket() doesn't account for possible future fees for delinquency, which can be a lose for lenders (although borrowers can send the fees after the market is closed, he/she can argue the market is closed and refuse to pay that).

Proof of Concept

In closeMarket(), we will send the difference between total debts and total assets from the borrower to the market:

  function closeMarket() external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0;
    if (_withdrawalData.unpaidBatches.length() > 0) {
      revert CloseMarketWithUnpaidWithdrawals();
    }
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld); <---
    }

The debts are accounted as normalized total supply, protocol fees, and unclaimed (paid) withdraws:

  function totalDebts(MarketState memory state) internal pure returns (uint256) {
    return
      state.normalizeAmount(state.scaledTotalSupply) +
      state.normalizedUnclaimedWithdrawals +
      state.accruedProtocolFees;
  }

However, even if the market isn't delinquent after the debts are paid, fees for delinquency can still exist depending on how long the market has been delinquent. If we only get the current total debts, the unclaimed delinquent fees can be a loss for lenders. E.g.:

1. Borrower Bob opens a market, and Alice is the lender.
2. The market has been delinquent for 30 days after the grace period.
3. Bob closes the market and pays all the debts, including the delinquent fees of 30 days.
4. However, there is still a 30-day fee for delinquency, but Bob doesn't pay it, arguing the market is closed.

Tools Used

Manual Review.

Recommended Mitigation Steps

1. Don't close the market if there are fees for delinquency.
2. Send all the debts and future feed for delinquency in closeMarket.

For 1, the patch can be:

diff --git a/src/market/WildcatMarket.sol b/src/market/WildcatMarket.sol
index e072c0f..8cde74c 100644
--- a/src/market/WildcatMarket.sol
+++ b/src/market/WildcatMarket.sol
@@ -148,6 +148,7 @@ contract WildcatMarket is
     if (_withdrawalData.unpaidBatches.length() > 0) {
       revert CloseMarketWithUnpaidWithdrawals();
     }
+    require(state.timeDelinquent <= delinquencyGracePeriod, "Delinquent fee is being collected.");
     uint256 currentlyHeld = totalAssets();
     uint256 totalDebts = state.totalDebts();
     if (currentlyHeld < totalDebts) {

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L142 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatArchController.sol#L8

Vulnerability details

Impact

We provide closeMarket() in WildMarket.sol for the borrower to close the market when they want to stop paying interest and return all outstanding debt. However, we didn't implement the relative call in WildMarketController.sol, which makes the method useless, and the borrower has to keep paying interest (assets loss).

Proof of Concept

The coseMarket() is only callable by the market controller:

  function closeMarket() external onlyController nonReentrant { <-----
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0;
    if (_withdrawalData.unpaidBatches.length() > 0) {
      revert CloseMarketWithUnpaidWithdrawals();
    }

However, there is no call to this method in the controller, making this method useless.

Tools Used

Manual Review.

Recommended Mitigation Steps

Add a call to closeMarket() in WildMarketController.sol:

diff --git a/src/WildcatMarketController.sol b/src/WildcatMarketController.sol
index 55f62f6..3a7a54a 100644
--- a/src/WildcatMarketController.sol
+++ b/src/WildcatMarketController.sol
@@ -513,4 +513,8 @@ contract WildcatMarketController is IWildcatMarketControllerEventsAndErrors {
       }
     }
   }
+
+  function closeMarket(address market) external onlyBorrower onlyControlledMarket(market) {
+    WildcatMarket(market).closeMarket();
+  }
 }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L134

Vulnerability details

Impact

The borrower can't increase the maximum supply, which may cause economic loss for the borrower/lenders or violate the offline law contract.

Proof of Concept

We provide a setMaxTotalSupply() method in WildcatMarketConfig.sol for the borrower to increase the max supply of a market:

  function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant { <---
    MarketState memory state = _getUpdatedState();

    if (_maxTotalSupply < state.totalSupply()) {
      revert NewMaxSupplyTooLow();
    }

    state.maxTotalSupply = _maxTotalSupply.toUint128();
    _writeState(state);
    emit MaxTotalSupplyUpdated(_maxTotalSupply);
  }

As we can see, it's only callable from the market controller. However, there is no such call in WildMarketController.sol, making this method useless.

Now consider:

1. Alice (the borrower) made an offline deal with lenders Bob and Frank that once the max supply reached for one month, she will increase it by 30%.
2. Since scaleFactor is increasing and Bob deposits a lot. The maximum supply is reached for one month.
3. Alice can't increase the max supply since it's not callable, violating the offline contract (and may pay a penalty offline).

Tools Used

Manual Review.

Recommended Mitigation Steps

diff --git a/src/WildcatMarketController.sol b/src/WildcatMarketController.sol
index 55f62f6..fd7ff3d 100644
--- a/src/WildcatMarketController.sol
+++ b/src/WildcatMarketController.sol
@@ -513,4 +513,9 @@ contract WildcatMarketController is IWildcatMarketControllerEventsAndErrors {
       }
     }
   }
+
+  function setMaxTotalSupply(address market, uint256 _maxTotalSupply) external onlyBorrower onlyControlledMarket(market) {
+    WildcatMarket(market).setMaxTotalSupply(_maxTotalSupply);
+  }
+
 }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketToken.sol#L54 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketToken.sol#L60

Vulnerability details

Impact

WildcatMarketToken will forbid tokens to transfer from or to a blocked/sanctioned address but doesn't forbid a blocked/sanctioned account from getting an allowance and using its allowance, i.e., interacting with WildcatMarketToken and trigger token transfers, which can lead to law issues (possible assets loss).

Proof of Concept

In WildMarketBase.sol:_getAccount(), we will revert if it's a blocked account:

  function _getAccount(address accountAddress) internal view returns (Account memory account) {
    account = _accounts[accountAddress];
    if (account.approval == AuthRole.Blocked) {
      revert AccountBlacklisted();
    }
  }

And in WildcatMarketToken.sol, we use the _getAccount for from/to address during transfer process:

  function _transfer(address from, address to, uint256 amount) internal virtual {
    MarketState memory state = _getUpdatedState();
    uint104 scaledAmount = state.scaleAmount(amount).toUint104();

    if (scaledAmount == 0) {
      revert NullTransferAmount();
    }

    Account memory fromAccount = _getAccount(from); <---
    fromAccount.scaledBalance -= scaledAmount;
    _accounts[from] = fromAccount;

    Account memory toAccount = _getAccount(to);    <---

So basically when an account is blocked/sanctioned, we forbid tokens to be transferred from or to the account address. However, in transferFrom(), we didn't check if the msg.sender is a blocked/sanctioned address:

  function transferFrom(
    address from,
    address to,
    uint256 amount
  ) external virtual nonReentrant returns (bool) {
    uint256 allowed = allowance[from][msg.sender];

    // Saves gas for unlimited approvals.
    if (allowed != type(uint256).max) {
      uint256 newAllowance = allowed - amount;
      _approve(from, msg.sender, newAllowance);
    }

    _transfer(from, to, amount);

So, a sanctioned account can still interact with WildcatMarketToken.transferFrom with its previous allowance.

To give a reference, tokens like USDT will check if the address is blacklisted in transferFrom:

    // Forward ERC20 methods to upgraded contract if this one is deprecated
    function transferFrom(address _from, address _to, uint _value) public whenNotPaused {
        require(!isBlackListed[_from]);
        if (deprecated) {
            return UpgradedStandardToken(upgradedAddress).transferFromByLegacy(msg.sender, _from, _to, _value);
        } else {
            return super.transferFrom(_from, _to, _value);
        }
    }

The following PoC shows that we can use transfer() directly for a blocked/sanctioned account, but it can use transferFrom for token transfers:

  // test/market/WildcatMarketConfig.t.sol:WildcatMarketConfigTest
  function test_sanction_transferFrom() external {
     _deposit(bob, 1e18);
     _deposit(alice, 1e18);

    sanctionsSentinel.sanction(bob);
    market.nukeFromOrbit(bob);
    assertEq(
      uint(market.getAccountRole(bob)),
      uint(AuthRole.Blocked),
      'account role should be Blocked'
    );

    startPrank(alice);
    IERC20(address(market)).approve(bob, 10000000);
    stopPrank();

    startPrank(bob);
    vm.expectRevert();
    IERC20(address(market)).transfer(address(0x1234), 10000000); // will revert in _transfer() with "AccountBlacklisted()"
    IERC20(address(market)).transferFrom(alice, address(0x1234), 10000000); // will success
  }

Output:

Running 1 test for test/market/WildcatMarketConfig.t.sol:WildcatMarketConfigTest
[PASS] test_sanction_transferFrom() (gas: 883736)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.49ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review.

Recommended Mitigation Steps

diff --git a/src/market/WildcatMarketToken.sol b/src/market/WildcatMarketToken.sol
index 5e53a0e..dcaeb6a 100644
--- a/src/market/WildcatMarketToken.sol
+++ b/src/market/WildcatMarketToken.sol
@@ -43,6 +43,8 @@ contract WildcatMarketToken is WildcatMarketBase {
   function transferFrom(
     address from,
     address to,
     uint256 amount
   ) external virtual nonReentrant returns (bool) {
+    require(_accounts[msg.sender].approval != AuthRole.Blocked);
+

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatSanctionsSentinel.sol#L95 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/interfaces/IWildcatSanctionsSentinel.sol#L71 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L172 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L166

Vulnerability details

Impact

1. escrow.account is wrongly set to the borrower. So a blocked lender's escrow assets can be released to the borrower while sanctioned. This is a direct assets loss for the lender (who should get the assets back after being unsanctioned) and an indirect loss for the borrower/other lenders (dirty money flows to them may make them sanctioned).
2. sanctionOverrides[borrower][escrowContract] is wrongly set to sanctionOverrides[account][escrowContract], so the escrow contract can be sanctioned and blocked later. But the borrower can use overrideSanction() to fix this.

Proof of Concept

In IWildcatSanctionsSentinel.sol we define createEscrow() as:

  function createEscrow(
    address account,
    address borrower,
    address asset
  ) external returns (address escrowContract);

And in WildcatMarketBase.sol:_blockAccount() and WildcatMarketWithdrawals.sol:executeWithdrawal(), we follow the definitions:

      if (scaledBalance > 0) {
        account.scaledBalance = 0;
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow( <-------
          accountAddress,
          borrower,
          address(this)
        );

......

    if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
      _blockAccount(state, accountAddress);
      address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow( <--------
        accountAddress,
        borrower,
        address(asset)
      );

However, in WildcatSanctionsSentinel.sol:createEscrow(), we wrongly reversed the order of borrower and accountAddress:

  function createEscrow(
    address borrower,
    address account,
    address asset
  ) public override returns (address escrowContract) {
    if (!IWildcatArchController(archController).isRegisteredMarket(msg.sender)) {
      revert NotRegisteredMarket();
    }

    escrowContract = getEscrowAddress(borrower, account, asset);

    if (escrowContract.codehash != bytes32(0)) return escrowContract;

    tmpEscrowParams = TmpEscrowParams(borrower, account, asset); <------

    new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }();

    emit NewSanctionsEscrow(borrower, account, asset);

    sanctionOverrides[borrower][escrowContract] = true;   <--------

So escrow.account is wrongly set to the borrower, which means canReleaseEscrow() will return true and the assets will be released to the borrower. Also, and sanctionOverrides[borrower][escrowContract] is wrongly set to sanctionOverrides[account][escrowContract], the borrower can use overrideSanction() to fix this.

The following PoC shows escrow assets of bob are wrongly released the borrower even bob is stationed:

  // test/market/WildcatMarketConfig.t.sol
  import 'forge-std/console.sol';
  function test_createEscrow_fail() external {
     _deposit(bob, 1e18);
    sanctionsSentinel.sanction(bob);
    market.nukeFromOrbit(bob);
    assertEq(
      uint(market.getAccountRole(bob)),
      uint(AuthRole.Blocked),
      'account role should be Blocked'
    );
    WildcatSanctionsSentinel sentinel = WildcatSanctionsSentinel(market.sentinel());
    address expect_escrowContract = sentinel.getEscrowAddress(market.borrower(), bob, address(market));
    address wrong_escrowContract = sentinel.getEscrowAddress(bob, market.borrower(), address(market));
    console.log("expect_escrowContract.codehash = %x", uint256(expect_escrowContract.codehash));
    console.log("wrong_escrowContract.codehash  = %x", uint256(wrong_escrowContract.codehash));
    console.log("wrong_escrowContract.account == bob: %s", WildcatSanctionsEscrow(wrong_escrowContract).account() == bob);
    console.log("wrong_escrowContract.account == borrower: %s", WildcatSanctionsEscrow(wrong_escrowContract).account() == market.borrower());
    console.log("assets can be released to the **borrower while bob being sanctioned**: ");
    console.log("wrong_escrowContract.canReleaseEscrow = %s", WildcatSanctionsEscrow(wrong_escrowContract).canReleaseEscrow());
    uint256 borrower_balance_before_release = IERC20(address(market)).balanceOf(market.borrower());
    WildcatSanctionsEscrow(wrong_escrowContract).releaseEscrow();
    uint256 borrower_balance_after_release = IERC20(address(market)).balanceOf(market.borrower());
    console.log("borrower get market tokens from bob's wrong_escrowContract = %s", borrower_balance_after_release - borrower_balance_before_release);
  }

Output:

Running 1 test for test/market/WildcatMarketConfig.t.sol:WildcatMarketConfigTest
[PASS] test_createEscrow_fail() (gas: 746081)
Logs:
  expect_escrowContract.codehash = 0x0
  wrong_escrowContract.codehash  = 0x15a581ca1b307ec9f5718c7472e92635eb6aba398642962be11b5d682a08f0d
  wrong_escrowContract.account == bob: false
  wrong_escrowContract.account == borrower: true
  assets can be released to the **borrower while bob being sanctioned**: 
  wrong_escrowContract.canReleaseEscrow = true
  borrower get market tokens from bob's wrong_escrowContract = 1000000000000000000

Tools Used

Manual Review.

Recommended Mitigation Steps

   function createEscrow(
-    address borrower,
     address account,
+    address borrower,
     address asset
   ) public override returns (address escrowContract) {
     if (!IWildcatArchController(archController).isRegisteredMarket(msg.sender)) {

Assessed type

Context

Judge has assessed an item in Issue #481 as 2 risk. The relevant finding follows:

A blocked/sanctioned account can still received interest