The Wildcat Protocol - nisedo's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L182-L190 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L112-L126 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L77-L81

Vulnerability details

Impact

The WildcatMarketController.updateLenderAuthorization function allows an attacker to arbitrarily set any address as a lender with withdrawal permission (AuthRole.WithdrawOnly). This critical vulnerability allows an attacker to illegitimately grant themselves withdrawal permissions, thereby enabling them to execute the queueWithdrawal function and illicitly withdraw funds from the contract.

Proof of Concept

In the WildcatMarketController contract, the updateLenderAuthorization function is marked as external with no access control, allowing it to be called by any external entity. By passing in an array of markets and a lender address, it attempts to update the authorization status of the lender for each of the markets.

File: WildcatMarketController.sol
  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));
    }
  }

The function checks if the lender is already authorized in _authorizedLenders.contains(lender). If not authorized, it calls updateAccountAuthorization with the lender address and false, effectively setting the address to the AuthRole.WithdrawOnly role.

File: WildcatMarketConfig.sol
  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);
  }

Once an external malicious actor has been assigned the AuthRole.WithdrawOnly role through this vulnerability, they can then call the queueWithdrawal function in the WildcatMarketWithdrawals contract. This function checks for the AuthRole.WithdrawOnly role, and since the attacker has assigned it to themselves, they can proceed to withdraw funds illegitimately.

