Inconsistent Access Control Allows Known Lenders to Bypass Deposit Restrictions via Transfers

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Proof of Concept

There is a inconsistency in how the isBlockedFromDeposits status is checked between deposit and transfer functions in the Wildcat Market contracts. This inconsistency allows known lenders who are blocked from deposits to bypass this restriction by receiving transfers, effectively allowing them to increase their position when they shouldn't be able to.

In the deposit function (via onDeposit in AccessControlHooks.sol), the blocked status is always checked:

function onDeposit(address lender, uint scaledAmount, MarketState calldata state, bytes calldata hooksData) external override {
  // ...
  LenderStatus memory status = _lenderStatus[lender];
  if (status.isBlockedFromDeposits) revert NotApprovedLender();
  // ...
}

However, in the transfer function (via onTransfer in AccessControlHooks.sol), the blocked status is only checked for non-known lenders:

function onTransfer(address /* caller */, address /* from */, address to, uint /* scaledAmount */, MarketState calldata /* state */, bytes calldata extraData) external override {
  // ...
  if (!isKnownLenderOnMarket[to][msg.sender]) {
    LenderStatus memory toStatus = _lenderStatus[to];
    if (toStatus.isBlockedFromDeposits) revert NotApprovedLender();
    // ...
  }
  // ...
}

Exploit scenario:

1. Alice is a known lender and has been blocked from deposits (isBlockedFromDeposits is true).
2. Alice can't deposit directly due to the blocked status check.
3. However, Bob can transfer tokens to Alice.
4. The transfer function doesn't check Alice's blocked status because she's a known lender.
5. Alice effectively increases her position in the market, bypassing the intended restriction.

This inconsistency undermines the purpose of the access control system and could potentially be exploited to circumvent market restrictions.

Recommended Mitigation Steps

Apply consistent access control in both deposit and transfer functions.

Assessed type

Access Control

[d1ll0n]

This is the intended behavior (and this scenario is precisely why the isKnownLender status exists). If an account is a known lender, they should always be able to purchase market tokens from other accounts. They can still be blocked from depositing, as borrowers should never be forced to accept funds from third parties they do not wish to receive money from, but if they have previously passed the borrower's KYC requirements they should always have the ability to receive tokens from other lenders.

The scenario in which this is relevant is when a market becomes delinquent and there exist some lenders who have sufficient assets in the market to make pursuing legal action worthwhile, while other lenders have an amount that would only make sense to write off as a loss. Allowing known lenders to transfer their tokens to each other means that a larger depositor has the ability to buy up the other lenders' claims on the debt at a discount and pursue a lawsuit that wouldn't make sense as a class action or a hundred different claims for $5k.

[3docSec]

Marking as intended behavior

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid