howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L863-L879 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L779-L801

Vulnerability details

Impact

A user with expired credentials can bypass restrictions and receive tokens through the transfer function because credential validation is not enforced in the same way it is in the deposit hook. This creates a security loophole where a user, even though they are no longer eligible due to expired credentials, can still receive tokens. This inconsistency in validation allows for unauthorized access to token transfers, while the same user would be unable to deposit tokens with expired credentials.

Proof of Concept

Deposit Hook: The deposit hook correctly prevents a user with expired credentials from depositing, as seen in the following logic:

   (bool hasValidCredential, bool roleUpdated) = _tryValidateAccessInner(
      status,
      lender,
      hooksData
   );

   if (market.depositRequiresAccess.and(!hasValidCredential)) {
      revert NotApprovedLender();
   }

even if the user was previously known.

Transfer Hook: The transfer hook, however, does not consistently enforce credential validation for recipients. If the recipient is a previously known lender, they can receive tokens without checking whether their credentials are still valid:

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

Credential Expiration Loophole: A known lender with expired credentials should not be allowed to receive tokens, but because the credential check is skipped for "known" lenders, they can bypass the restrictions. This discrepancy allows users who are ineligible (due to expired credentials) to receive transfers, which should be prevented.

Tools Used

Manual code analysis and review.

Recommended Mitigation Steps

To resolve this issue, ensure that the transfer hook enforces the same credential validation rules as the deposit hook. A user with expired credentials should not be able to bypass access control just because they are previously known.

Proposed Solution:

Modify the onTransfer function to enforce credential validation for all users, even if they are previously known, similar to the deposit logic:

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

    // Retrieve the recipient's status
++    LenderStatus memory toStatus = _lenderStatus[to];

  if (!isKnownLenderOnMarket[to][msg.sender]) {

    // Check if the recipient is blocked from receiving deposits or transfers
    if (toStatus.isBlockedFromDeposits) revert NotApprovedLender();

--  // Attempt to validate the lender's access even if the market does not require
--      // a credential for transfers.
--      (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);
}

++    // Always check if the recipient has valid credentials, regardless of known status
++    (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);
}

This modification ensures that all users, regardless of whether they are previously known, will have their credentials validated during transfers. This prevents users with expired credentials from receiving tokens, thus maintaining consistency with the deposit restrictions.

Assessed type

Error

c4-judge commented 1 month ago

3docSec changed the severity to QA (Quality Assurance)