The Wildcat Protocol - deth's results

Lines of code

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

Vulnerability details

Impact

Closing a market is a vital part of logic in the protocol. Closing a market will make depositing and borrowing unavailable and will either transfer tokens from the borrower to the market or vice versa depending on the t otalDebts and totalAssets.

closeMarket is marked as onlyController, meaning only the WildcatMarketController can call the function.

The problem here is that, the WildcatMarketController has no function that calls closeMarket on a specific market. This means that markets can never be closed.

Proof of Concept

Inspecting WildcatMarketController reveals that nowhere does it call closeMarket or has any function that interacts with closeMarket.

Something of note, is that the tests that are testing the closeMarket functionality are written incorrectly.

Example:

function test_closeMarket_FailTransferRemainingDebt() external asAccount(address(controller)) {
    // Borrow 80% of deposits then request withdrawal of 100% of deposits
    _depositBorrowWithdraw(alice, 1e18, 8e17, 1e18);
    vm.expectRevert(SafeTransferLib.TransferFromFailed.selector);
    market.closeMarket();
  }

You'll notice the modifier called asAccount.

modifier asAccount(address prank) {
    startPrank(prank);
    _;
    stopPrank();
  }

This modifier just pranks as the address that is given to him. The problem here is that we are just pranking as a contract address and using Foundry we call closeMarket.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a function inside WildcatMarketController that calls closeMarket.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L98 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L172-L176 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L166-L170

Vulnerability details

Impact

createEscrow is used to create an escrow for an account that was sanctioned by Chainanalysis. When an account gets blocked his funds gets transferred to the escrow. The only way for the account to retrieve the tokens from the escrow is to get unsanctioned by Chainanalysis or the borrower of the market to override the sanction.

Let's take a look at the createEscrow function:

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

You can see the arguments of the function are as follows: address borrower, address account, address asset.

Now let's see where the createEscrow is called: In 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, // This should be the borrower
          borrower, // This should be the sanctioned account (accountAddress)
          address(this) 
        );
        emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
        _accounts[escrow].scaledBalance += scaledBalance;
        emit SanctionedAccountAssetsSentToEscrow(
          accountAddress,
          escrow,
          state.normalizeAmount(scaledBalance)
        );
      }
      _accounts[accountAddress] = account;
    }
  }

In WildcatMarketWithdrawals.sol.

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, // This should be the borrower
        borrower, // This should be the sanctioned account (accountAddress)
        address(asset)
      ); 
      asset.safeTransfer(escrow, normalizedAmountWithdrawn);
      emit SanctionedAccountWithdrawalSentToEscrow(
        accountAddress,
        escrow,
        expiry,
        normalizedAmountWithdrawn
      );
    } else {
    ...
  }

You can see that in both these instances the borrower and accountAddress are swapped, meaning that when we create an escrow, the accountAddress will be set as the borrower and the account will be set to the borrower.

Proof of Concept

Paste the following inside test/market/WildcatMarket.t.sol and run forge test --mt test_EscrowIsNotCreatedCorrectly -vvvv.

function test_EscrowIsNotCreatedCorrectly() external {
    // Alice deposits 10_000 tokens to the market
    vm.prank(alice);
    market.depositUpTo(10_000e18);

    // Alice queues a withdrawal for all her tokens
    vm.prank(alice);
    market.queueWithdrawal(10_000e18);

    // Chainanalysis sanctions Alice
    MockChainalysis(address(SanctionsList)).sanction(alice);

    vm.warp(block.timestamp + 2 weeks);

    // We executeWithdrawal for alice and the batch
    market.executeWithdrawal(alice, 1);

    // 0xEa0D550abcacF378882E6f583ed06dCF036d3aB7 address of the created escrow, take a look at the logs for more info
    // You can see that inside the escrow, the borrower is Alice
    // while the account is the borrower
    // It should be the other way around
    assertEq(alice, WildcatSanctionsEscrow(0xEa0D550abcacF378882E6f583ed06dCF036d3aB7).borrower());
    assertEq(borrower, WildcatSanctionsEscrow(0xEa0D550abcacF378882E6f583ed06dCF036d3aB7).account());
  }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Swap the the borrower and accountAddress, so they are in their correct positions.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

collectFees is a function that collects the protocol fees and sends them to the fee recipient address.

collectFees calls _writeState, which checks if liqudityRequried > totalAssets and if it is, it sets isDelinquent = true, meaning the borrower will now start accruing extra fees until he covers the liquidityRequired.

The problem here lies in that, _writeState is called before asset.transferFrom, meaning that when _writeState is called, totalAssets will be the token amount BEFORE the fees were sent out to the fee recipient, basically it's using an old incorrect value.

This is a problem, as if collectFees is called and it should put the borrower in delinquency, it won't, since it's using an old value for totalAssets. Because of this, the borrower won't start accruing delinquency fees immediately or even not at all, if he repays some tokens so the next time _writeState is called liqudityRequired < totalAssets.

