The Wildcat Protocol - 3docSec's results

Lines of code

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

Vulnerability details

The closeMarket() function available in the WildcatMarket contract has the onlyController modifier so is hardwired to be called only by the WildcatMarketController contract. This contract has however no way to effectively make this call.

Impact

Borrowers are unable to close markets.

Proof of Concept

    function testMarketClose() public {
        // the borrower can't close the market
        vm.startPrank(borrower);
        vm.expectRevert();
        market.closeMarket();

        // the borrower looks the methods available in the controller
        // 🔎 nothing to be found 🤷
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Allow the borrower to call the closeMarket function:

  // WildcatMarket.sol:142
- function closeMarket() external onlyController nonReentrant {
+ function closeMarket() external onlyBorrower nonReentrant {
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L188 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L36

Vulnerability details

The consequence of sanctions within the protocol is preventing sanctioned lenders from withdrawing the funds deposited in markets.

This mechanism can be easily worked around, given that liquidity tokens:

• can always be transferred to any address, under the sole condition that the transferred tokens have not been escrowed yet - that is, before a nukeFromOrbit or executeWithdrawal call
• can always be withdrawn by any non-sanctioned address

In case the above happens, the borrower and the protocol admins cannot take any action rather than soliciting the new address to be included in the Chainalysis sanctions list - something that cannot be guaranteed to happen quickly enough to prevent the execution of a withdrawal.

Impact

Sanctioned lenders can always prevent their funds from being escrowed without requiring collusion from any accredited actor, simply by transferring unescrowed tokens to an un-authorized account in their control.

Proof of Concept

    function testWayAroundSanction() public {
        vm.startPrank(lender);

        // the lender deposits some tokens
        uint256 amt = 1e18;
        token.mint(lender, amt);
        token.approve(address(market), amt);
        market.deposit(amt);

        // flag the lender on the mock sanction list. Nuke from orbit is on the way!
        sanctionsList.sanction(lender, true);

        // the lender front-runs the nuke call by diverting tokens to a random account
        address randomAccount = address(uint160(uint256(keccak256("really random?"))));
        market.transfer(randomAccount, 1e18);

        // finally the nuke comes
        market.nukeFromOrbit(lender);

        // but the assets are safe and can be withdrawn after having the controller
        // airdrop the "WithdrawOnly" role to the random account
        changePrank(randomAccount);
        address[] memory markets = new address[](1);
        markets[0] = address(market);

        controller.updateLenderAuthorization(randomAccount, markets);

        // yay!
        market.queueWithdrawal(1e18);
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

• consider changing the WildcatMarketController.updateLenderAuthorization to assign the WithdrawOnly role only to accounts who "have been lenders at some point", and not to any account - this will prevent withdrawals of funds routed via unvetted accounts. Accounts who have never been lenders in the market may get a Null role instead, so they can receive funds but cannot withdraw without passing a vetting process
• do not allow market token transfers in favor of entities that are not approved lenders, ideally with the DepositAndWithdraw role

Assessed type

Access Control

Lines of code

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

Vulnerability details

When a lender is nuked out of orbit, their funds are escrowed and kept available for recovery once they are no longer sanctioned. However, due to an incorrect ordering of function call parameters, the escrow is incorrectly created for the borrower's account.

This is equally true for escrows created for market and asset tokens.

Impact

Lenders with removed or overridden sanctions see all of their tokens escrowed with the borrower as beneficiary. The borrower can take over the escrowed credit as soon as the lender is nuked from orbit.

Proof of Concept

The following test shows how nuked funds are incorrectly escrowed and can be immediately released with the borrower as beneficiary:

    function testEscrowBeneficiary() public {
        vm.startPrank(lender);

        // the lender deposits some tokens
        uint256 amt = 1e18;
        token.mint(lender, amt);
        token.approve(address(market), amt);
        market.deposit(amt);

        // flag the lender on the mock sanction list & nuke from orbit
        sanctionsList.sanction(lender, true);
        market.nukeFromOrbit(lender);

        // ⚠️ there is no escrow with lender as beneficiary
        address expectedEscrowAddress = sentinel.getEscrowAddress(
            borrower, // borrower,
            lender, // account,
            address(market) // asset
        );
        assertEq(0, expectedEscrowAddress.code.length);

        // ⚠️ there is an escrow with borrower as beneficiary
        address actualEscrowAddress = sentinel.getEscrowAddress(
            lender, // borrower,
            borrower, // account,
            address(market) // asset
        );
        assertTrue(0 != actualEscrowAddress.code.length);

        // since the borrower is not sanctioned, the faulty escrow can be released
        // without any sanction being removed
        vm.stopPrank();
        IWildcatSanctionsEscrow(actualEscrowAddress).releaseEscrow();

        // and the lender can't recover these funds because they are
        // on the borrower's balance
        assertEq(0, market.balanceOf(lender));
        assertEq(amt, market.balanceOf(borrower));
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider correcting the parameters ordering in the createEscrow call within the _blockAccount function:

  // WildcatMarketBase.sol:172
  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,
+         accountAddress,
          address(this)
        );
        emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
        _accounts[escrow].scaledBalance += scaledBalance;
        emit SanctionedAccountAssetsSentToEscrow(
          accountAddress,
          escrow,
          state.normalizeAmount(scaledBalance)
        );
      }
      _accounts[accountAddress] = account;
    }
  }

... as well as in the executeWithdrawal function:

    // WildcatMarketWithdrawals:166
    if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
      _blockAccount(state, accountAddress);
      address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
-       accountAddress,
        borrower,
+       accountAddress,
        address(asset)
      );

