Minimum deposit requirement can be bypassed

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L850-L880
https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L812-L825
https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L769-L804

Vulnerability details

Impact

Users can bypass the minimum deposit requirement to create small positions
Small positions may be used as an attack path

Vulnerability Details

The minimum deposit for market is not enforced in the withdrawal process or the transfer process so lender can bypass the requirement.

Say the minimum deposit is 100_000; a user can:

• Deposit 100_000 to pass the check
• Withdraw as much as they want, as soon as the withdraw duration will allow them, to leave themselves below the minimum deposit level
  or
• Transfer as much as they want to another user to leave themselves below the minimum deposit level

Both hooks templates enforce the minimum deposit (if set) in the onDeposit hook as:

  function onDeposit(
    address lender,
    uint scaledAmount,
    MarketState calldata state,
    bytes calldata hooksData
  ) external override {

    // SOME CODE

    // Check that the deposit amount is at or above the market's minimum
    uint normalizedAmount = scaledAmount.rayMul(state.scaleFactor);
>>> if (market.minimumDeposit > normalizedAmount) {
      revert DepositBelowMinimum();
    }

    // SOME CODE
  }

However there is no check when a user executes a transfer of Scaled Tokens or withdraws from the protocol to go below the minimum deposit level.

See onTransfer()

See onQueueWithdrawal()

The market prefers not to allow small positions because they may be too small to be worth the gas cost of conducting operations on.
Furthermore, an attacker can add a large amount of small positions in order to exploit the block gas limit in a function which is gas heavy using operations like looping, such as closeMarket() sich that they may be able to cause a DOS.

POC

Add the test function below to WildcatMarket.t.sol and run:

User bypasses minimum deposit

  function test_POC_3() external {
    parameters.minimumDeposit = 100_000;
    parameters.withdrawalBatchDuration = 0;
    setUpContracts(false);
    startPrank(alice);
    asset.mint(alice, 1e18);
    asset.approve(address(market), 1e18);
    vm.expectRevert(AccessControlHooks.DepositBelowMinimum.selector);
    market.deposit(50_000);

    market.deposit(100_000);
    uint32 expiry = market.queueWithdrawal(50_000);

    fastForward(parameters.withdrawalBatchDuration + 1);
    market.executeWithdrawal(alice, expiry);

  }

Tools Used

Manual Review
Foundry Testing

Recommendations

Enforce minimum deposit with a check in the onTransfer & onQueueWithdrawal hooks in both hooks templates so that user cannot go below it

Assessed type

Invalid Validation

[laurenceday]

If this recommendation was taken at face value and implemented, then once a lender deposited assets they'd never be able to get anything out again (because any balance below the minimum deposit would be verboten).

We're not interested in implementing this, and don't consider it a valid finding. Minimum deposit amounts are a QOL improvement for borrowers such as market makers who don't want lenders to park 1,000 USDC into a market when they need to find five multisig signers to execute the transaction to borrow it.

More pragmatically, if the minimum deposit limit is 100,000 USDC and someone wants to subsequently withdraw 99,000 of that straight away, their funds are still going to be locked up for the duration of a withdrawal cycle (in which case no one can access it for the interim). As a matter of common sense, lenders just aren't going to do that (not least because they're probably going to have their KYC held by the borrower in some capacity so could catch blowback). If someone does this anyway, the borrower is well within their rights (and has the ability) to just revoke their deposit credential.

The proposed closeMarket attack path is neither realistic or effective - a borrower could simply call repayAndProcessUnpaidWithdrawalBatches with a sufficiently small maxBatches parameter enough times to clear the backlog and then terminate as expected.

[3docSec]

The impact here is quite unclear, apart from the gas griefing vector that is mitigated as per sponsor's comment

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)