Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AccessControlHooks onQueueWithdrawal() does not check if market is hooked which could lead to unexpected errors such as temporary DoS #11

Open
c4-bot-10 opened this issue Sep 18, 2024 · 7 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working grade-b M-08 primary issue Highest quality submission among a set of duplicates 🤖_00_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

The onQueueWithdrawal() function does not check if the caller is a hooked market, meaning anyone can call the function and attempt to verify credentials on a lender. This results in calls to registered pull providers with arbitrary hookData, which could lead to potential issues such as abuse of credentials that are valid for a short term, e.g. 1 block.

Proof of Concept

The onQueueWithdrawal() function does not check if the msg.sender is a hooked market, which is standart in virtually all other hooks:

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

  /**
   * @dev Called when a lender attempts to queue a withdrawal.
   *      Passes the check if the lender has previously deposited or received
   *      market tokens while having the ability to deposit, or currently has a
   *      valid credential from an approved role provider.
   */
  function onQueueWithdrawal(
    address lender,
    uint32 /* expiry */,
    uint /* scaledAmount */,
    MarketState calldata /* state */,
    bytes calldata hooksData
  ) external override {
    LenderStatus memory status = _lenderStatus[lender];
    if (
      !isKnownLenderOnMarket[lender][msg.sender] && !_tryValidateAccess(status, lender, hooksData)
    ) {
      revert NotApprovedLender();
    }
  }

If the caller is not a hooked market, the statement !isKnownLenderOnMarket[lender][msg.sender], will return true, because the lender will be unknown. As a result the _tryValidateAccess() function will be executed for any lender and any hooksData passed. The call to _tryValidateAccess() will forward the call to _tryValidateAccessInner(). Choosing a lender of arbitrary address, say address(1) will cause the function to attempt to retrieve the credential via the call to _handleHooksData(), since the lender will have no previous provider or credentials.

As a result, the _handleHooksData function will forward the call to the encoded provider in the hooksData and will forward the extra hooks data as well, say merkle proof, or any arbitrary malicios data.

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

  function _handleHooksData(
    LenderStatus memory status,
    address accountAddress,
    bytes calldata hooksData
  ) internal returns (bool validCredential) {
    // Check if the hooks data only contains a provider address
    if (hooksData.length == 20) {
      // If the data contains only an address, attempt to query a credential from that provider
      // if it exists and is a pull provider.
      address providerAddress = _readAddress(hooksData);
      RoleProvider provider = _roleProviders[providerAddress];
      if (!provider.isNull() && provider.isPullProvider()) {
        return _tryGetCredential(status, provider, accountAddress);
      }
    } else if (hooksData.length > 20) {
      // If the data contains both an address and additional bytes, attempt to
      // validate a credential from that provider
      return _tryValidateCredential(status, accountAddress, hooksData);
    }
  }

The function call will be executed in tryValidateCredential(), where the extra hookData will be forwarded. As described in the function comments, it will execute a call to provider.(address account, bytes calldata data).

This means that anyone can call the function and pass arbitrary calldata. This can lead to serios vulnerabilities as the calldata is passed to the provider.

Consider the following scenario:

  • The pull provider is implemented to provide a short-term(say one block) approval timestamp.
  • A user of the protocol provides a merkle-proof which would grant the one-time approval to withdraw in a transaction.
  • A malicios miner frontruns the transaction submitting the same proof, but does not include the honest transaction in the mined block. Instead it is left for the next block.
  • In the next block, the credential is no longer valid and as a result the honest user has their transaction revert.
  • The miner does this continuosly essentially DoSing the entire market that uses this provider until it is removed and a new one added.

By following this scenario, a malicios user can essentially DoS a specific type pull provider.

Depending on implemenation of the pull provider, this can lead to other issues, as the malicios user can supply any arbitrary hookData in the function call.

Recommended Mitigation Steps

Require the caller to be a registered hooked market, same as onQueueWithdrawal() in FixedTermloanHooks

Assessed type

DoS

@c4-bot-10 c4-bot-10 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 18, 2024
c4-bot-10 added a commit that referenced this issue Sep 18, 2024
@c4-bot-13 c4-bot-13 added the 🤖_00_group AI based duplicate group recommendation label Sep 18, 2024
@howlbot-integration howlbot-integration bot added sufficient quality report This report is of sufficient quality duplicate-26 labels Sep 20, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 4, 2024

3docSec changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 4, 2024

3docSec marked the issue as grade-b

@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by 3docSec

@c4-judge c4-judge reopened this Oct 10, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Oct 10, 2024
@c4-judge
Copy link
Contributor

3docSec marked the issue as satisfactory

@c4-judge
Copy link
Contributor

3docSec marked the issue as selected for report

@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Oct 10, 2024
@3docSec
Copy link

3docSec commented Oct 10, 2024

See #83 (comment)

@laurenceday
Copy link

laurenceday commented Oct 23, 2024

We don't consider this a real issue, in that we’ve always wanted it to be possible for anyone to call the validate function to poke a credential update. This finding assumes that you have the signature someone else would be using as a credential and generally relies on a specific implementation of the provider that doesn’t actually exist, so there's no need to check isHooked.

It's been upgraded to a Medium, and we're not going to argue with this at this stage. As such, we're acknowledging rather than confirming or disputing simply to put a cap on the report.

@laurenceday laurenceday added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working grade-b M-08 primary issue Highest quality submission among a set of duplicates 🤖_00_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants