The Wildcat Protocol - jasonxiale's results

Lines of code

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

Vulnerability details

Impact

According to WildcatMarket.closeMarket's definition, the function can only be called by controller, but within WildcatMarketController.sol contract, WildcatMarket.closeMarket is not called. In such case, it makes the borrower can't close the market, if the borrower pays more than his/her debt, event he/she can set annualInterestBips=0 and setReserveRatioBips=0, the remaining token will be stucked in the market.

Proof of Concept

WildcatMarket.closeMarket is defined as:

  function closeMarket() external onlyController nonReentrant { <----------------- only controller can call this funciton
    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 borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
      // Transfer excess assets to borrower
      asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
    _writeState(state);
    emit MarketClosed(block.timestamp);
  }

As above code shows, onlyController modifier is used here, which makes the function can only be called by controller, but within WildcatMarketController.sol contract, WildcatMarket.closeMarket is not called

Tools Used

VIM

Recommended Mitigation Steps

Adding WildcatMarket.closeMarket related code in WildcatMarketController.sol

diff --git a/src/WildcatMarketController.sol b/src/WildcatMarketController.sol
index 55f62f6..a39565b 100644
--- a/src/WildcatMarketController.sol
+++ b/src/WildcatMarketController.sol
@@ -487,6 +487,13 @@ contract WildcatMarketController is IWildcatMarketControllerEventsAndErrors {
     WildcatMarket(market).setAnnualInterestBips(annualInterestBips);
   }
 
+  function closeMarket(
+    address market
+  ) external virtual onlyBorrower onlyControlledMarket(market) {
+
+    WildcatMarket(market).closeMarket();
+  }
+
   function resetReserveRatio(address market) external virtual {
     TemporaryReserveRatio memory tmp = temporaryExcessReserveRatio[market];
     if (tmp.expiry == 0) {

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L36-L39 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L42-L77 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L137-L188

Vulnerability details

Impact

When lender deposits into the market, he/she will get some WildcatMarketToken, because WildcatMarketToken.transfer doesn't stop sanctioned lender from transferring the WildcatMarketToken, an sanctioned lender can transfer his token to another lender(who is not sanctioned), and then the second lender can withdraw the assets.

Proof of Concept

1. When lender deposits into the market, he/she will get some WildcatMarketToken at L63-L65

  function depositUpTo(
    uint256 amount
  ) public virtual nonReentrant returns (uint256 /* actualAmount */) {
    ...
    // Cache account data and revert if not authorized to deposit.
    Account memory account = _getAccountWithRole(msg.sender, AuthRole.DepositAndWithdraw); <---- Here mint WildcatMarketToken
    account.scaledBalance += scaledAmount;
    _accounts[msg.sender] = account;
    ...
  }

2. WildcatMarketToken._transfer doesn't check if from or to address are sanctioned

  function _transfer(address from, address to, uint256 amount) internal virtual {
    MarketState memory state = _getUpdatedState();
    uint104 scaledAmount = state.scaleAmount(amount).toUint104();

    if (scaledAmount == 0) {
      revert NullTransferAmount();
    }

    Account memory fromAccount = _getAccount(from);
    fromAccount.scaledBalance -= scaledAmount;
    _accounts[from] = fromAccount;

    Account memory toAccount = _getAccount(to);
    toAccount.scaledBalance += scaledAmount;
    _accounts[to] = toAccount;

    _writeState(state);
    emit Transfer(from, to, amount);
  }

3. Normally, if Alice(the sanctioned lender) wants to withdraw her assets, she will call WildcatMarketWithdrawals.queueWithdrawal, and after the withdrawalBatchDuration she calls WildcatMarketWithdrawals.executeWithdrawal to withdraw her asset. But because she is sanctioned. the if branch in L164-L178 is executed, and her asset will be transferred to an escrow. To avoid her asset being transferred to escrow, she can call WildcatMarketToken.transfer to transfer her asset to another lender address she owns, and withdraw her asset as other users

Tools Used

VIM

Recommended Mitigation Steps

Assessed type

Other

[L-01] WildcatMarketConfig.setMaxTotalSupply is never be called

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

WildcatMarketConfig.setMaxTotalSupply has a modifier which only can be called by controller, but after searching through controller's code, the function is never be called.

[L-02] WildcatMarketControllerFactory.setProtocolFeeConfiguration missing UpdateProtocolFeeConfiguration event

File: https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketControllerFactory.sol#L195C12-L217 Accoring to UpdateProtocolFeeConfiguration, an event UpdateProtocolFeeConfiguration is defined for function setProtocolFeeConfiguration, but the event is never used

[L-03] WildcatMarketControllerFactory.deployController missing NewController event

File: https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketControllerFactory.sol#L282-L301 According to comments and NewController, an event NewController should be emitted in function deployController, but the event is never emitted

[L-04] In WildcatMarketController.updateLenderAuthorization, _authorizedLenders.contains(lender) can cached

File: https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L188 In WildcatMarketController.updateLenderAuthorization, _authorizedLenders.contains(lender) can be cached to save some gas within for-loop

diff --git a/src/WildcatMarketController.sol b/src/WildcatMarketController.sol
index 55f62f6..7ce3c6f 100644
--- a/src/WildcatMarketController.sol
+++ b/src/WildcatMarketController.sol
@@ -180,12 +180,13 @@ contract WildcatMarketController is IWildcatMarketControllerEventsAndErrors {
    *      status.
    */
   function updateLenderAuthorization(address lender, address[] memory markets) external {
+    bool _isAuthorized = _authorizedLenders.contains(lender);
     for (uint256 i; i < markets.length; i++) {
       address market = markets[i];
       if (!_controlledMarkets.contains(market)) {
         revert NotControlledMarket();
       }
-      WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
+      WildcatMarket(market).updateAccountAuthorization(lender, _isAuthorized);
     }
   }

[L-05] MathUtils.calculateLinearInterestFromBips can be removed

File: https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/MathUtils.sol#L30-L39

MathUtils.calculateLinearInterestFromBips is only called by FeeMath.sol#L34-L37, but the same function calculateLinearInterestFromBips is defined in FeeMath.sol, which means we can remove MathUtils.calculateLinearInterestFromBips

[L-06] LibStoredInitCode.create2WithStoredInitCode doesn't check create2 return value

File: https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/LibStoredInitCode.sol#L115 According to yul document, create2 might return 0, so the function should check create2 return value