The Wildcat Protocol - Yanchuan's results

Lines of code

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

Vulnerability details

Impact

According to Wildcat's design, a market can be closed. However, the closeMarket function has a modifier onlyController. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142-L149

  function closeMarket() external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0;
    if (_withdrawalData.unpaidBatches.length() > 0) {
      revert CloseMarketWithUnpaidWithdrawals();
    }

The onlyController modifier is defined as follows. Therefore, only the WildcatMarketController has the authority to close a market. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L136-L139

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

However, WildcatMarketController does not have a method that calls closeMarket. No one, including the controller, can invoke closeMarket, and as a result, the market cannot be closed.

Proof of Concept

Nobody can close a market.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a method to close the market in the WildcatMarketController contract.

  function closeMarket(address market) external virtual onlyBorrower onlyControlledMarket(market) {
    WildcatMarket(market).closeMarket();
  }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

if lender address is added to a national sanctions list, a escrow contract is deployed, and lender's asset will be transferred to this account. The first parameter is borrower, and the second parameter is lender. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L102

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

However, when calling the createEscrow function, the order of borrower and lender is wrong. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L164-L171

    if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
      _blockAccount(state, accountAddress);
      address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
>>        accountAddress,
>>        borrower,
        address(asset)
      );
      asset.safeTransfer(escrow, normalizedAmountWithdrawn);

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

        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
>>          accountAddress,
>>          borrower,
          address(this)
        );

This will allow the lender to remove themselves from the sanction list. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L48-L59

  function overrideSanction(address account) public override {
    sanctionOverrides[msg.sender][account] = true;
    emit SanctionOverride(msg.sender, account);
  }

  /**
   * @dev Removes the sanction override of `account` for `borrower`.
   */
  function removeSanctionOverride(address account) public override {
    sanctionOverrides[msg.sender][account] = false;
    emit SanctionOverrideRemoved(msg.sender, account);
  }

Proof of Concept

Assuming Alice is the borrower and Bob is a lender on the sanction list. When Alice calls nukeFromOrbit(bob) to block Bob, but in reality, the parameters for createEscrow are createEscrow(bob, alice), effectively blocking Alice instead.

Tools Used

Manual Review

Recommended Mitigation Steps

When calling the createEscrow function, adjust the order of the parameters borrower and accountAddress.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The _getUpdatedState() function is crucial for updating the state of the market, including key data like scaleFactor, scaledTotalSupply, accruedProtocolFee, etc. This function serves as a core element, being invoked by various functions such as deposit, borrow, queueWithdrawal, executeWithdrawal, among others. However, there's a bug in the function. When there are unfully paid withdrawals in the market, the method for calculating the protocol fee is incorrect.

Assuming maxTotalSupply is 100, withdrawalBatchDuration is 1 day, reserveRatioBips is 2000, annualInterestBips is 1000, and protocolFeeBips is 1000, the test scenario is described as follows:

Day 0: Alice calls deposit(50) to deposit 50 tokens. The borrower calls borrow(40) to borrow 40 tokens. Alice then calls queueWithdrawal(10) to request a withdrawal of 10 tokens. Day 1: Alice calls executeWithdrawal(alice, day1) to withdraw 10 tokens and then calls queueWithdrawal(20) to request a withdrawal of 20 tokens. Day 2: Nothing happens. Day 3: The borrower calls asset.transfer(market, 20) to repay 20 tokens and then calls market.updateState() to update the market's state.

Now, let's analyze the execution process of updateState() on Day 3.

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

  function updateState() external nonReentrant {
    MarketState memory state = _getUpdatedState();
    _writeState(state);
  }

The core function is _getUpdatedState(). This function encompasses two tasks: (1) handling withdrawal requests and (2) invoking updateScaleFactorAndFees() to update the market's state. It's important to note that before processing withdrawal requests, there is also a call to updateScaleFactorAndFees (L366-L372). Therefore, this function updates the market's state twice.

Before the execution of the updateScaleFactorAndFees function (L366-L372), the value of state.lastInterestAccruedTimestamp is day 1, and expiry = state.pendingWithdrawalExpiry, considering Alice applied for a withdrawal of 20 tokens on day 1, so the value of expiry is day 2. and the value of state.scaledTotalSupply is 40. After the completion of updateScaleFactorAndFees(), the value of state.lastInterestAccruedTimestamp becomes day 2, and state.scaledTotalSupply remains 40. Then, the _processExpiredWithdrawalBatch() is called to handle the withdrawal request.

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

  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
>>          );
        emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
      }
>>      _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);
    }
  }

In the _processExpiredWithdrawalBatch function, the withdrawal request is obtained through WithdrawalBatch memory batch = _withdrawalData.batches[expiry];. Since the borrower returned 20 tokens on day 3, reserving some for paying the protocol fee, the remaining tokens can be used for Alice's withdrawal. So the value of availableLiquidity is greater than 0, the _applyWithdrawalBatchPayment function is called to handle Alice's withdrawal request. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L466-L492

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

    emit WithdrawalBatchExpired(
      expiry,
      batch.scaledTotalAmount,
      batch.scaledAmountBurned,
      batch.normalizedAmountPaid
    );

    if (batch.scaledAmountBurned < batch.scaledTotalAmount) {
      _withdrawalData.unpaidBatches.push(expiry);
    } else {
      emit WithdrawalBatchClosed(expiry);
    }

    state.pendingWithdrawalExpiry = 0;

    _withdrawalData.batches[expiry] = batch;
  }

In the _applyWithdrawalBatchPayment function, scaledAmountBurned represents the amount of tokens that Alice can withdraw. This value is approximately 20. Then, at L521, the value of state.scaledTotalSupply is updated, and after the update, the value of state.scaledTotalSupply is approximately equal to 20 (40 - 20 = 20). https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L498-L526

  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;

    // Burn market tokens to stop interest accrual upon withdrawal payment.
>>    state.scaledTotalSupply -= scaledAmountBurned;

    // Emit transfer for external trackers to indicate burn.
    emit Transfer(address(this), address(0), normalizedAmountPaid);
    emit WithdrawalBatchPayment(expiry, scaledAmountBurned, normalizedAmountPaid);
  }

Summary: Before the update, state.lastInterestAccruedTimestamp was day 1, and state.scaledTotalSupply was 40. After the update, state.lastInterestAccruedTimestamp became day 2, and state.scaledTotalSupply became 20. However, in reality, nothing happened on day 2, and the value of state.scaledTotalSupply should be 40, not 20.

Returning to the _getUpdatedState() function for the second market state update (day 3), before the update, the value of state.scaledTotalSupply is 20. The protocol fee is calculated as follows:

protocolFee = uint256(state.scaledTotalSupply).rayMul(uint256(state.scaleFactor).rayMul(protocolFeeRay));

Because the value of state.scaledTotalSupply is incorrect, it will result in a protocol fee that is smaller than the actual value. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L378-L387

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

Proof of Concept

The following is the test program.

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

import 'forge-std/Test.sol';

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

import '../src/WildcatArchController.sol';
import '../src/WildcatMarketControllerFactory.sol';
import '../src/WildcatMarketController.sol';
import '../src/WildcatSanctionsEscrow.sol';
import '../src/WildcatSanctionsSentinel.sol';
import '../src/market/WildcatMarket.sol';
import '../src/libraries/StringQuery.sol';
import '../src/libraries/MarketState.sol';


address constant alice = address(0xa11ce); 
address constant bob = address(0xb0b);
address constant cindy = address(0xb0c);
address constant daren = address(0xb0d);
address constant feeRecipient = address(0xfee); 
address constant borrower = address(0xb04405e4); 

uint128 constant DefaultMaximumSupply = 100_000e18;
uint16 constant DefaultInterest = 1000; 
uint16 constant DefaultDelinquencyFee = 1000; 
uint16 constant DefaultReserveRatio = 2000; 
uint32 constant DefaultGracePeriod = 2000; 
uint16 constant DefaultProtocolFeeBips = 1000; 
uint32 constant DefaultWithdrawalBatchDuration = 86400;

uint32 constant MinimumDelinquencyGracePeriod = 0; 
uint32 constant MaximumDelinquencyGracePeriod = 86_400; 

uint16 constant MinimumReserveRatioBips = 1_000; 
uint16 constant MaximumReserveRatioBips = 10_000;

uint16 constant MinimumDelinquencyFeeBips = 1_000; 
uint16 constant MaximumDelinquencyFeeBips = 10_000;

uint32 constant MinimumWithdrawalBatchDuration = 0; 
uint32 constant MaximumWithdrawalBatchDuration = 365 days;

uint16 constant MinimumAnnualInterestBips = 0; 
uint16 constant MaximumAnnualInterestBips = 10_000;



contract WildcatTest is Test {

    WildcatArchController public  archController;
    WildcatMarketControllerFactory public  marketControllerFactory;
    WildcatMarketController public marketController;
    WildcatSanctionsEscrow  public sanctionsEscrow;
    WildcatMarket public market;
    WildcatSanctionsSentinel public sanctionsSentinel;
    ChainalysisSanctionsList  public sanctionsList;

    MockERC20 public asset;
    MarketParameterConstraints internal constraints;


    function setUp() public {
        address marketControllerAddr;
        address marketAddr;
        
        asset = new MockERC20('Token', 'TKN', 18);
        asset.mint(alice, 1000_000e18);
        asset.mint(bob, 1000_000e18);
        asset.mint(cindy, 1000_000e18);
        asset.mint(borrower, 1000_000e18);


        archController = new WildcatArchController();
        archController.registerBorrower(borrower);

        sanctionsList = new ChainalysisSanctionsList();
        sanctionsSentinel = new WildcatSanctionsSentinel(address(archController), address(sanctionsList));

 
        _resetConstraints();
        marketControllerFactory = new WildcatMarketControllerFactory(address(archController), address(sanctionsSentinel), constraints);
        archController.registerControllerFactory(address(marketControllerFactory));
        marketControllerFactory.setProtocolFeeConfiguration(feeRecipient, address(0), 0, DefaultProtocolFeeBips);


        vm.startPrank(borrower);
        (marketControllerAddr, marketAddr) = marketControllerFactory.deployControllerAndMarket(
            "Wildcat-",
            "WC-",
            address(asset),
            DefaultMaximumSupply,
            DefaultInterest,
            DefaultDelinquencyFee,
            DefaultWithdrawalBatchDuration,
            DefaultReserveRatio,
            DefaultGracePeriod
        );
        vm.stopPrank();
        
        marketController = WildcatMarketController(marketControllerAddr);
        market = WildcatMarket(marketAddr);


        vm.prank(alice);
        asset.approve(address(market), type(uint256).max);
        vm.prank(bob);
        asset.approve(address(market), type(uint256).max);
        vm.prank(cindy);
        asset.approve(address(market), type(uint256).max);
        vm.prank(borrower);
        asset.approve(address(market), type(uint256).max);


 
        vm.startPrank(address(borrower));
        address[] memory addrs = new address[](3);
        addrs[0] = alice;
        addrs[1] = bob;
        addrs[2] = cindy;
        marketController.authorizeLenders(addrs);


        address[] memory markets = new address[](1);
        markets[0] = address(market);
        marketController.updateLenderAuthorization(alice, markets);
        marketController.updateLenderAuthorization(bob, markets);
        marketController.updateLenderAuthorization(cindy, markets);
        vm.stopPrank();

    }

  function _resetConstraints() internal {

    constraints = MarketParameterConstraints({
      minimumDelinquencyGracePeriod: MinimumDelinquencyGracePeriod,
      maximumDelinquencyGracePeriod: MaximumDelinquencyGracePeriod,
      minimumReserveRatioBips: MinimumReserveRatioBips,
      maximumReserveRatioBips: MaximumReserveRatioBips,
      minimumDelinquencyFeeBips: MinimumDelinquencyFeeBips,
      maximumDelinquencyFeeBips: MaximumDelinquencyFeeBips,
      minimumWithdrawalBatchDuration: MinimumWithdrawalBatchDuration,
      maximumWithdrawalBatchDuration: MaximumWithdrawalBatchDuration,
      minimumAnnualInterestBips: MinimumAnnualInterestBips,
      maximumAnnualInterestBips: MaximumAnnualInterestBips
    });
  }


    function test_ProtocolFeeWithWithdrawal() public {

        // alice deposits 50_000e18
        vm.prank(alice);
        market.deposit(50_000e18);

        // borrower takes away 40_000e18
        vm.prank(borrower);
        market.borrow(40_000e18);

        // alice requests to withdraw 10_000e18
        vm.prank(alice);
        market.queueWithdrawal(10_000e18); 

        skip(DefaultWithdrawalBatchDuration);
        // alice gets 10_000e18
        vm.prank(alice);
        market.executeWithdrawal(alice, uint32(block.timestamp));
        
        // alice requests 20_000e18
        vm.prank(alice);
        market.queueWithdrawal(20_000e18);

        // skip 2 days
        skip(DefaultWithdrawalBatchDuration);
        skip(DefaultWithdrawalBatchDuration);
        
        vm.prank(borrower);
        asset.transfer(address(market), 20_000e18);
        market.updateState();
        console.log("protocolFee: %s", market.accruedProtocolFees());

    }

}


