The Wildcat Protocol - nirlin's results

Lines of code

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

Vulnerability details

Impact

User cannot change the capacity of the vault, implementation for this change is in the code but is not callable by anyone, as the setMaxTotalSupply is only callable via the controller but is never called in the controller.

Proof of Concept

About changing the the capacity of the market the whitepaper states the following:

If a borrower decides they want less, this is also fine - dropping the capacity does
not have any impact on assets that are currently in the vault, as the capacity
dictates openness to new deposits. If a lender deposits 100 WETH into a vault
and the borrower decides that actually, that’ll do, they can drop the maximum
capacity to zero if they wish, but they’re still on the hook to ultimately repay
the lender that 100 WETH plus any interest accrued.

So a user is free to increase or decrease the maxTotalSupply/capacity of the vault, but the final implementation just let to increase the capacity which is the intended design.

So lets have a look at the code for the setMaxTotalSupply:

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

We can see that modifier used is onlyController which means that only controller can call this function, so borrower should have a way to invoke this via the controller code.

But the catch is this function is never called in the controller but someone may point how this obvious thing can be missed out in the unit tests.

So if we look at the test contracts for this function, we are calling this function always pranking as the controller but controller actually have no point of entry for borrower to invoke. Following is the test code:

  function test_maxTotalSupply() external returns (uint256) {
    assertEq(market.maxTotalSupply(), parameters.maxTotalSupply);
    vm.prank(parameters.controller);
    market.setMaxTotalSupply(10000);
    assertEq(market.maxTotalSupply(), 10000);
  }

vm.prank(parameters.controller); is the problem in this test case

POC

I have added the following function in the test cases that pranks as borrower to invoke the function directly this can be WildcatMarketConfig.t.sol and run the command: forge test --match-contract WildcatMarketConfigTest --match-test test_maxTotalSupply_Borrower -vv and it would pass for the expected revert

  function test_maxTotalSupply_Borrower() external returns(uint256)
  {
       assertEq(market.maxTotalSupply(), parameters.maxTotalSupply);
    vm.prank(parameters.borrower);
    // expect revert with NotController()
    vm.expectRevert(IMarketEventsAndErrors.NotController.selector);
    market.setMaxTotalSupply(10000);
    
    
  }

Above will give the output:

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

Now lets add another function in the controller and run another test

Add the following code in the controller code:

  function setMaxTotalSupply(address market, uint256 maxTotalSupply) external onlyBorrower {
      require(_controlledMarkets.contains(market),"Not a controller market");
      WildcatMarket(market).setMaxTotalSupply(maxTotalSupply);
  }

And add the following code into the WildcatMarketConfig.t.sol and run the command

  function test_setmaxTotaSupply_Borrower_Fixed() external returns(uint256)
  {
        assertEq(market.maxTotalSupply(), parameters.maxTotalSupply);
    vm.prank(parameters.borrower);
    controller.setMaxTotalSupply(address(market),10000);
    assertEq(market.maxTotalSupply(), 10000);
  }

forge test --match-contract WildcatMarketConfigTest --match-test test_setmaxTotaSupply_Borrower_Fixed -vv

It should give the following output:

Running 1 test for test/market/WildcatMarketConfig.t.sol:WildcatMarketConfigTest
[PASS] test_setmaxTotaSupply_Borrower_Fixed():(uint256) (gas: 54618)

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.61ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry + Sleep + Some brain power + luck

Recommended Mitigation Steps

Add the above added setMaxTotalSupply function in the controller and give the entry point to the borrower, or maybe make the function directly callable by the borrower in the config itself.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L163-L187 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L137-L188 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L120

Vulnerability details

Impact

While calling createEscrow in WildcatMarketBase and WildcatMarketWithdrawl wrong order of inputs is passed instead of the correct signature of the createEscrowFunction which leads to setting the escrow address against the lender in sanctionOverrides instead of the borrower and also the deployed address is different from what it should have been due to wrong salt.

Proof of Concept

Lets look at the function signature of the the createEscrow

  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;

    emit SanctionOverride(borrower, escrowContract);

    _resetTmpEscrowParams();
  }
}

We can see the first argument is borrower, second is lender and third one and last is the asset being lent.

But the problem is this function is called at two places and in both places first two arguments are switched and in one case third argument is passed as the calling contract instead of the asset in scope.