Proof of Concept

Paste the following inside test/market/WildcatMarket.t.sol and run forge test --mt test_collectFeesDoesntPutBorrowerInDelinquencyEvenIfItShould -vvvv.

  // To see the difference, run this test once with 
  // _writeState() before asset.transfer
  // and once with
  // _writeState() after asset.transfer
  function test_collectFeesDoesntPutBorrowerInDelinquencyEvenIfItShould() external {
    // Alice deposits tokens
    vm.prank(alice);
    market.depositUpTo(50_000e18);

    vm.warp(block.timestamp + 12); // 1 block passes

    // Borrower borrows up to the maximum borrowableAssets - 1e18
    vm.startPrank(borrower);
    market.borrow(market.borrowableAssets() - 1e18);
    vm.stopPrank();

    // Time passes and collectFees is called
    vm.warp(block.timestamp + 7 hours);
    market.collectFees();
    // false, if _writeState() is called before asset.transfer
    // true, if _writeState() is called after asset.transfer 
    assertEq(false, market.previousState().isDelinquent); 

    vm.warp(block.timestamp + 365 days); // Warp time to better showcase the difference in the values below

    market.updateState();
    // 1100087941403649145486263143 is scaleFactor if _writeState() is called before asset.transfer
    // 1200089593611292566435953603 is scaleFactor if _writeState() is called after asset.transfer 
    assertEq(1100087941403649145486263143, market.previousState().scaleFactor);
  }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Call _writeState after asset.safeTransfer.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When creating a Market through the WildcatMarketController, there are certain min/max constraints that need to be followed. They apply for several parameters including annualInterestBips.

When creating a market though deployMarket the constraints are enforced through the function enforceParameterConstraints.

There is also a function inside WildcatMarketController called setAnnualInterestBips. The function is used to set the annualInterestBips for a specific market that the borrower has already deployed.

The protocol team stated that a invariant of the protocol is: "Market parameters should never be able to exit the bounds defined by the controller which deployed it". Basically the market params can't go under/over the min/max values that the protocol team has defined.

The problem here is that no constraints are enforced on annualInterestBips, which breaks assumptions and an important invariant of the protocol.

Proof of Concept

Go to test/shared/TestConstants.sol and change the following values.

    ...
    uint16 constant MinimumAnnualInterestBips = 1_000; // Originally was 0
    uint16 constant MaximumAnnualInterestBips = 8_000; // Originally was 10_000

Add the following test inside test/market/WildcatMarket.t.sol and run forge test --mt test_noConstraintsOnSetAnnualInterest -vvvv

function test_noConstraintsOnSetAnnualInterest() external {
    vm.prank(borrower);
    controller.setAnnualInterestBips(address(market), 0); // The min is 1_000 and we can set it to 0
    
    assertEq(0, market.previousState().annualInterestBips);

    vm.prank(borrower);
    controller.setAnnualInterestBips(address(market), 10_000); // The max is 8_000 and we can set it to 10_000

    assertEq(10_000, market.previousState().annualInterestBips);
  }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Add the following inside setAnnualInterestBips:

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.
    assertValueInRange(
      annualInterestBips,
      MinimumAnnualInterestBips,
      MaximumAnnualInterestBips,
      AnnualInterestBipsOutOfBounds.selector
    );
    ...
    

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Currently there is no type of repay function inside WildcatMarket, so the borrower has to use ERC20.transfer/transferFrom to repay his debts.

The problem is, even if the borrower transfers asset tokens to the market, updateState isn't called. So for example if the borrower is delinquent and decides to repay his loans, his state won't get updated automatically with the transfer of tokens, meaning he will stay in delinquency and continue accruing fees, even after repaying.

In my opinion it's bad design to force the borrower to call another function, instead of just having a repay function. It's possible a malicious actor to block stuff the borrower from calling updateState. The delinquency fees has to be quite large so it makes sense to grief the borrower in such a way, but it's still possible in some occasions.

Proof of Concept

Let's imagine the following:

1. Bob (borrower) creates a market.
2. Alice (lender) deposits 10_000 tokens.
3. Bob borrows all the borrowable assets.
4. Alice decides to withdraw 1_000 tokens, putting Bob in delinquency.
5. Time passes and Bob decides to repay everything + fees, so he should no longer be delinquent.
6. Even though Bob repaid everything, because the state hasn't been updated, Bob is still delinquent. 7A. Bob attempts to call updateState, but gets block stuffed by a malicious actor. Bob unfairly accrues more delinquency fees. 7B. Bob forgets to call updateState, so more delinquency fees are accrued to him.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a repay function inside WildcatMarket that transfers the assets to the market and calls _getUpdatedState and _writeState.

Assessed type

Other