Markets cannot be closed

[c4-submissions]

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

[c4-pre-sort]

minhquanym marked the issue as duplicate of #147

[c4-judge]

MarioPoneder changed the severity to 2 (Med Risk)

[c4-judge]

MarioPoneder marked the issue as partial-50

[c4-judge]

MarioPoneder changed the severity to 3 (High Risk)