Borrowers can frontrun ArchController Owner to deploy markets without protocol fees

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketControllerFactory.sol#L282-L301 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketControllerFactory.sol#L195-L217

Vulnerability details

Description

When a new factory is deployed and registered to the ArchController contract it can immediately be used to deploy a controller and a market by a registered borrower. The factory deployer may forget to call setProtocolFeeConfiguration on the factory to set the protocol fees: feeRecipient, protocolFeeBips, originationFeeAsset, and `originationFeeAmounts or the transaction may be frontran by a registered borrower to deploy a controller he can use in creating markets without protocol fees.

Impact

This allows a borrower to not pay any protocol fees to the Wildcat protocol team.

Proof of Concept

When a factory is deployed, the protocol fees aren't set in the constructor which allows them to have default values of zero and zero address respectively.

constructor(
    address _archController,
    address _sentinel,
    MarketParameterConstraints memory constraints
  ) {
    archController = IWildcatArchController(_archController);
    sentinel = _sentinel;
    ...
    MinimumDelinquencyGracePeriod = constraints.minimumDelinquencyGracePeriod;
    MaximumDelinquencyGracePeriod = constraints.maximumDelinquencyGracePeriod;
    MinimumReserveRatioBips = constraints.minimumReserveRatioBips;
    MaximumReserveRatioBips = constraints.maximumReserveRatioBips;
    MinimumDelinquencyFeeBips = constraints.minimumDelinquencyFeeBips;
    MaximumDelinquencyFeeBips = constraints.maximumDelinquencyFeeBips;
    MinimumWithdrawalBatchDuration = constraints.minimumWithdrawalBatchDuration;
    MaximumWithdrawalBatchDuration = constraints.maximumWithdrawalBatchDuration;
    MinimumAnnualInterestBips = constraints.minimumAnnualInterestBips;
    MaximumAnnualInterestBips = constraints.maximumAnnualInterestBips;

    ...
  }

They have to be set by calling setProtocolFeeConfiguration.

     function setProtocolFeeConfiguration(
    address feeRecipient,
    address originationFeeAsset,
    uint80 originationFeeAmount,
    uint16 protocolFeeBips
  ) external onlyArchControllerOwner {
    ...
    _protocolFeeConfiguration = ProtocolFeeConfiguration({
      feeRecipient: feeRecipient,
      originationFeeAsset: originationFeeAsset,
      originationFeeAmount: originationFeeAmount,
      protocolFeeBips: protocolFeeBips
    });
  }

If the factory is registered in the archController before setProtocolFeeConfiguration is called, a borrower can call deployController to deploy a controller that allows him to create markets without protocol fees. He can frontrun the setProtocolFeeConfiguration or simply create the controller if the deployer forgets to call the function.

Here's a simple Proof Of Concept:

  function test_deploy() external {
    archController = new WildcatArchController();
    sanctionsSentinel = new MockSanctionsSentinel(address(archController));
    controllerFactory = new MockControllerFactory(
      address(archController),
      address(sanctionsSentinel)
    );

    archController.registerBorrower(borrower);
    archController.registerControllerFactory(address(controllerFactory));

    // frontrun deployer
    startPrank(borrower);
    (address _controller, address _market) = controllerFactory.deployControllerAndMarket(
      'Wildcat ',
      'WC',
      address(asset),
      1,
      1,
      DefaultDelinquencyFee,
      1,
      DefaultReserveRatio,
      DefaultGracePeriod
    );

    uint256 protocolFeeBips = WildcatMarket(_market).protocolFeeBips();
    address feeRecipient = WildcatMarket(_market).feeRecipient();
    assertEq(protocolFeeBips, 0);
    assertEq(uint160(feeRecipient), 0);
  }

Test Result

Running 1 test for test/market/WildcatMarket.t.sol:WildcatMarketTest
[PASS] test_deploy() (gas: 16110461)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.72ms

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add feeRecipient, protocolFeeBips, originationFeeAsset, and originationFeeAmount to the constructor parameters. They can also be set to default values in the constructor if they necessarily have to be called after deploying the factory.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as duplicate of #552

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b

[nonseodion]

Severity Level

In Codearena audit contests it's general knowledge that findings arising from admin mistakes are considered invalid. E.g supplying wrong arguments or losing private keys. But this finding although related to admin functions does not rely on mistakes done by the admin or governance.

The admin/governance just has to register the factory and set the fee like he would do normally. This does not stop the attacker from frontrunning the transaction to set fees with a transaction to create a controller and a market.

Considering it's the protocol that loses funds and the issue can clearly be mitigated even after deployment, I believe this should be Medium. It can be mitigated by using Flashbots to execute the two transactions or doing the two transactions directly from a single smart contract call.

Quality of finding

The issue clearly states the problem with a valid POC. It also list simple mitigation steps. If for whatever reason it remains a QA issue, I believe it should have a Grade-A. If the judge discerns that it doesn't fit this criteria, I'd appreciate it if he can give clear reasons why.