The Wildcat Protocol - TrungOre's results

Lines of code

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

Vulnerability details

Impact

After the market closes, users are unable to receive their rightful assets, resulting in a loss of funds for them.

Proof of Concept

The WildcatMarket.closeMarket() function is used to close the market. Specifically, it sets the market's Annual Percentage Rate (APR) to 0% and marks the market as closed. Additionally, it performs the necessary asset transfers between the borrower and the market to ensure that the market is settled with no surplus funds left in the contract.

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();
    }
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from the borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
      // Transfer excess assets to the borrower
      asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
    _writeState(state);
    emit MarketClosed(block.timestamp);
}

However, a flaw arises in this function. While the function correctly resets the value of annualInterestBips to 0, it does not address the delinquencyFeeBips, which remains unchanged. Consequently, if the market is closed and state.timeDelinquent surpasses the delinquencyGracePeriod, the interest continues to accrue, causing the scaleFactor to grow. As a result, the total assets in the market may not be sufficient for all users, as their corresponding assets continue to increase even after the market is closed.

To illustrate this issue, we can place the following test to WildMarket.t.sol

function test_closeMarket_CloseMarketBugInterestStillIncrease() 
    external
    asAccount(address(controller)) 
  {
    // both alice and bob deposit 5e17 tokens
    _deposit(alice, 5e17);
    _deposit(bob, 5e17);
    assertEq(market.balanceOf(alice), market.balanceOf(bob));

    // borrower borrows 80% 
    _borrow(8e17);

    // market become delinquent 
    fastForward(10);
    market.updateState();
    assertEq(market.previousState().isDelinquent, true);

    // Advance time past the `delinquencyGracePeriod` to commence accruing the `delinquentFee`
    fastForward(market.delinquencyGracePeriod() * 2);
    market.updateState();

    // close market 
    asset.mint(borrower, 2e18);
    _approve(borrower, address(market), type(uint256).max);
    market.closeMarket(); 
    market.collectFees(); // collect fee for the protocol 

    // expected amount that alice and bob should receive when making a withdrawal 
    uint expectedAmountOfAlice = market.balanceOf(alice);
    uint expectedAmountOfBob = market.balanceOf(bob);

    // deliquentFee is still != 0 --> scaleFactor increases as time passes 
    uint oldScaleFactor = market.previousState().scaleFactor;
    fastForward(market.previousState().timeDelinquent);
    market.updateState();
    assertGt(market.previousState().scaleFactor, oldScaleFactor); // oldScaleFactor < newScaleFactor

    // alice makes a withdrawal 
    uint receivedAmountOfAlice = _oreWithdraw(alice);

    // bob makes a withdrawal 
    uint receivedAmountOfBob = _oreWithdraw(bob);

    // assertion here 
    assertGt(receivedAmountOfAlice, receivedAmountOfBob); // alice receive more than bob 
    assertGt(receivedAmountOfAlice, expectedAmountOfAlice); // alice receive more than expected 
    assertLt(receivedAmountOfBob, expectedAmountOfBob); // bob receive less than expected 
  }

  function _oreWithdraw(address user) internal asAccount(user) returns (uint receivedAmount) {
    uint balanceBefore = asset.balanceOf(user);

    market.queueWithdrawal(market.balanceOf(user));
    uint32 expiry = market.previousState().pendingWithdrawalExpiry;

    fastForward(parameters.withdrawalBatchDuration);
    market.executeWithdrawal(user, expiry);

    receivedAmount = asset.balanceOf(user) - balanceBefore;
  }

In this test, both Alice and Bob initially deposit the same amount of assets into the market. It demonstrates that the scaleFactor continues to increase even after the market is closed (line 36). When Alice and Bob decide to withdraw their funds, it becomes apparent that:

• Alice receives more tokens than Bob, despite their initially equal deposits (line 45)
• Alice receives more tokens than she should have received when the market closed (line 46)
• Bob receives fewer tokens than he should have received when the market closed (line 47).

Tools Used

Manual review

Recommended Mitigation Steps

Consider transforming the delinquencyFeeBips into a regular variable (non-immutable) and resetting it to 0 upon the invocation of the closeMarket() function.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The inability to close the market results in the continued accrual of interest, even after the borrower has fully repaid the debt, leading to financial losses for the borrower.

Proof of Concept

The WildcatMarket.closeMarket() function serves the purpose of closing the market, specifically by performing the following actions:

• Setting the market Annual Percentage Rate (APR) to 0%
• Setting the state.isClosed flag to true
• Transferring tokens between the borrower and the market to ensure that the market is entirely settled, leaving no surplus tokens in the market.

/// link = 
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();
    }
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from the borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
      // Transfer excess assets to the borrower
      asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
    _writeState(state);
    emit MarketClosed(block.timestamp);
}

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

Notably, it's crucial to note that this function can only be triggered by the controller, which represents an object of the WildcatMarketController contract. However, it's apparent that there is no method within the WildcatMarketController contract that can initiate the closeMarket() function.

The absence of a trigger method for the closeMarket() function in the controller could result in the market being unable to close properly. This failure to close the market might prevent the APR from resetting to 0%, even after the borrower has repaid all the debt. Consequently, this situation could lead to financial losses for borrowers since they would still be obligated to pay the interest.

Tools Used

Manual review

Recommended Mitigation Steps

Modify the WildcatMarket.closeMarket() function to permit invocation by the borrower.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L164-L180 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L72-L78

Vulnerability details

Impact

Sanctioned account can prevent funds from being sent to escrow

Proof of Concept

When an account is sanctioned by the ChainalysisSanctionsList contract, the account can be assigned the Blocked role if someone triggers the WildMarketConfig.nukeFromOrbit() function in the market. However, there are several reasons why this function may not be regularly called:

1. There is no incentive for external users not participating in the market to call the function, and it also wastes their gas.
2. Managing numerous markets is challenging for both the protocol and the borrower, and updating all sanctioned accounts across all markets incurs substantial gas fees.

Consequently, the protocol needs to develop a mechanism to prevent sanctioned accounts from absconding with tokens without requiring the triggering of the WildMarketConfig.nukeFromOrbit() function for these accounts. As a solution, every time an account triggers WildcatMarketWithdrawals.executeWithdrawal() to retrieve their tokens, the function executes an external call IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress) to check if the accountAddress is sanctioned, rather than simply checking the account's role. If the account is sanctioned, the funds are then locked in escrow until the account is unblocked.

// link = https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L164-L180
function executeWithdrawal(
    address accountAddress,
    uint32 expiry
) external nonReentrant returns (uint256) {
    ... 
        
    if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
      _blockAccount(state, accountAddress);
      address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
        accountAddress,
        borrower,
        address(asset)
      );
      asset.safeTransfer(escrow, normalizedAmountWithdrawn);
      emit SanctionedAccountWithdrawalSentToEscrow(
        accountAddress,
        escrow,
        expiry,
        normalizedAmountWithdrawn
      );
    } else {
      asset.safeTransfer(accountAddress, normalizedAmountWithdrawn);
    }
    
    ... 
}

However, a sanctioned account can easily circumvent this mechanism and prevent funds from being locked in escrow by utilizing another address. Here is the complete strategy for an attacker to follow:

1. The attacker holds x shares of a market in wallet A.
2. Wallet A is added to the sanctioned list of the ChainalysisSanctionsList contract.
3. The attacker creates a new Externally Owned Account (EOA) wallet, referred to as wallet B.
4. The attacker uses wallet A to transfer x shares to wallet B.
5. The attacker uses wallet B to trigger the function WildcatMarketController.updateLenderAuthorization() -> WildcatMarketConfig.updateAccountAuthorization(). Since wallet B has not yet been added to _authorizedLenders, it receives the WithdrawOnly role.
6. The attacker uses wallet B to call queueWithdrawal() -> executeWithdrawal(). As wallet B is not sanctioned, the tokens are not transferred to the escrow contract, allowing the attacker to immediately take control of the tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing an external call to IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress) within the WildcatMarketToken._transfer() function to verify whether the accountAddress is sanctioned before making any balance modifications.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

An address with the Null role can withdraw tokens from the market.

Proof of Concept

As stated in the third point under "Main Invariants" in the contest README:

• The assets of a market should never be able to be withdrawn by anyone that is not the borrower or a lender with either the WithdrawOnly or DepositAndWithdraw role.

This implies that the withdrawal cannot be carried out by an account with the Null or Blocked role. However, an address with the Null role can easily bypass this invariant by utilizing the WildcatMarketController.updateLenderAuthorization() function.

/// link = https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L182-L190
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();
    }
    WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
  }
}

/// link = https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L112-L126
function updateAccountAuthorization(
  address _account,
  bool _isAuthorized
) external onlyController nonReentrant {
  MarketState memory state = _getUpdatedState();
  Account memory account = _getAccount(_account);
  if (_isAuthorized) {
    account.approval = AuthRole.DepositAndWithdraw;
  } else {
    account.approval = AuthRole.WithdrawOnly;
  }
  _accounts[_account] = account;
  _writeState(state);
  emit AuthorizationStatusUpdated(_account, account.approval);
}

As seen, for each pair of (lender, market), the WildcatMarketController.updateLenderAuthorization() function makes an external call to the WildcatMarketConfig.updateAccountAuthorization() function to update the account's authorization. Since the account with the Null role is not included in the _authorizedLenders set, the input _isAuthorized of the updateAccountAuthorization function will be set as false. Consequently, the account's role will change from Null to WithdrawOnly. Thus, the account can now execute the withdrawal and break the invariant.

Tools Used

Manual review

Recommended Mitigation Steps

Consider limiting the caller of the WildcatMarketController.updateLenderAuthorization() function to only the borrower.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

• Lenders loses their tokens from escrow
• Borrower can take control of lenders's tokens in the escrow

Proof of Concept

The WildcatSanctionsSentinel.createEscrow(borrower, account, asset) function is utilized for generating a fresh WildcatSanctionsEscrow contract dedicated to the specified borrower, account, and asset. The order of passing parameters indicates that the borrower address is expected before the account address.

/// link = https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L99
function createEscrow(
    address borrower,
    address account,
    address asset
) public override returns (address escrowContract) {
    ...
}

However, during the execution of the WildcatMarketWithdrawals.executeWithdrawal() function, when the accountAddress is sanctioned, the escrow is created with the following code:

/// link = https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L166-L170
address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
    accountAddress,
    borrower,
    address(asset)
  );

The issue lies in the fact that the accountAddress is placed before the borrower address in the createEscrow() function, contrary to the expected order. Consequently, this incorrect arrangement results in the IMMUTABLE variables for the newly created WildcatSanctionsEscrow as described below:

sentinel = msg.sender
borrower = accountAddress
account = borrower
asset = address(asset)

This flawed implementation can lead to a serious predicament if, when the accountAddress is unsanctioned and triggers WildcatSanctionsEscrow.releaseEscrow() to retrieve the tokens, the amount is received by the borrower instead of the rightful accountAddress. This occurs because the WildcatSanctionsEscrow.account has become the borrower due to the incorrect parameter order.

Tools Used

Manual review

Recommended Mitigation Steps

Modify the call to createEscrow as follows:

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

• Break the invariant of the protocol
• Borrowers can borrow asset from market without paying any interest

Proof of Concept

The function WildcatMarketController.setAnnualInterestBips() is employed to adjust the interest rate for a market.

function setAnnualInterestBips(
    address market,
    uint16 annualInterestBips
) external virtual onlyBorrower onlyControlledMarket(market) {
    // If the 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);
    }

    WildcatMarket(market).setAnnualInterestBips(annualInterestBips);
}

The issue arises when the new annualInterestBips value is not verified against the boundaries of [MinimumAnnualInterestBips, MaximumAnnualInterestBips]. This absence of validation checks violates point 1 in the "Main Invariants" section of the contest README.

Consequently, this vulnerability allows the borrower to set the interest rate to an extremely low value, resulting in a profitable scenario for the borrower when borrowing assets from the market.

Tools Used

Manual review

Recommended Mitigation Steps

Consider checking the new annualInterestBips within the bound [MinimumAnnualInterestBips, MaximumAnnualInterestBips]

Assessed type

Other