File: WildcatMarketWithdrawals.sol
  function queueWithdrawal(uint256 amount) external nonReentrant {
    MarketState memory state = _getUpdatedState();


    // Cache account data and revert if not authorized to withdraw.
    Account memory account = _getAccountWithRole(msg.sender, AuthRole.WithdrawOnly);

Recommended Mitigation Steps

1. Implement strict access control on the updateLenderAuthorization function to ensure that only trusted entities as the Borrower or the Controller can call it.
2. If a lender isn't authorized by _authorizedLenders.contains(lender), it shouldn't add him as a lender with withdrawal permissions AuthRole.WithdrawOnly. Instead, it should revert the transaction.

Assessed type

Access Control

QA1: Remove unused imports

File: WildcatMarketBase.sol
- import "../libraries/FeeMath.sol";

File: WildcatMarketControllerFactory.sol
- import {EnumerableSet} from "openzeppelin/contracts/utils/structs/EnumerableSet.sol";
- import "./interfaces/WildcatStructsAndEnums.sol";
- import "./interfaces/IWildcatMarketController.sol";
- import "./interfaces/IWildcatArchController.sol";
- import "./libraries/LibStoredInitCode.sol";
- import "./libraries/MathUtils.sol";
- import "./market/WildcatMarket.sol";

File: WildcatMarketWithdrawals.sol
- import "../libraries/MarketState.sol";
- import "../libraries/FeeMath.sol";
- import "../libraries/FIFOQueue.sol";
- import "../interfaces/IWildcatSanctionsSentinel.sol";

File: WildcatMarketConfig.sol
- import "../libraries/FeeMath.sol";

File: WildcatMarket.sol
- import "../libraries/FeeMath.sol";

File: WildcatSanctionsSentinel.sol
- import {SanctionsList} from "./libraries/Chainalysis.sol";

File: WildcatSanctionsEscrow.sol
- import {IChainalysisSanctionsList} from "./interfaces/IChainalysisSanctionsList.sol";
- import {SanctionsList} from "./libraries/Chainalysis.sol";

QA2: Remove todo comments

FIFOQueue.sol#L10-L12

File: FIFOQueue.sol
10: // @todo - make array tightly packed for gas efficiency with multiple reads/writes
11: //         also make a memory version of the array with (nextIndex, startIndex, storageSlot)
12: //         so that multiple storage reads aren't required for tx's using multiple functions

QA3: Remove useless unchecked block

ReentrancyGuard.sol#L64-L66

File: ReentrancyGuard.sol
64    unchecked {
65      _reentrancyGuard = _ENTERED;
66    }

QA4: Incorrect Documentation on WildcatMarketConfig.nukeFromOrbit() Access Control

Description

The documentation for the function WildcatMarketConfig.nukeFromOrbit() claims that the function will revert if the caller is not the sentinel.

Reverts if: the caller is not the sentinel.

However, there is no check to ensure that the caller is the sentinel in the code. This can lead to potential misuse or misunderstanding of the function's access control, as the function relies on the result of IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress) to determine behavior, rather than directly validating the caller's identity.

File: WildcatMarketConfig.sol
  function nukeFromOrbit(address accountAddress) external nonReentrant {
    if (!IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
      revert BadLaunchCode();
    }
    MarketState memory state = _getUpdatedState();
    _blockAccount(state, accountAddress);
    _writeState(state);
  }

Recommendations

1. Update the function WildcatMarketConfig.nukeFromOrbit() to include an explicit check to verify that the caller is indeed the sentinel. This can be done using a simple if or require statement.
2. Update the documentation to accurately reflect the function's behavior.

QA5: Lack of Access Control on WildcatMarket.collectFees()

Description

The function collectFees allows for the collection of accrued protocol fees. However, there is no access control implemented on this function, meaning any external actor can call it. This presents a security risk as potentially malicious or unauthorized users can trigger the fee collection, leading to unexpected behavior or potential loss of funds.

File: WildcatMarket.sol
  function collectFees() external nonReentrant {
    MarketState memory state = _getUpdatedState();
    if (state.accruedProtocolFees == 0) {
      revert NullFeeAmount();
    }
    uint128 withdrawableFees = state.withdrawableProtocolFees(totalAssets());
    if (withdrawableFees == 0) {
      revert InsufficientReservesForFeeWithdrawal();
    }
    state.accruedProtocolFees -= withdrawableFees;
    _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
    emit FeesCollected(withdrawableFees);
  }

Recommendations

1. Implement access control on the collectFees function. Ideally, only authorized entities (such as the protocol's admin or designated fee collector) should be allowed to collect fees.
2. Use a modifier like onlyAuthorized or a similar naming convention to ensure that only specific addresses or roles can call this function.

Note to the Judge:

All gas optimization estimations presented in this report have been determined using the forge snapshot feature. This tool allows for a precise analysis of gas consumption before and after the proposed changes, ensuring the accuracy of the savings mentioned.

GAS1: State variables that are used multiple times in a function should be cached in stack variables

By caching state variables in stack variables, we reduce the need to frequently access storage, thereby saving gas.

File: WildcatSanctionsEscrow.sol
  function releaseEscrow() public override {
    if (!canReleaseEscrow()) revert CanNotReleaseEscrow();

    uint256 amount = balance();
    
+   address _account = account;
+   address _asset = asset;

-   IERC20(asset).transfer(account, amount);
+   IERC20(_asset).transfer(_account, amount);

-   emit EscrowReleased(account, asset, amount);
+   emit EscrowReleased(_account, _asset, amount);
  }

Overall gas change: -194421 (-0.230%)

GAS2: Pack struct

Reordering the structure of the MarketState ensures that the struct variables are packed efficiently, minimizing storage costs.

File: MarketState.sol
	struct MarketState {
		uint128 maxTotalSupply;                 // 16 bytes
		uint128 accruedProtocolFees;            // 16 bytes

		uint128 normalizedUnclaimedWithdrawals; // 16 bytes
		uint104 scaledTotalSupply;              // 13 bytes
		uint16 annualInterestBips;              // 2 bytes
		bool isClosed;                          // 1 byte
		
		uint104 scaledPendingWithdrawals;       // 13 bytes
		uint112 scaleFactor;                    // 14 bytes
		uint16 reserveRatioBips;                // 2 bytes
		bool isDelinquent;                      // 1 byte

		uint32 pendingWithdrawalExpiry;         // 4 bytes
		uint32 timeDelinquent;                  // 4 bytes
		uint32 lastInterestAccruedTimestamp;    // 4 bytes
	}

GAS3: WildcatMarket.closeMarket() function can be optimized

By moving condition checks up in the execution flow, we save on computational steps and improve the gas efficiency of the closeMarket() function.

File: WildcatMarket.sol
  function closeMarket() external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
+    if (_withdrawalData.unpaidBatches.length() > 0) {
+      revert CloseMarketWithUnpaidWithdrawals();
+    }
    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);
  }

Overall gas change: -1524 (-0.002%)

GAS4: WildcatMarketConfig.setAnnualInterestBips() function can be optimized

By moving condition checks up in the execution flow, we save on computational steps and improve the gas efficiency of the setAnnualInterestBips() function.

File: WildcatMarketConfig.sol
  function setAnnualInterestBips(uint16 _annualInterestBips) public onlyController nonReentrant {
+   if (_annualInterestBips > BIP) {
+     revert InterestRateTooHigh();
+   }
    MarketState memory state = _getUpdatedState();

-   if (_annualInterestBips > BIP) {
-     revert InterestRateTooHigh();
-   }

    state.annualInterestBips = _annualInterestBips;
    _writeState(state);
    emit AnnualInterestBipsUpdated(_annualInterestBips);
  }

Overall gas change: -7567 (-0.009%)

GAS6: WildcatMarketConfig.stunningReversal() function can be optimized

By moving condition checks up in the execution flow, we save on computational steps and improve the gas efficiency of the stunningReversal() function.

File: WildcatMarketConfig.sol
  function stunningReversal(address accountAddress) external nonReentrant {
+   if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
+     revert NotReversedOrStunning();
+   }
    Account memory account = _accounts[accountAddress];
    if (account.approval != AuthRole.Blocked) {
      revert AccountNotBlocked();
    }


-   if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
-     revert NotReversedOrStunning();
-   }


    account.approval = AuthRole.Null;
    emit AuthorizationStatusUpdated(accountAddress, account.approval);


    _accounts[accountAddress] = account;
  }

GAS7: WildcatMarketWithdrawals.queueWithdrawal() function can be optimized

By moving condition checks up in the execution flow, we save on computational steps and improve the gas efficiency of the queueWithdrawal() function.

File: WildcatMarketWithdrawals.sol
  function queueWithdrawal(uint256 amount) external nonReentrant {
    MarketState memory state = _getUpdatedState();

+   uint104 scaledAmount = state.scaleAmount(amount).toUint104();
+   if (scaledAmount == 0) {
+     revert NullBurnAmount();
+   }

    // Cache account data and revert if not authorized to withdraw.
    Account memory account = _getAccountWithRole(msg.sender, AuthRole.WithdrawOnly);


-   uint104 scaledAmount = state.scaleAmount(amount).toUint104();
-   if (scaledAmount == 0) {
-     revert NullBurnAmount();
-   }


    // Reduce caller's balance and emit transfer event.
    account.scaledBalance -= scaledAmount;
    _accounts[msg.sender] = account;
    emit Transfer(msg.sender, address(this), amount);


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

Overall gas change: -9258 (-0.011%)

GAS8: WildcatMarketWithdrawals.executeWithdrawal() function can be optimized

By moving condition checks up in the execution flow, we save on computational steps and improve the gas efficiency of the executeWithdrawal() function.

File: WildcatMarketWithdrawals.sol
  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;

+   if (normalizedAmountWithdrawn == 0) {
+     revert NullWithdrawalAmount();
+   }

    status.normalizedAmountWithdrawn = newTotalWithdrawn;
    state.normalizedUnclaimedWithdrawals -= normalizedAmountWithdrawn;


-   if (normalizedAmountWithdrawn == 0) {
-     revert NullWithdrawalAmount();
-   }


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


    emit WithdrawalExecuted(expiry, accountAddress, normalizedAmountWithdrawn);


    // Update stored state
    _writeState(state);


    return normalizedAmountWithdrawn;
  }

gas: -413 (-0.070%)

GAS9: The function MathUtils.calculateLinearInterestFromBips() is duplicated and can be deleted

The function calculateLinearInterestFromBips() is duplicated in FeeMath.sol and MathUtils.sol. Removing the duplicated calculateLinearInterestFromBips() function in MathUtils.sol and using the one in FeeMath.sol eliminates redundancy and saves gas.

File: FeeMath.sol
20:     function calculateLinearInterestFromBips(uint256 rateBip, uint256 timeDelta)
21:         internal
22:         pure
23:         returns (uint256 result)
24:     {
25:         uint256 rate = rateBip.bipToRay();
26:         uint256 accumulatedInterestRay = rate * timeDelta;
27:         unchecked {
28:             return accumulatedInterestRay / SECONDS_IN_365_DAYS;
29:         }
30:     }

File: MathUtils.sol
30:     function calculateLinearInterestFromBips(
31:         uint256 rateBip,
32:         uint256 timeDelta
33:     ) internal pure returns (uint256 result) {
34:         uint256 rate = rateBip.bipToRay();
35:         uint256 accumulatedInterestRay = rate * timeDelta;
36:         unchecked {
37:             return accumulatedInterestRay / SECONDS_IN_365_DAYS;
38:         }
39:     }

The MathUtils.calculateLinearInterestFromBips() function can be deleted to save gas and optimize the code. Don't forget to update the references to the function in the code by replacing them with the FeeMath.calculateLinearInterestFromBips() function instead of MathUtils.calculateLinearInterestFromBips() function.

GAS10: The function MathUtils._applyWithdrawalBatchPayment() can be optimized

By moving condition checks up in the execution flow, we save on computational steps and improve the gas efficiency of the _applyWithdrawalBatchPayment() function.

File: WildcatMarketBase.sol
  function _applyWithdrawalBatchPayment(
    WithdrawalBatch memory batch,
    MarketState memory state,
    uint32 expiry,
    uint256 availableLiquidity
  ) internal {
+   uint104 scaledAmountOwed = batch.scaledTotalAmount - batch.scaledAmountBurned;
+   // Do nothing if batch is already paid
+   if (scaledAmountOwed == 0) {
+     return;
+   }
    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);
  }

Overall gas change: -5380 (-0.006%)