contract ChainalysisSanctionsList {
    mapping(address => bool) sanctionsList;

    function sanction(address addr, bool flag) external {
        sanctionsList[addr] = flag;
    }

    function isSanctioned(address addr) external view returns (bool) {
        return sanctionsList[addr];
    }
}

The test results are as follows, and the final value of the protocol fee is 2741573981180866228.

Running 1 test for test/WildcatTest.t.sol:WildcatTest
[PASS] test_ProtocolFeeWithWithdrawal() (gas: 425214)
Logs:
  protocolFee: 2741573981180866228

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

If you remove this line asset.transfer(address(market), 20_000e18);, since there are no tokens in the market, the _getUpdatedState function will no longer process Alice's withdrawal request. When running the test program again, the protocol fee value is 3289459121609366972.

        // alice requests 20_000e18
        vm.prank(alice);
        market.queueWithdrawal(20_000e18);

        // skip 2 days
        skip(DefaultWithdrawalBatchDuration);
        skip(DefaultWithdrawalBatchDuration);
        
        //vm.prank(borrower);
        //asset.transfer(address(market), 20_000e18);
        market.updateState();
        console.log("protocolFee: %s", market.accruedProtocolFees());

Running 1 test for test/WildcatTest.t.sol:WildcatTest
[PASS] test_ProtocolFeeWithWithdrawal() (gas: 374443)
Logs:
  protocolFee: 3289459121609366972

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.90ms

Conclusion: Due to issues in the _getUpdatedState function for updating the market statue, it results in a protocol fee that is less than expected.

Tools Used

Foundry

Recommended Mitigation Steps

Modify the code for updating the market state to ensure that all data aligns with the actual scenario.

Assessed type

Context

uint16 constant DefaultDelinquencyFee = 0; 
uint16 constant MinimumDelinquencyFeeBips = 0; 
uint32 constant DefaultWithdrawalBatchDuration = 365 days;

// ...

  function test_ProtocolFeeWithWithdrawal() public {
      // ...
      assertEq(market.accruedProtocolFees(), 1_132.4e18);
   }