The Wildcat Protocol - T1MOH's results

Lines of code

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

Vulnerability details

Impact

Function WildcatMarket.closeMarket() can be called only by Controller. However there isn't function in Controller to call it.

As a result, borrower can't close market.

Proof of Concept

It has onlyController modifier

  function closeMarket() external onlyController nonReentrant {
    ...
  }

However function closeMarket() is never called from Controller

Tools Used

Manual Review

Recommended Mitigation Steps

Either add function to Controller, or change modifier to onlyBorrower

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

WildcatMarketConfig.setMaxTotalSupply() is used to increase maxTotalSupply in market. It can be called by Controller, however Controller never calls this function

As a result, borrower can't increase maxTotalSupply

Proof of Concept

Function has modifier onlyController, however setMaxTotalSupply() is never called in Controller

  function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {
    ...
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Either add function to Controller, or change modifier to onlyBorrower

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L106-L107

Vulnerability details

Impact

Asset balance is overestimated by amount of withdrawableFees in delinquency check in _writeState() while collecting protocol fees.

Suppose current market state:

1. assetBalance = 1000
2. accruedProtocolFees = 200
3. requiredReserves = 600
4. unclaimedWithdrawals = 300

Obviously after repaying protocolFee market should be marked delinquent because 900 > 800 is true However it will perform check 900 > 1000 is false, leaving market non-delinquent

Proof of Concept

Here it firstly updates state.accruedProtocolFees, than writes state, than transfers asset. Note that asset transfer is after _writeState.

  function collectFees() external nonReentrant {
    ...
    state.accruedProtocolFees -= withdrawableFees;
    _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
    emit FeesCollected(withdrawableFees);
  }

_writeState() checks whether market is delinquent:

  function _writeState(MarketState memory state) internal {
@>  bool isDelinquent = state.liquidityRequired() > totalAssets();
    state.isDelinquent = isDelinquent;
    _state = state;
    emit StateUpdated(state.scaleFactor, isDelinquent);
  }

Let's have a look on what values are checked. Asset balance is checked against liabilities. Note that state.accruedProtocolFees was previously reduced by withdrawableFees totalAssets() is current balance of asset:

  function totalAssets() public view returns (uint256) {
    return IERC20(asset).balanceOf(address(this));
  }

  function liquidityRequired(
    MarketState memory state
  ) internal pure returns (uint256 _liquidityRequired) {
    uint256 scaledWithdrawals = state.scaledPendingWithdrawals;
    uint256 scaledRequiredReserves = (state.scaledTotalSupply - scaledWithdrawals).bipMul(
      state.reserveRatioBips
    ) + scaledWithdrawals;
    return
      state.normalizeAmount(scaledRequiredReserves) +
      state.accruedProtocolFees +
      state.normalizedUnclaimedWithdrawals;
  }

Issue is that above check occurs before asset transfer. In fact asset balance is higher by the amount of withdrawableFees than actual balance during delinquency check

  function collectFees() external nonReentrant {
    ...
    state.accruedProtocolFees -= withdrawableFees;
    _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
    emit FeesCollected(withdrawableFees);
  }

That's how asset is overestimated and can not mark market delinquent.

Tools Used

Manual Review

Recommended Mitigation Steps

Check it after transfer

  function collectFees() external nonReentrant {
    MarketState memory state = _getUpdatedState();
    if (state.accruedProtocolFees == 0) {
      revert NullFeeAmount();
    }
    uint128 withdrawableFees = state.withdrawableProtocolFees(totalAssets());
    if (withdrawableFees == 0) {
      revert InsufficientReservesForFeeWithdrawal();
    }
    state.accruedProtocolFees -= withdrawableFees;
-   _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
+   _writeState(state);
    emit FeesCollected(withdrawableFees);
  }

And change it in borrow() for future, despite now there is no issue

  function borrow(uint256 amount) external onlyBorrower nonReentrant {
    MarketState memory state = _getUpdatedState();
    if (state.isClosed) {
      revert BorrowFromClosedMarket();
    }
    uint256 borrowable = state.borrowableAssets(totalAssets());
    if (amount > borrowable) {
      revert BorrowAmountTooHigh();
    }
-   _writeState(state);
    asset.safeTransfer(msg.sender, amount);
+   _writeState(state);
    emit Borrow(amount);
  }

Assessed type

Error

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

1. Low. setAnnualInterestBips() will decrease reserveRatio for the next 2 weeks if it was above 90%

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L197-L213 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L112-L126

Vulnerability details

Impact

Authorized lenders can deposit to Market. In deposit, Market uses cashed value of lender authorization instead of fetching current state from Controller. Inconsistency arises when Borrower deauthorizes lender, lender still can deposit until somebody explicitly calls WildcatMarketController.updateLenderAuthorization()

As a result, lender still can deposit after deauthorization because market cashes this value instead of fetching.

Proof of Concept

Lender will be removed from authorized lenders in Controller. And it requires to submit second transaction to remove lender authorization from specific market:

  function deauthorizeLenders(address[] memory lenders) external onlyBorrower {
    for (uint256 i = 0; i < lenders.length; i++) {
      address lender = lenders[i];
      if (_authorizedLenders.remove(lender)) {
        emit LenderDeauthorized(lender);
      }
    }
  }

  function updateLenderAuthorization(address lender, address[] memory markets) external {
    for (uint256 i; i < markets.length; i++) {
      address market = markets[i];
      if (!_controlledMarkets.contains(market)) {
        revert NotControlledMarket();
      }
      WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
    }
  }

In Market it doesn't fetch Approval from Controller if approval was set previously:

  function _getAccountWithRole(
    address accountAddress,
    AuthRole requiredRole
  ) internal returns (Account memory account) {
    account = _getAccount(accountAddress);
    // If account role is null, see if it is authorized on controller.
    //@audit Will always be falls if lender previously interacted with protocol
@>  if (account.approval == AuthRole.Null) {
      if (IWildcatMarketController(controller).isAuthorizedLender(accountAddress)) {
        account.approval = AuthRole.DepositAndWithdraw;
        emit AuthorizationStatusUpdated(accountAddress, AuthRole.DepositAndWithdraw);
      }
    }
    ...
  }

Here lender can bypass authorization check due to old cashed value:

  function depositUpTo(
    uint256 amount
  ) public virtual nonReentrant returns (uint256 /* actualAmount */) {
    ...

    // Cache account data and revert if not authorized to deposit.
    Account memory account = _getAccountWithRole(msg.sender, AuthRole.DepositAndWithdraw);
    
    ...
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Remove methods WildcatMarketController.updateLenderAuthorization(), WildcatMarketConfig.updateAccountAuthorization() Remove field approval from struct Account

Fetch this value from controller:

  function _getAccountWithRole(
    address accountAddress,
    AuthRole requiredRole
  ) internal returns (Account memory account) {
    account = _getAccount(accountAddress);
    // If account role is null, see if it is authorized on controller.
-   if (account.approval == AuthRole.Null) {
-     if (IWildcatMarketController(controller).isAuthorizedLender(accountAddress)) {
-       account.approval = AuthRole.DepositAndWithdraw;
-       emit AuthorizationStatusUpdated(accountAddress, AuthRole.DepositAndWithdraw);
-     }
-   }
+   if (IWildcatMarketController(controller).isAuthorizedLender(accountAddress)) {
+     if (account.approval != AuthRole.DepositAndWithdraw) {
+     account.approval = AuthRole.DepositAndWithdraw;
+     emit AuthorizationStatusUpdated(accountAddress, AuthRole.DepositAndWithdraw);
+     }
+   } else {
+     if (account.approval != AuthRole.WithdrawOnly) {
+       account.approval = AuthRole.WithdrawOnly;
+       emit AuthorizationStatusUpdated(accountAddress, AuthRole.WithdrawOnly)
+     }
+   }
    // If account role is insufficient, revert.
    if (uint256(account.approval) < uint256(requiredRole)) {
      revert NotApprovedLender();
    }
  }

Assessed type

Invalid Validation