A borrower cannot prevent the transfer of market tokens if the market allows deposits only when the lenders have valid credentials

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L893-L923

Vulnerability details

Impact

A borrower cannot prevent the transfer of market tokens if the market allows deposits only when the lenders have valid credentials

Proof of Concept

The Wildcat protocol states that a borrower can disable the token transferring function when deploying a new market
https://docs.wildcat.finance/using-wildcat/day-to-day-usage/borrowers#market-type:

Transferability restrictions: should the debt token issued by your Wildcat market be freely transferable to any recipient, restricted only to addresses that have credentials/authorisation to engage with the market, or further constrained to only move to/from the market itself?

https://docs.wildcat.finance/using-wildcat/day-to-day-usage/lenders#making-deposits:

Depending on the constraints placed upon the markets, lenders may be able to transfer market tokens freely (you can send them to a cold wallet, you can LP them, you can build additional infrastructure around them). Borrowers are able to constrain transfers to only those addresses that have received an onboarding credential, or completely prevent transfers except for those to/from the market contract.

In most cases, it is the most fundamental requirement that a lender can deposit their assets into the market only when they have valid credentials.
However, a borrower can not deploy a market which disables the token transferring function while requires lenders having valid credentials for deposit.

We can deep dive FixedTermLoanHooks#onTransfer() for detail:
onTransfer() can revert only when transferRequiresAccess is true and the lender has no valid credential (hasValidCredential is false)

917:      if (market.transferRequiresAccess.and(!hasValidCredential)) {
918:        revert NotApprovedLender();
919:      }

However, it a lender has no valid credential, they can not deposit their assets as long as the market requires lenders having any valid credentials for deposit.
https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L835-L837:

835:    if (market.depositRequiresAccess.and(!hasValidCredential)) {
836:      revert NotApprovedLender();
837:    }

If we take a look at AccessControlHooks.sol, we can find that it doesn't support disabling token transfer function at all.

Tools Used

Manual review

Recommended Mitigation Steps

isTransferDisabled should be introduced to config a market , which will be initialized in _onCreateMarket‎(), and used for token transfer control:

struct HookedMarket {
  bool isHooked;
+ bool isTransferDisabled;
  bool transferRequiresAccess;
  bool depositRequiresAccess;
  bool withdrawalRequiresAccess;
  uint128 minimumDeposit;
  uint32 fixedTermEndTime;
}

  function onTransfer(
    address /* caller */,
    address /* from */,
    address to,
    uint /* scaledAmount */,
    MarketState calldata /* state */,
    bytes calldata extraData
  ) external override {
    HookedMarket memory market = _hookedMarkets[msg.sender];

    if (!market.isHooked) revert NotHookedMarket();
+   if (market.isTransferDisabled) revert TransferDisabled();

    // If the recipient is a known lender, skip access control checks.
    if (!isKnownLenderOnMarket[to][msg.sender]) {
      LenderStatus memory toStatus = _lenderStatus[to];
      // Respect `isBlockedFromDeposits` only if the recipient is not a known lender
      if (toStatus.isBlockedFromDeposits) revert NotApprovedLender();

      // Attempt to validate the lender's access even if the market does not require
      // a credential for transfers, as the recipient may need to be updated to reflect
      // their new status as a known lender.
      (bool hasValidCredential, bool wasUpdated) = _tryValidateAccessInner(toStatus, to, extraData);

      // Revert if the recipient does not have a valid credential and the market requires one
      if (market.transferRequiresAccess.and(!hasValidCredential)) {
        revert NotApprovedLender();
      }

      _writeLenderStatus(toStatus, to, hasValidCredential, wasUpdated, true);
    }
  }

Assessed type

Access Control

[d1ll0n]

The intent here was just to remove a method by which the borrower could prevent lenders from becoming marked as known lenders (granted, they could still do that by removing all role providers). Acknowledging because it is valid and the rationale for this can be bypassed.

[3docSec]

I agree that the finding explains how tokens could be transferred in situations when they shouldn't. I however don't see an HM impact here, so L seems more appropriate in this case.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[laurenceday]

Fixed by wildcat-finance/v2-protocol@3f92bf6