Assessed type

Error

Lines of code

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

Vulnerability details

When a WildcatMarketController is created, it is hardcoded with a MinimumAnnualInterestBips and MaximumAnnualInterestBips ramge that cannot be changed; these values come from WildcatMarketControllerFactory where they are specified by the protocol owners.

When a market is created at the borrower's request, the annual interest requested by the borrower is validated to sit within these bounds.

However, the borrower is also allowed to change this value at a later point through the WildcatMarketController.setAnnualInterestBips function. This entry point does not offer any validation, except for the downstream WildcatMarket.setAnnualInterestBips that checks for the value not to exceed BIPS.

Impact

After the creation of a market, the borrower is allowed to change its annual interest outside the bounds allowed by the protocol.

Proof of Concept

    function testArbitraryInterestRate() public {
        WildcatArchController archController = new WildcatArchController();
        SanctionsList sanctionsList = new SanctionsList();
        WildcatSanctionsSentinel sentinel = new WildcatSanctionsSentinel(
            address(archController), 
            address(sanctionsList));


        // the protocol mandates a 10% annual interest
        WildcatMarketControllerFactory controllerFactory = new WildcatMarketControllerFactory(
            address(archController), // _archController,
            address(sentinel), // _sentinel,
            MarketParameterConstraints( // constraints
                uint32(0), // minimumDelinquencyGracePeriod;
                uint32(0), // maximumDelinquencyGracePeriod;
                uint16(0), // minimumReserveRatioBips;
                uint16(0), // maximumReserveRatioBips;
                uint16(0), // minimumDelinquencyFeeBips;
                uint16(0), // maximumDelinquencyFeeBips;
                uint32(0), // minimumWithdrawalBatchDuration;
                uint32(0), // maximumWithdrawalBatchDuration;
                uint16(10_00), // minimumAnnualInterestBips;
                uint16(10_00)  // maximumAnnualInterestBips;
            )
        );

        address borrower = address(uint160(uint256(keccak256("borrower"))));

        archController.registerBorrower(borrower);
        archController.registerControllerFactory(address(controllerFactory));

        MockERC20 token = new MockERC20();

        vm.startPrank(borrower);
        WildcatMarketController controller = WildcatMarketController(
            controllerFactory.deployController());

        // the borrower creates a market with 10% annual interest - all good till now
        address market = controller.deployMarket(
            address(token), // asset,
            "3d", // namePrefix,
            "3", // symbolPrefix,
            type(uint128).max, // maxTotalSupply,
            10_00, // annualInterestBips
            0, // delinquencyFeeBips,
            0, // withdrawalBatchDuration,
            0, // reserveRatioBips,
            0 // delinquencyGracePeriod
        );

        // now, the borrower is allowed to change the interest below 10%
        // which is not allowed by the protocol config
        controller.setAnnualInterestBips(market, 5_00);

        // and get away with it
        assertEq(5_00, WildcatMarket(market).annualInterestBips());
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider introducing the validation of the new interest rate:

  // WildcatMarketController.sol:468
  function setAnnualInterestBips(
    address market,
    uint16 annualInterestBips
  ) external virtual onlyBorrower onlyControlledMarket(market) {
+   assertValueInRange(
+     annualInterestBips,
+     MinimumAnnualInterestBips,
+     MaximumAnnualInterestBips,
+     AnnualInterestBipsOutOfBounds.selector
+   );
    // If borrower is reducing the interest rate, increase the reserve
    // ratio for the next two weeks.

Assessed type

Invalid Validation

[L-01] ArchController allows adding a MarketControllerFactory that points to another ArchController

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

MarketControllerFactory uses ArchController for authorization purposes. ArchController allows for authorizing MarketControllerFactory instances that point to another ArchController, and if this happens, the impact would be to allow otherwise unauthorized actors to perform operations i.e. change the protocol fee configuration or deploy controllers.

Consider adding a check in ArchController.registerControllerFactory to make sure that the to-be-added MarketControllerFactory is correctly paired with it.

[L-02] WildcatMarketController's authorizeLenders and deauthorizeLenders miss checks if the operation was successful

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

These functions don't return or log any error if the operation fails (addition of a lender already in the set, removal of a lender not present). This may for example trick borrowers who mistype a lender address into thinking they successfully removed a lender when in fact they didn't.

Consider returning errors consistently with how checks are done in the ArchController.

[L-03] Floating pragma

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

All contracts in scope are floating the pragma version.

Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version.

Note that pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or a package.

Consider locking the pragma version to a specific Solidity version i.e. 0.8.21

[L-04] WildcatSanctionsSentinel.removeSanctionOverride should not be allowed for escrows

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L56

When WildcatSanctionsSentinel creates an escrow, it sets up a sanction override for its address. The borrower is incorrectly allowed to remove this override through the removeSanctionOverride function, which is meant to be used with lender addresses instead.

Consider storing the addresses of escrows in the Sentinel, and do not allow borrowers to remove the sanction override for them

[L-05] WildcatMarketController.authorizeLenders allows the borrower to authorize themselves as lender

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

In WildcatMarket instances, the borrower has complete control over the addresses authorized to perform lender operations. While this does not represent an issue per se, it would be reasonable to prevent borrowers from adding themselves as lenders to avoid unforeseen side-effects.

Consider preventing the borrower from adding themselves as lender in WildcatMarketController or WildcatMarket.

[L-06] Market allows changing the annual rate after it's when in closed state

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

After a borrower closed a WildcatMarket (state.isClosed = true), the market's annual interest is set to zero. It is however possible to change it to a different value throught a setAnnualInterestBips call.

Consider checking for market state within the setAnnualInterestBips function.

[N-01] Underflow protection produces obscure error messages

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L89 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L50 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L73

In several places, the protocol relies on Solidity 0.8.x underflow protection for preventing users from performing actions they shouldn't.

Some examples of these are:

• the check on available balance on WildcatMarketWithdrawals.queueWithdrawal
• the allowance check on WildcatMarketToken.transferFrom
• the balance check on WildcatMarketToken._transfer

Consider adding proper require statements to instead deliver more user-friendly error messages.