The Wildcat Protocol - max10afternoon's results

Lines of code

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

Vulnerability details

Impact

Due to their modifieres, the closeMarket and setMaxTotalSupply functions cannot be called. Meaning that a market can never be closed and the maximum total supply cannot be modified

Proof of Concept

Both the closeMarket function of the WildcatMarket contract, and the setMaxTotalSupply function of the WildcatMarketConfig, have an onlyController modifier:

function closeMarket() external onlyController nonReentrant {

function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {

This modifier is implemented in the WildcatMarketBase contract and it checks that the caller is the controller of the market:

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

The controller being the one that has deployed the marked, as can be observed in the implementation of the getMarketParameters function, found in the WildcatMarketController contract, which is used to select the parameters inside of the wildcatMarket's constructor.

  function getMarketParameters() external view returns (MarketParameters memory parameters) {
    parameters.asset = _tmpMarketParameters.asset;
    parameters.namePrefix = _tmpMarketParameters.namePrefix;
    parameters.symbolPrefix = _tmpMarketParameters.symbolPrefix;
    parameters.borrower = borrower;
    parameters.controller = address(this);
    ...

The issue is that the WildcatMarketController contract never invokes the closeMarket or setMaxTotalSupply functions, nor it has a way to call arbitrary external functions. Meaning that the closeMarket and setMaxTotalSupply functions can never be called, making it impossible to close a market or to modify the initial max total supply.

This can be observed by searching for any mention of "closeMarket()" or "setMaxTotalSupply(" (either with grep or any find functionality) inside the /src folder, where the protocol contract's are implemented (the results, depending on the tool used, will look something like this):

Searching 33 files for "closeMarket()"

/home/user01/Desktop/code4rena/wildcat/2023-10-wildcat/src/market/WildcatMarket.sol:
  146     *      debts to the borrower.
  147     */
  148:   function closeMarket() external onlyController nonReentrant {//ERR? Can this be called?
  149      MarketState memory state = _getUpdatedState();
  150      state.annualInterestBips = 0;

1 match in 1 file

Searching 33 files for "setMaxTotalSupply"

/home/user01/Desktop/code4rena/wildcat/2023-10-wildcat/src/market/WildcatMarketConfig.sol:
  133     *      Can not be set lower than current total supply.
  134     */
  135:   function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {
  136      MarketState memory state = _getUpdatedState();
  137  

1 match in 1 file

With the only mention of the functions being the implementation it self.

This issue can further be observed by repeating the same search in the /test folder:

Searching 50 files for "closeMarket"

/home/user01/Desktop/code4rena/wildcat/2023-10-wildcat/test/market/WildcatMarket.t.sol:
  197  
  198    // ===================================================================== //
  199:   //                             closeMarket()                              //
  200    // ===================================================================== //
  201  
  202:   function test_closeMarket_TransferRemainingDebt() external asAccount(address(controller)) {
  203      // Borrow 80% of deposits then request withdrawal of 100% of deposits
  204      _depositBorrowWithdraw(alice, 1e18, 8e17, 1e18);
  ...
  208      vm.expectEmit(address(asset));
  209      emit Transfer(borrower, address(market), 8e17);
  210:     market.closeMarket();
  211    }
  212  
  213:   function test_closeMarket_TransferExcessAssets() external asAccount(address(controller)) {
  214      // Borrow 80% of deposits then request withdrawal of 100% of deposits
  215      _depositBorrowWithdraw(alice, 1e18, 8e17, 1e18);
  ...
  217      vm.expectEmit(address(asset));
  218      emit Transfer(address(market), borrower, 2e17);
  219:     market.closeMarket();
  220    }
  221  
  222:   function test_closeMarket_FailTransferRemainingDebt() external asAccount(address(controller)) {
  223      // Borrow 80% of deposits then request withdrawal of 100% of deposits
  224      _depositBorrowWithdraw(alice, 1e18, 8e17, 1e18);
  225      vm.expectRevert(SafeTransferLib.TransferFromFailed.selector);
  226:     market.closeMarket();
  227    }
  228  
  229:   function test_closeMarket_NotController() external {
  230      vm.expectRevert(IMarketEventsAndErrors.NotController.selector);
  231:     market.closeMarket();
  232    }
  233  
  234:   function test_closeMarket_CloseMarketWithUnpaidWithdrawals()
  235      external
  236      asAccount(address(controller))
  ...
  241      uint32[] memory unpaidBatches = market.getUnpaidBatchExpiries();
  242      assertEq(unpaidBatches.length, 1);
  243:     vm.expectRevert(IMarketEventsAndErrors.CloseMarketWithUnpaidWithdrawals.selector);
  244:     market.closeMarket();
  245    }
  246  }

13 matches in 1 file

As you can see each time closeMarket is called (the same is valid for setMaxTotalSupply), it is inside a test function with the asAccount(address(controller)) modifier, which pranks as the address passed as argument (except for the test where it is expected to revert with IMarketEventsAndErrors.NotController.selector). Which means that it will work in the tests, but would not be usable on mainnet.

Recommended Mitigation Steps

The closeMarket and setMaxTotalSupply functions could be either callable by the borrower using the onlyBorrower modifier (since the borrower has control of the controller contract this wouldn't technically change much) or the controller contract could implement a wrapper function to invoke them.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

One of the invariants in the project is the following: The assets of a market should never be able to be withdrawn by anyone that is not the borrower or a lender with either the WithdrawOnly or DepositAndWithdraw role.

Although not technically broken, any account can grant them self the WithdrawOnly permission level, for any existing market. Meaning that the WithdrawOnly check performed by the queueWithdrawal function is actually meaning less (since anyone can autonomously grant them self such permission). Therefor, even tough, the invariant above is technically respected, it can be circumvented by any unsanctioned account in the blockchain.

Proof of Concept

The WildcatMarketConfig contract implements the updateAccountAuthorization function which enables the WildcatMarketController contract to update the role of an account:

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

As it can be observed, this function, gives the AuthRole.WithdrawOnly or AuthRole.DepositAndWithdraw, to any unsanctioned account (such control being implemented by the _getAccount function). Also the updateAccountAuthorization function can only be invoked by the controller contract, which does so, using the updateLenderAuthorization function:

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

This function has no access control, meaning that anyone can call it, and it invokes the updateAccountAuthorization with two parameters: the address who's permissions should be upgraded (which can be any address, since this parameter is user controllable) and a boolean, which will be true, if the address that is being updated is part of the _authorizedLenders or false otherwise. If the account is not an authorized lender the second parameter will be false, and the if statement in updateAccountAuthorization will default to the AuthRole.WithdrawOnly authorization. This is probably done, to prevent borrowers from negating lenders the opportunity to withdraw, by revoking their authorizations, but it also means that anyone can call the updateLenderAuthorization on an arbitrary account and give it WithdrawOnly privileges. Allowing anyone to withdraw from the market regardless of their authorization level.

Here is a very simple foundry test, that shows that an arbitrary account can queue a withdraw. Place is in the /test/market/ folder to preserve dependencies:

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

import 'src/interfaces/IMarketEventsAndErrors.sol';
import '../BaseMarketTest.sol';

contract WildcatMarketConfigTest is BaseMarketTest {
  function test_anyoneCanWithdraw(uint128 userBalance, uint128 withdrawalAmount) external {
    userBalance = uint128(bound(userBalance, 2, DefaultMaximumSupply));
    withdrawalAmount = uint128(bound(withdrawalAmount, 2, userBalance));

    _deposit(alice, userBalance);
    vm.prank(alice);
    market.transfer(address(0x123), withdrawalAmount);


    address[] memory markedAddress = new address[](1);
    markedAddress[0]=address(market);

    vm.prank(address(0x123));
    controller.updateLenderAuthorization(address(0x123), markedAddress);

    vm.startPrank(address(0x123));
    market.queueWithdrawal(withdrawalAmount);
  }
}

Recommended Mitigation Steps

The updateLenderAuthorization function could implement some level of access control, to allow only accounts that where lenders at some points, to invoke it.

Assessed type

Access Control