The Wildcat Protocol - Eigenvectors's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L134-L144 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L136-L139 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L1-L516

Vulnerability details

Summary

The setMaxTotalSupply function inside WildcatMarketConfig is only callable by the controller of the market, but there is no function implemented inside the controller which is able to call this function. Therefore, this function is not executable and the borrower is not able to change the maxTotalSupply.

Vulnerability Details

This is a critical issue as one of the core features of the protocol can not be used and borrowers are not able to reduce their wished debt.

/**
 * @dev Sets the maximum total supply - this only limits deposits and
 *      does not affect interest accrual.
 *
 *      Can not be set lower than current total supply.
 */
function setMaxTotalSupply(uint256 _maxTotalSupply) external **onlyController** nonReentrant {
  ...
}

Impact

The borrower is not able to change the maxTotalSupply of a market, and therefore a critical feature is missing that needs to be implemented.

Tools Used

Manual Review

Recommendations

Implement a function inside the controller that calls the setMaxTotalSupply function inside the market, or change the onlyController modifier of the setMaxTotalSupply function to onlyBorrower.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142-L161 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L136-L139 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L1-L516

Vulnerability details

Summary

To close a market, the function closeMarket inside of WildcatMarket with the modifier onlyController has to be called. No function inside of WildcatMarketController calls this function. Therefore, the function closeMarket is not executable and it is impossible to close a market.

Vulnerability Details

This is a critical feature that needs to be accessible and implemented, as it leads to indefinite debt, the borrower will hold and will need to pay compound interest and fees on, for as long as lenders leave funds in the market, which could be forever.

/**
 * @dev Sets the market APR to 0% and marks market as closed.
 *
 *      Can not be called if there are any unpaid withdrawal batches.
 *
 *      Transfers remaining debts from borrower if market is not fully
 *      collateralized; otherwise, transfers any assets in excess of
 *      debts to the borrower.
 */
function closeMarket() external **onlyController** nonReentrant {
  ...
}

Impact

With the closeMarket function being not executable, it becomes impossible to close a market. This leads to infinite debt (compound interest makes this worse) if not all borrowers withdraw their funds voluntarily.

Tools Used

Manual Review

Recommendations

Implement a function inside WildcatMarketController which calls the closeMarket function inside of WildcatMarket or change the onlyController modifier to the onlyBorrower modifier.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L178-L190 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L108-L126 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L50-L81 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L123-L188 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L83-L92

Vulnerability details

Summary

As a bug allows everyone to give any not blocked EOA the withdrawOnly role, sanctioned lenders can bypass the freeze of their funds when trying to withdraw, by sending their market tokens to another EOA, give this EOA the withdrawOnly role and withdraw from this new account instead.

Vulnerability Details

The docs of the protocol state out that only the borrower should be able to add accounts to the market controller so that they are able to withdraw funds:

”Lenders can transfer market tokens freely - you can send them to a cold wallet, you can LP them, you can build additional infrastructure around them. However, it is worth noting that withdrawal requests (and subsequent claims/redemptions) can only be generated by addresses that have been approved for the controller contract of a given market - if Lender A sends their market tokens from their depositing wallet to a secondary one, they must either be sent back in order to claim, or the secondary wallet address must also be added to the market controller by the borrower.”

But this can be bypassed in the following way:

Inside the WildcatMarketController is a function called updateLenderAuthorization which should update the authorization state of the given lender in the given markets depending on if the lender is contained in the _authorizedLenders set or not. This function is callable by everyone, because only the borrower is able to add, and remove lenders from the _authorizedLenders set and therefore the protocol probably made the assumption that it does not matter who syncs the state from the controller to the given markets:

/**
 * @dev Update lender authorization for a set of markets to the current
 *      status.
 */
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));
  }
}

As we can see, this function calls the updateAccountAuthorization function in the given markets:

/**
 * @dev Updates an account's authorization status based on whether the controller
 *      has it marked as approved.
 */
function updateAccountAuthorization(
  address _account,
  bool _isAuthorized
) external onlyController nonReentrant {
  MarketState memory state = _getUpdatedState();
  Account memory account = _getAccount(_account);
  if (_isAuthorized) {
    account.approval = AuthRole.DepositAndWithdraw;
  } else {
    account.approval = AuthRole.WithdrawOnly;
  }
  _accounts[_account] = account;
  _writeState(state);
  emit AuthorizationStatusUpdated(_account, account.approval);
}

This function then calls the _getAccount function, which only checks if the account has the AuthRole.Blocked therefore, this passes if the AuthRole is the AuthRole.Null:

/**
 * @dev Retrieve an account from storage.
 *
 *      Reverts if account is blocked.
 */
function _getAccount(address accountAddress) internal view returns (Account memory account) {
  account = _accounts[accountAddress];
  if (account.approval == AuthRole.Blocked) {
    revert AccountBlacklisted();
  }
}

If we now look back up to the updateAccountAuthorization functions inside the controller and the market, we can see that anyone is able to give an account the WithdrawOnly role if this account is not blocked and is not inside the _authorizedLenders set. This should not be possible as already stated out in the beginning, and it allows sanctioned lenders to withdraw their funds in the following way.

When a lender is sanctioned by the chainalysisSanctionsList the protocol intend to not allow this lender to withdraw funds. To prevent this, a _blockAccount function was implemented which sets the authorization state of an account to blocked and transfers the funds of this account to an escrow contract. This can either be tricked manually by calling the nukeFromOrbit function, or automatically if a user try to withdraw funds:

/**
 * @dev Execute a pending withdrawal request for a batch that has expired.
 *
 *      Withdraws the proportional amount of the paid batch owed to
 *      `accountAddress` which has not already been withdrawn.
 *
 *      If `accountAddress` is sanctioned, transfers the owed amount to
 *      an escrow contract specific to the account and blocks the account.
 *
 *      Reverts if:
 *      - `expiry > block.timestamp`
 *      -  `expiry` does not correspond to an existing withdrawal batch
 *      - `accountAddress` has already withdrawn the full amount owed
 */
function executeWithdrawal(
  address accountAddress,
  uint32 expiry
) external nonReentrant returns (uint256) {
  ...
  if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
    _blockAccount(state, accountAddress);
    address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
      accountAddress,
      borrower,
      address(asset)
    );
    asset.safeTransfer(escrow, normalizedAmountWithdrawn);
    emit SanctionedAccountWithdrawalSentToEscrow(
      accountAddress,
      escrow,
      expiry,
      normalizedAmountWithdrawn
    );
  } else {
		...
	}
	...
}

If now a lender becomes aware about being a part of the chainalysisSanctionsList, the user can transfer all market tokens to another EOA account which is not on this list, give this account the WithdrawOnly authorization role by abusing the bug explained above and withdraw with this new account to bypass the freeze of funds.

Proof of Concept

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

import './BaseMarketTest.sol';
import 'src/interfaces/IMarketEventsAndErrors.sol';
import 'src/libraries/MathUtils.sol';
import 'src/libraries/SafeCastLib.sol';
import 'src/libraries/MarketState.sol';
import 'solady/utils/SafeTransferLib.sol';
import { SanctionsList } from '../src/libraries/Chainalysis.sol';
import { MockChainalysis, deployMockChainalysis } from './helpers/MockChainalysis.sol';

contract AuditTest is BaseMarketTest {
  using MathUtils for uint256;
  using FeeMath for uint256;

  function test_normal_conditions() external {
    uint128 userBalance = DefaultMaximumSupply / 2;
    uint128 withdrawalAmount = userBalance;
    _deposit(alice, userBalance);

    assertGt(market.balanceOf(alice), 0);
    
    // alice gets sanctioned
    WildcatSanctionsSentinel sentinel = new WildcatSanctionsSentinel(
        address(archController), address(SanctionsList)
    );
    MockChainalysis(address(SanctionsList)).sanction(address(alice));
    assertEq(sentinel.isSanctioned(borrower, address(alice)), true);

    // alice request withdrawal
    _requestWithdrawal(alice, withdrawalAmount);
    uint256 expiry = block.timestamp + parameters.withdrawalBatchDuration;
    fastForward(parameters.withdrawalBatchDuration);
    MarketState memory state = pendingState();
    updateState(state);
    uint256 previousBalanceAsset = asset.balanceOf(alice);
    withdrawalAmount = state.normalizedUnclaimedWithdrawals;
    vm.prank(alice);
    market.executeWithdrawal(alice, uint32(expiry));

    // alice asset balance does not change as the funds were sent to the escrow contract instead
    assertEq(asset.balanceOf(alice), previousBalanceAsset);
  }

  function test_bypass_freeze() external {
    uint128 userBalance = DefaultMaximumSupply / 2;
    uint128 withdrawalAmount = userBalance;
    _deposit(alice, userBalance);
    
    // alice gets sanctioned
    WildcatSanctionsSentinel sentinel = new WildcatSanctionsSentinel(
        address(archController), address(SanctionsList)
    );
    MockChainalysis(address(SanctionsList)).sanction(address(alice));
    assertEq(sentinel.isSanctioned(borrower, address(alice)), true);

    // alice sents her funds to another EOA
    address aliceSecondEOA = address(1234567890);
    vm.prank(alice);
    market.transfer(aliceSecondEOA, withdrawalAmount);
    assertGt(market.balanceOf(aliceSecondEOA), 0);
    address[] memory marketsArray = new address[](1);
    marketsArray[0] = address(market);
    vm.prank(aliceSecondEOA);
    controller.updateLenderAuthorization(aliceSecondEOA, marketsArray);

    // alice request withdrawal with the new EOA
    _requestWithdrawal(aliceSecondEOA, withdrawalAmount);
    uint256 expiry = block.timestamp + parameters.withdrawalBatchDuration;
    fastForward(parameters.withdrawalBatchDuration);
    MarketState memory state = pendingState();
    updateState(state);
    uint256 previousBalanceAsset = asset.balanceOf(aliceSecondEOA);
    withdrawalAmount = state.normalizedUnclaimedWithdrawals;
    vm.prank(aliceSecondEOA);
    market.executeWithdrawal(aliceSecondEOA, uint32(expiry));

    // withdrawal works and therefore alice bypassed the freeze of funds
    assertEq(asset.balanceOf(aliceSecondEOA), previousBalanceAsset + withdrawalAmount);
  }
}

Impact

Sanctioned lenders are able to bypass the freeze of their funds and withdraw them

Tools Used

Manual Review

Recommendations

Update the logic of the updateLenderAuthorization functions so that only borrowers are able to set the WithdrawOnly role. Be aware that this would create a new issue inside the stunningReversal function, which is used to remove the blocked authorization state of an account, if this account is no longer sanctioned:

/**
 * @dev Unblock an account that was previously sanctioned and blocked
 *      and has since been removed from the sanctions list or had
 *      their sanctioned status overridden by the borrower.
 */
function stunningReversal(address accountAddress) external nonReentrant {
  Account memory account = _accounts[accountAddress];
  if (account.approval != AuthRole.Blocked) {
    revert AccountNotBlocked();
  }

  if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
    revert NotReversedOrStunning();
  }

  account.approval = AuthRole.Null;
  emit AuthorizationStatusUpdated(accountAddress, account.approval);

  _accounts[accountAddress] = account;
}

As we can see, this function sets the accounts approval role to Null and not to WithdrawOnly. Therefore, lenders which are no longer sanctioned will still rely on the borrower to update their role to WithdrawOnly to get their funds back. This could potentially lead to the borrower blackmailing the lender, and therefore the role of the account should be set to WithdrawOnly.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L394-L440 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L468-L488 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L149-L158

Vulnerability details

Summary

When updating the annualInterestBips for an already deployed market, a check is missing if the given value is inside the allowed bounds.

Vulnerability Details

The protocol sets some min and max values for all the parameters of a market given by the controllerFactory. At deployment of a new market, these are checked with the enforceParameterConstraints function:

function enforceParameterConstraints(
  string memory namePrefix,
  string memory symbolPrefix,
  uint16 annualInterestBips,
  uint16 delinquencyFeeBips,
  uint32 withdrawalBatchDuration,
  uint16 reserveRatioBips,
  uint32 delinquencyGracePeriod
) internal view virtual {
  ...
  assertValueInRange(
    annualInterestBips,
    MinimumAnnualInterestBips,
    MaximumAnnualInterestBips,
    AnnualInterestBipsOutOfBounds.selector
  );
  ...
}

There is one market parameter called annualInterestBips which declares how much interest the lenders receive. Borrower are able to update the annualInterestBips, of an already deployed market, by calling the setAnnualInterestBips function:

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 function calls the setAnnualInterestBips function inside the market:

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

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

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

And as we can see, it never checks if the annualInterestBips variable is inside the min and max range given by the protocol.

Impact

The Main invariant “Market parameters should never be able to exit the bounds defined by the controller which deployed it.” can be broken.

Tools Used

Manual Review

Recommendations

Check if the given annualInterestBips parameter is inside the bounds of the controller.

Assessed type

Invalid Validation