First in the WildcatMarketBase.sol

  function _blockAccount(MarketState memory state, address accountAddress) internal {
    Account memory account = _accounts[accountAddress];
    if (account.approval != AuthRole.Blocked) {
      uint104 scaledBalance = account.scaledBalance;
      account.approval = AuthRole.Blocked;
      emit AuthorizationStatusUpdated(accountAddress, AuthRole.Blocked);

      if (scaledBalance > 0) {
        account.scaledBalance = 0;
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
          accountAddress,
          borrower,
          address(this)
        );
        emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
        _accounts[escrow].scaledBalance += scaledBalance;
        emit SanctionedAccountAssetsSentToEscrow(
          accountAddress,
          escrow,
          state.normalizeAmount(scaledBalance)
        );
      }
      _accounts[accountAddress] = account;
    }
  }

Here we can clearly see the arguments are messed up

Second in the WildcatMarketWithdrawl.sol

function executeWithdrawal(
    address accountAddress,
    uint32 expiry
  ) external nonReentrant returns (uint256) {
    if (expiry > block.timestamp) {
      revert WithdrawalBatchNotExpired();
    }
    MarketState memory state = _getUpdatedState();

    WithdrawalBatch memory batch = _withdrawalData.batches[expiry];
    AccountWithdrawalStatus storage status = _withdrawalData.accountStatuses[expiry][
      accountAddress
    ];

    uint128 newTotalWithdrawn = uint128(
      MathUtils.mulDiv(batch.normalizedAmountPaid, status.scaledAmount, batch.scaledTotalAmount)
    );

    uint128 normalizedAmountWithdrawn = newTotalWithdrawn - status.normalizedAmountWithdrawn;

    status.normalizedAmountWithdrawn = newTotalWithdrawn;
    state.normalizedUnclaimedWithdrawals -= normalizedAmountWithdrawn;

    if (normalizedAmountWithdrawn == 0) {
      revert NullWithdrawalAmount();
    }

    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 {
      asset.safeTransfer(accountAddress, normalizedAmountWithdrawn);
    }

    emit WithdrawalExecuted(expiry, accountAddress, normalizedAmountWithdrawn);

    // Update stored state
    _writeState(state);

    return normalizedAmountWithdrawn;
  }

So passing the arguments in wrong order, firstly deploys at the wrong address which is not desired by the protocol, and secondly to prevent the escrow contract we are using the sanctionOverride mapping which set the escrow contract for the borrower to true to prevent permanent locking of funds in case the oracle gets manipulated or shuts down.

But in this case instead of setting the escrow address for the borrower it is set for the lender(account) which is not intended behaviour.

Tools Used

Manual review

Recommended Mitigation Steps

Simply pass the arguments in right order.

Assessed type

Other

<img src="https://user-images.githubusercontent.com/68193826/278458401-f97e2225-8cea-49c3-8423-8ad24e580144.jpg" loading="lazy" width="30" alt="" class="image-9"> Description and Overview

Wildcat introduces a unique model of lending and borrowing that we have not seen in DEFI ever before. Instead of giving all the control to the protocol, protocol gives all the control to borrowers to create a market with every possible parameter to be modifiable in accordance to the needs of borrowers and lenders.

As the official whitepaper states:

Wildcat is primarily aimed at organisations such as market makers wishing to borrow funds in the medium-term, but can reasonably be extended
to parties such as DeFi protocols wishing to raise funds without the consequences of selling a native-token filled treasury into the ground to do so.
As a protocol that is - in its current form - reliant upon counterparty
selection by borrowers, Wildcat is fundamentally permissioned in nature.
Given that the positions that Wildcat vaults allow a lender to enter are
undercollateralised by design, it is not suitable for usage in many cases,
dependent on risk appetite.

<img src="https://user-images.githubusercontent.com/68193826/278458401-f97e2225-8cea-49c3-8423-8ad24e580144.jpg" loading="lazy" width="30" alt="" class="image-9"> Approach and Scope

For this audit I followed the top down approach of diving into code on the following scope:

Scope

• 1. WildcatMarket.sol:

  • This is the top level contract and entry point for the deposit into the markets and also to collect fees. It interact with all other below mentioned contract and libraries to give a relatively easy to understand entry point, abstracting away the complexities it using under the hood.

• 2. WildcatMarketConfig.sol:

  • this contract allows the borrower to set the important parameters such as authorization, totalsupply, annualInterestBips and reserveRatioBips. And also let unblock accounts(lender) and block accounts using the nuke function.

• 3. WildcatToken.sol:

  • This contract implements the erc20 token but the added functionality of scaling the balances.

• 4. WildcatMarketWithdrawal.sol:

  • this contract deals with the whole withdrawing process, queuing, executing and processing the withdrawal and view functions to get the current withdrawal data.
• 5. WildcatMarketBase.sol:
  • this functions serves as a whole base for the market with functions that update the state of the market, view functions to access each state, and internal functions to process the withdrawal.

• 6. WildcatArchController.sol:

  • This contract is a top level contract that stores all the data for all the controllers, borrowers and factories.

• 7. WildcatControllerFactory.sol:

  • This contract deploys all the controllers and have all the wildcat deployed set parameters with which all the controllers and markets are deployed, including max and min values for interest bips, reserve ratio bips, deliquincy fee bips, withdrawal batch duration, and grace periods. This contract also lets deploy market along with controller.

• 8. WildcatMarketController.sol:

  • this function is used to deploy the market, set interest rates, reserve ratios and authorize and deauthorize the lenders.
• 9. WildcatSanctionsEscrow.sol:
  • this contract is used as escrow when a lender is blocked and is used to refund the lender funds back to him if possible.

-10.WildcatSanctionsSentinal.sol

• This contract deploys the escrow contract, and lets override the oracle blocking the lenders to prevent any oracle problem.

Architecture of code base

First have a look at the overall architecture of market how market is formed from these multiple contracts

MarketBase inherits from three main contracts, that each bring in their functionalities as discussed above, using different libraries and their own custom functions.

Than the wildcatMarket.sol inherits from all these contracts

contract WildcatMarket is
  WildcatMarketBase,
  WildcatMarketConfig,
  WildcatMarketToken,
  WildcatMarketWithdrawals

The interaction between the controller factory, arch controller, sentinal, escrow deployment, controller and market deployment is as per following diagram:

<img src="https://user-images.githubusercontent.com/68193826/278458401-f97e2225-8cea-49c3-8423-8ad24e580144.jpg" loading="lazy" width="30" alt="" class="image-9"> Codebase Quality

I would say the codebase quality is good but can be improved, there are checks in place to handle different roles, each standard is ensured to be followed. And if something is not fully being followed that have been informed. But still it can be improved in following areas

<img src="https://user-images.githubusercontent.com/68193826/278458401-f97e2225-8cea-49c3-8423-8ad24e580144.jpg" loading="lazy" width="30" alt="" class="image-9"> Centralisation Risks

Protocol is highly dependent on the well behaving of the borrower as borrower can run away any time. But wild cat reduces that risks by doing the KYC on its own end before adding the borrower into the market and letting him open the vaults. So it seems covered and safe in this case and there seems to be no concentration risk.

Another case is the deployer may go rouge and add the malicious borrowers into the wild cat system and set the wrong parameters, but it is not clear will either the deployer be multisig or not. This concern can be mitigated too by using the multisig.

<img src="https://user-images.githubusercontent.com/68193826/278458401-f97e2225-8cea-49c3-8423-8ad24e580144.jpg" loading="lazy" width="30" alt="" class="image-9"> Dependecies Used

If we look at the the package.json for the project, we can see that the not the latest version of openzeppelin is used:

''' "dependencies": { "@openzeppelin/contracts": "^4.9.3", "@types/node": "^20.4.2", "cli-barchart": "^0.2.3", "ethers": "^6.6.3" } '''

Project is using the version ^4.9.3 while the latest recent major upgrade is 0.5.0 which have many improvements and optimisation which makes it more efficient to use.

Some of the main upgrades are:

1. Namespaced Storage for better upgrades security
2. Leveraging new Solidity compiler additions
3. Tooling updates
4. Gas-efficiency without compromising Security
5. Replacing revert strings with custom errors
6. Removing duplicated SLOADs and adding immutable variables
7. Packing variables in storage
8. Flexible and Transparent Access Management
9. Redefining Access Control through AccessManager
10. Best-in-class Audit & Testing Practices

So instead of using the old version, the project is using ^4.3.1. Replace the old version with newer one to get advantage of all these new updates.

<img src="https://user-images.githubusercontent.com/68193826/278458401-f97e2225-8cea-49c3-8423-8ad24e580144.jpg" loading="lazy" width="30" alt="" class="image-9"> Conclusion

Wildcat brings a novel solution to the existing problem that we have not seen before. Overall the code quality and mission statement is great. Had some hard time understanding the bip and ray mathematics, more documentations would have helped but the solution is gonna serve well to most of the people.

Time spent:

27 hours