The Wildcat Protocol - squeaky_cactus's results

Lines of code

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

Vulnerability details

Impact

The combination of modifiers for WildcatMarket.closeMarket and the implementation of WildcatMarketController creates a situation where closeMarket can never actually be successfully invoked.

The condition for closing the market is there are no unpaid withdrawal batches (the market may still have capital from lenders), with the result being the APR is set to zero.

If a borrower cannot have their market closed, that exit condition is still possible, yet additionally requires lender participation, time, interest accrual and a greater number of transactions to achieve.

Description

WildcatMarket,closeMarket uses the onlyController modifier from WildcatMarketBase, which will revert unless the msg.sender is the controller address. The controller address is set in the constructor, being the WildcatMarketController that deploys the market. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142

  function closeMarket() external onlyController nonReentrant {

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L136-L139

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

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L119

    MarketParameters memory parameters = IWildcatMarketController(msg.sender).getMarketParameters();
    ...
    controller = parameters.controller;
  }

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L234-L253

    function getMarketParameters() external view returns (MarketParameters memory parameters) {
    ...
    parameters.controller = address(this);
  }

As the WildcatMarketController contains zero methods that have a function call to closeMarket, and it can only be invoked by the controller successfully (i.e. without reverting), it can never be called. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L1-L516

Proof of Concept

WildcatMarket.t.sol contains closeMarket tests that are using a modifier that pranks the address to be the controller, but they demonstrate that only the controller address can successfully invoke closeMarket. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/test/market/WildcatMarket.t.sol#L198-L246

Tools Used

Manual review

Recommended Mitigation Steps

The appropriate level of the change will depend on the desired behaviour for a closed market in the future, specifically whether a closed market is ever allowed to be re-opened, or another market from the same controller and salt (asset, namePrefix, symbolPrefix).

Suggestions: 1) Swap the modifier Quickest choice, replace onlyController with onlyBorrower, accepting it means the borrower can control closing down the market and as state.isClosed is only set a false on construction then that market will be permanently closed.

2) Add a close market function to the controller

3) Enable market reuse In addition to providing the ability to close the market, add a function to open the market, allowing reuse of a previously deployed contract.

Assessed type

Access Control