Missing isHooked check when calling onQueueWithdrawal hook function

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

Due to missing check of isHooked value in onQueueWithdrawal function, improper reverts could happen with withdrawal operations.

Proof of Concept

Hooks are utilized to oversee and intervene in market activities such as deposits, transfers, and withdrawals. Each hook instance maintains a state variable named isHooked for every market, ensuring it is properly registered and authorized to interact with market operations. This is clearly demonstrated in the implementation of the onDeposit and onTransfer hook functions.

  function onDeposit(
    address lender,
    uint scaledAmount,
    MarketState calldata state,
    bytes calldata hooksData
  ) external override {
    HookedMarket memory market = _hookedMarkets[msg.sender];
>>  if (!market.isHooked) revert NotHookedMarket();
    ...

  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();
    ...

However, the onQueueWithdrawal function lacks this check, potentially leading to mishandling of hook actions by an unauthorized hook instance. Both AccessControlHooks and FixedTermLoanHooks contracts have this issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the check in the onQueueWithdrawal function:

  function onQueueWithdrawal(
    address lender,
    uint32 /* expiry */,
    uint /* scaledAmount */,
    MarketState calldata /* state */,
    bytes calldata hooksData
  ) external override {
+   HookedMarket memory market = _hookedMarkets[msg.sender];
+   if (!market.isHooked) revert NotHookedMarket();

    LenderStatus memory status = _lenderStatus[lender];
    if (
      !isKnownLenderOnMarket[lender][msg.sender] && !_tryValidateAccess(status, lender, hooksData)
    ) {
      revert NotApprovedLender();
    }
  }

Assessed type

Invalid Validation

[d1ll0n]

The poking of a lender's account is not permissioned; anyone could do it through the transfer function. The only reason the other functions have the isHooked check is because they can modify more of the state than just bumping a user's credentials through provider queries: specifically, deposit and transfer can both change whether an account is recorded as being a known lender, so those functions must be gated. Other hooks may require this check for more than these functions, but for this template it's not needed.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by 3docSec

[3docSec]

See #83 (comment) on why this group has been marked as M.

This particular dupe however lacks any elaboration on how a legitimately "missing check" translates to "improper reverts could happen".

[c4-judge]

3docSec marked the issue as unsatisfactory:
Insufficient proof