Borrowers could still deploy market while the template is diabled

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/HooksFactory.sol#L506

Vulnerability details

Impact

Borrowers could still deploy market while the template is diabled, since HooksFactory.sol:deployMarket() doesn't check if HooksTemplate is existed and enbled.

Proof of Concept

ArchControllerOwner could disable hookstemplate with disableHooksTemplate(address hooksTemplate). While disabled, deployMarket to this template should get reverted.

function deployMarket(
    DeployMarketInputs calldata parameters,
    bytes calldata hooksData,
    bytes32 salt,
    address originationFeeAsset,
    uint256 originationFeeAmount
  ) external override nonReentrant returns (address market) {
    if (!IWildcatArchController(_archController).isRegisteredBorrower(msg.sender)) {
      revert NotApprovedBorrower();
    }
    address hooksInstance = parameters.hooks.hooksAddress();
    address hooksTemplate = getHooksTemplateForInstance[hooksInstance];
    if (hooksTemplate == address(0)) {
      revert HooksInstanceNotFound();
    }
    HooksTemplate memory templateDetails = _templateDetails[hooksTemplate];
    market = _deployMarket(
      parameters,
      hooksData,
      hooksTemplate,
      templateDetails,
      salt,
      originationFeeAsset,
      originationFeeAmount
    );
  }

Tools Used

manually reviewed

Recommended Mitigation Steps

add the following checks before _deployMarket():

    if (!templateDetails.exists) {
      revert HooksTemplateNotFound();
    }
    if (!templateDetails.enabled) {
      revert HooksTemplateNotAvailable();
    }

Assessed type

Access Control

[laurenceday]

Please see: https://github.com/code-423n4/2024-08-wildcat-findings/issues/7#issuecomment-2360703992

A template being removed is for deprecation of old templates if we need to update them to add some new functionality. We don't want that to impact a borrower's ability to deploy new markets using their existing hooks instances though, as their hooks instance has their whole access control configuration.

The function the warden is looking at relates to when there is a specific hooks instance already in existence, in which case we don't want to prevent deployments for precisely the above reason.

In the event that a new hook instance is to be deployed, the function that is instead called is deployMarketAndHooks, and the exists check is the very first thing that takes place after the template struct is loaded.

[3docSec]

Intended behavior

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid