The Wildcat Protocol - Toshii's results

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/market/WildcatMarketConfig.sol#L134

Vulnerability details

Impact

Core functions of the WildcatMarket contract can never be called (such as WildcatMarket:closeMarket or WildcatMarketConfig:setMaxTotalSupply) due to them being protected by the onlyController modifier, while the WildcatMarketController contract doesn't actually have any logic which can actually call these functions. This ultimately means that certain core logic can never be used.

Proof of Concept

The WildcatMarket:closeMarket function is implemented as follows:

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);
  } else if (currentlyHeld > totalDebts) {
    // Transfer excess assets to borrower
    asset.safeTransfer(borrower, currentlyHeld - totalDebts);
  }
  _writeState(state);
  emit MarketClosed(block.timestamp);
}

It uses the onlyController modifier which is defined as follows:

modifier onlyController() {
  if (msg.sender != controller) revert NotController();
  _;
}

This is then fundamentally a problem, as the controller does not have any function that can, for example, call WildcatMarket:closeMarket. Additionally, it's clear that they intended the controller to be the WildcatMarketController based on the following tests: BaseMarketTest and Test.

Tools Used

Manual review

Recommended Mitigation Steps

For all onlyController protected functions of the WildcatMarket contract, ensure that there is an associated function on the WildcatMarketController contract which can call them.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L361-L376 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L466-L492

Vulnerability details

Impact

Until the exact timestamp that a payment is processed for a given withdrawal batch (batch.scaledAmountBurned incremented, batch.normalizedAmountPaid incremented), the underlying amount of that batch should continue to be earning interest. It's only when the payment is actually made that the underlying amount of the batch should stop earning interest - at this point users in that batch are actually able to withdraw funds. This is clear with the general flow of logic of how _applyWithdrawalBatchPayment is being called.

This flow is not being followed when a batch of withdrawals first expires however. Instead of getting paid interest up until the current block.timestamp, they are only paid interest up to the timestamp of the expiry. This means that (assuming theres enough assets currently in the market to pay off that batch of withdrawals) that users in that batch are unfairly losing out on (block.timestamp - expiry) duration of interest. As this duration increases, those lenders are losing increasing amounts of interest.

Proof of Concept

The logic for handling a withdrawal batch when it first expires occurs in the WildcatMarketBase:_getUpdatedState function, which is defined as follows:

function _getUpdatedState() internal returns (MarketState memory state) {
  state = _state;
  // Handle expired withdrawal batch
  if (state.hasPendingExpiredBatch()) {
    uint256 expiry = state.pendingWithdrawalExpiry;
    // Only accrue interest if time has passed since last update.
    // This will only be false if withdrawalBatchDuration is 0.
    if (expiry != state.lastInterestAccruedTimestamp) {
      (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
        .updateScaleFactorAndFees(
          protocolFeeBips,
          delinquencyFeeBips,
          delinquencyGracePeriod,
          expiry // @issue
        );
      emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
    }
    _processExpiredWithdrawalBatch(state);
  }
  ...
}

Effectively, the market parameters related to the accrued interest owed to lenders (most important of which is state.scaleFactor) is updated in the state.updateScaleFactorAndFees(..) call, which we can see updates the interest up to the expiry timestamp. Let's consider a scenario where state.lastInterestAccruedTimestamp = N, expiry=N, and block.timestamp=N+86_400 (one day). At this point in time, all lenders in this market (including lenders in this expiring withdrawal batch) are owed interest for N+86_400-N = 86_400, which is the duration from the current block.timestamp to the previous timestamp that interest was accrued.

Considering this, in the above function call, (expiry != state.lastInterestAccruedTimestamp)=False and so it is skipped. However, even if expiry=N+1 for example, and it is not skipped, there is still a fundamental issue. For simplicity, I'm just assuming expiry=N. Next, _processExpiredWithdrawalBatch is called, which is defined as follows:

function _processExpiredWithdrawalBatch(MarketState memory state) internal {
  uint32 expiry = state.pendingWithdrawalExpiry;
  WithdrawalBatch memory batch = _withdrawalData.batches[expiry];

  // Burn as much of the withdrawal batch as possible with available liquidity.
  uint256 availableLiquidity = batch.availableLiquidityForPendingBatch(state, totalAssets());
  if (availableLiquidity > 0) {
    _applyWithdrawalBatchPayment(batch, state, expiry, availableLiquidity);
  }
  ...
}

Here, let's assume that the borrower has left enough assets in the Market so that the entire withdrawal batch which just expired can be paid out in full. This logic is implemented in the _applyWithdrawalBatchPayment function, which is defined as follows:

function _applyWithdrawalBatchPayment(
  WithdrawalBatch memory batch,
  MarketState memory state,
  uint32 expiry,
  uint256 availableLiquidity
) internal {
  uint104 scaledAvailableLiquidity = state.scaleAmount(availableLiquidity).toUint104();
  uint104 scaledAmountOwed = batch.scaledTotalAmount - batch.scaledAmountBurned;
  // Do nothing if batch is already paid
  if (scaledAmountOwed == 0) {
    return;
  }
  uint104 scaledAmountBurned = uint104(MathUtils.min(scaledAvailableLiquidity, scaledAmountOwed));
  uint128 normalizedAmountPaid = state.normalizeAmount(scaledAmountBurned).toUint128();

  batch.scaledAmountBurned += scaledAmountBurned;
  batch.normalizedAmountPaid += normalizedAmountPaid;
  state.scaledPendingWithdrawals -= scaledAmountBurned;

  // Update normalizedUnclaimedWithdrawals so the tokens are only accessible for withdrawals.
  state.normalizedUnclaimedWithdrawals += normalizedAmountPaid;
  ...
}

The important thing to note here is that the normalizedAmountPaid is based on referencing the current state.scaleFactor. However, this state.scaleFactor does not include the 86_400 seconds of interest which is actually owed to the lenders in this withdrawal batch. Rather, they are being cheated out of this interest entirely.

Tools Used

Manual review

Recommended Mitigation Steps

It is not logical that the most recent expired batch only get paid interest up to the expiry. Instead they should be getting the full amount of interest up to the amount of time they are paid. The WildcatMarketBase:_getUpdatedState function should be changed accordingly:

function _getUpdatedState() internal returns (MarketState memory state) {
  state = _state;

  if (block.timestamp != state.lastInterestAccruedTimestamp) {
    (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
      .updateScaleFactorAndFees(
        protocolFeeBips,
        delinquencyFeeBips,
        delinquencyGracePeriod,
        block.timestamp
      );
    emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
  }

  // Handle expired withdrawal batch
  if (state.hasPendingExpiredBatch()) {
    _processExpiredWithdrawalBatch(state);
  }
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatMarketController.sol#L468-L488

Vulnerability details

Impact

Any market which is created is tied to a specific controller, which has specific allowable ranges for core market parameters, such as minimumAnnualInterestBips. There is currently an issue where when the borrower calls the setAnnualInterestBips function of the WildcatMarketController, they can effectively circumvent the minimumAnnualInterestBips and set a new interest rate to any value, including 0. This effectively breaks the constraints of the WildcatMarketController and is invalid.

Proof of Concept

When the borrower wants to change the interest rate for a given market, they call WildcatMarketController:setAnnualInterestBips which is defined as follows:

function setAnnualInterestBips(
  address market,
  uint16 annualInterestBips
) external virtual onlyBorrower onlyControlledMarket(market) {
  // If borrower is reducing the interest rate, increase the reserve
  // ratio for the next two weeks.
  if (annualInterestBips < WildcatMarket(market).annualInterestBips()) {
    TemporaryReserveRatio storage tmp = temporaryExcessReserveRatio[market];

    if (tmp.expiry == 0) {
      tmp.reserveRatioBips = uint128(WildcatMarket(market).reserveRatioBips());

      // Require 90% liquidity coverage for the next 2 weeks
      WildcatMarket(market).setReserveRatioBips(9000);
    }

    tmp.expiry = uint128(block.timestamp + 2 weeks);
  }

  WildcatMarket(market).setAnnualInterestBips(annualInterestBips);
}

This then makes a call to WildcatMarketConfig:setAnnualInterestBips:

function setAnnualInterestBips(uint16 _annualInterestBips) public onlyController nonReentrant {
  MarketState memory state = _getUpdatedState();

  if (_annualInterestBips > BIP) {
    revert InterestRateTooHigh();
  }

  state.annualInterestBips = _annualInterestBips;
  _writeState(state);
  emit AnnualInterestBipsUpdated(_annualInterestBips);
}

As can be seen in this flow, there is never any check as to whether the new interest rate is >= minimumAnnualInterestBips (or <= maximumAnnualInterestBips). This means the borrower can circumvent the controls of the WildcatMarketController.

Tools Used

Manual review

Recommended Mitigation Steps

Ensure that all changes to market parameters still result in those parameters being within the constraints specified by the WildcatMarketController.

Assessed type

Other