The Wildcat Protocol - LokiThe5th's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142-L162

Vulnerability details

Impact

The protocol is designed such that the entirety of the underlying assets in a market always accrue interest. This design choice means that to halt the accrual of interest once all the repayments are done, the borrower should call WildcatMarket::closeMarket(). This halts the interest accrual by setting the reserve ratio and the interest to 0.

WildcatMarket exposes this functionality through a closeMarket() function and the call to this function is protected by an onlyController modifier. This means that only the MarketController that deployed that particular market should be able to close it. This is appropriate, as the intention seems to be that the borrower has the power to close the market.

The issue is that the WildcatMarketController does not expose a function call, directly or indirectly, that calls closeMarket() for a particular market. The consequence of this is that no market can be closed at any time. The borrower will then, according to the smart contracts, be on the hook for the interest accumulated on the underlying in perpetuity.

Due to this continuous accrual of liability for the borrower, and the breaking of critical functionality, this has been evaluated to high.

Please note: The reviewer is aware that the relationship between the lender and the borrower is defined off-chain through a legal agreement and that the protocol is effectively a vehicle for that legal instrument. An argument can then be made that the borrower would not be liable for excess interest as there is a governing legal agreement (i.e. terms were agreed upon off-chain). But within the confines of the review, the code is the only agreement between parties, and at the moment the agreement is for the borrower to accrue excess liability and then to pay that liability, or to become delinquent, or count on the good grace of the lender to withdraw the funds at the appropriate time.

Proof of Concept

The issue is in this block of code:

  function closeMarket() external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0;
    if (_withdrawalData.unpaidBatches.length() > 0) {
      revert CloseMarketWithUnpaidWithdrawals();
    }
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
      // Transfer excess assets to borrower
      asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
    _writeState(state);
    emit MarketClosed(block.timestamp);
  }

We can see that the call is protected by an onlyController modifier, which can be found in the WildcatMarketBase contract:

  modifier onlyController() {
    if (msg.sender != controller) revert NotController();
    _;
  }

Inspecting the WildcatMarketController shows us that there is no call to closeMarket() possible.

The consequence is that the market cannot be closed, and because of that the interest will continue to accumulate.

Tools Used

Manual review. Foundry.

Recommended Mitigation Steps

This is likely a simple oversight. Add a function into the WildcatMarketController:

  function closeMarket(address targetMarket) external onlyBorrower {
    if (!_controlledMarkets.contains(targetMarket)) revert NoMarket();

    WildcatMarket(market).closeMarket(targetMarket);
    emit MarketClosed(targetMarket);
  }

This will make sure the borrower can close the market when appropriate. Please note that the custom error and event in the code block would also be needed to be added.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/LibStoredInitCode.sol#L106-L118 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L354

Vulnerability details

Impact

The contract WildcatMarketBase contains multiple validity checks in its constructor to ensure all new markets adhere to expected parameters.

To deploy new markets the contract makes use of the LibStoredInitCode library. The library uses a create2 call to deploy new contracts.

The issue is that the LibStoredInitCode library does not check the return value of the create2 operation. If deployment fails in a create2 call then the returned value would be 0 (cast as an address). Because there are validation checks in the constructor of the deployed contract that are not present in the WildcatMarketController::deployMarket(), it is possible for the constructor execution to revert but the deployMarket call to succeed.

Thus, the success of the deployment is not checked in either the deployMarket function, or in the LibStoredInitCode::create2WithStoredInitCode. This leads to a situation where the expected market address (derived here) will be added to the WildcatArchController and the WildcatMarketController.

Because this silent failure to deploy a market is dependent on external requirements (invalid inputs) and because it leads to a loss for the deployer (the origination fee for the market is still paid), this has been evaluated to medium.

This is plausible, considering the QA-level issue that there is a 32 byte limit on asset name or symbol but which is not mentioned in the documentation. Deployments of such tokens are doomed to always fail and because of this issue lead to the loss of the market origination fee by the borrower.

Proof of Concept

Modify BaseMarketTest.t.sol line 32 to:

parameters.asset = address(asset = new MockERC20('Arbitrum Coinbase Wrapped Staked Ether', 'TKN', 18));

Run test with forge test --match-test test_totalAssets -vvvvv

The log output will be:

  [5201] WildcatMarketBaseTest::test_totalAssets() 
    ├─ [0] 0x83A6ce873F6C4157D74FD3D4ec00aFde147A966F::totalAssets() [staticcall]
    │   └─ ← ()
    └─ ← "EvmError: Revert" 

This is our first clue that the target contract has not been deployed.

We can test this directly by adding the below test into the WildcatMarketBase.t.sol file, while keeping the above modification to the MockERC20's name. The test can be run with forge test --match-test test_create2_POC -vvvv.

  function test_create2_POC() external {
    uint256 codeSize;
    address marketAddress = address(market);

    assembly {
      codeSize := extcodesize(marketAddress)
    }

    require(controller.isControlledMarket(marketAddress), "Market was deployed");

    require(codeSize == 0, "POC failed");
  }

The console ouput is:

Running 1 test for test/market/WildcatMarketBase.t.sol:WildcatMarketBaseTest
[PASS] test_create2_POC() (gas: 12768)
Traces:
  [12768] WildcatMarketBaseTest::test_create2_POC() 
    ├─ [2713] MockController::isControlledMarket(0x5AD3dd3d5300133d245C678f9134D4a9107fa94B) [staticcall]
    │   └─ ← true
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.19ms

The test proves that the market deployment failed silently: the code at the expected address is 0, deployment failed, but was not reverted. In addition, the expected address was still added to the WildcatMarketController.

Tools Used

Manual review. Foundry.

Recommended Mitigation Steps

Add a check to the LibStoredInitCode library that reverts if the deployment address is 0.

  function create2WithStoredInitCode(
    address initCodeStorage,
    bytes32 salt,
    uint256 value
  ) internal returns (address deployment) {
    assembly {
      let initCodePointer := mload(0x40)
      let initCodeSize := sub(extcodesize(initCodeStorage), 1)
      extcodecopy(initCodeStorage, initCodePointer, 1, initCodeSize)
      deployment := create2(value, initCodePointer, initCodeSize, salt)
    }
    
    if (deployment == address(0)) revert DeployFailed();
  }

Assessed type

Other