howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L786-L791 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L864-L881 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L733-L742

Vulnerability details

The issue stems from the difference in logic between the onDeposit and onTransfer functions in AccessControlHooks.sol, particularly regarding the minimum deposit check. Let's analyze this step by step:

Behavior of the onDeposit function: In the onDeposit function, there's an explicit check for the minimum deposit amount:

   // 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();
   }

This ensures that direct deposits must meet the minimum deposit requirement.

Behavior of the onTransfer function: In contrast, the onTransfer function doesn't have a similar minimum amount check. It primarily focuses on validating the permissions of the recipient (to address):
```
  // If the recipient is a known lender, skip access control checks.
  if (!isKnownLenderOnMarket[to][msg.sender]) {
    // ... check logic ...
  }
```

Impact

Attackers might gain known lender status by receiving transfers of extremely small amounts, without meeting the normal minimum deposit requirements.
This could lead to a large number of accounts in the system holding tiny amounts but having known lender privileges.
It may affect the system's economic model and risk assessment.

Proof of Concept

Attackers might exploit this difference to bypass the minimum deposit limit while still gaining known lender status. The attack steps could be as follows:

a. The attacker first obtains a valid credential through legitimate means (possibly a small deposit or other method).

b. Then, the attacker uses this valid credential to receive a very small amount (potentially far below the minimum deposit requirement) through the onTransfer function.

c. Since onTransfer doesn't have a minimum amount check, as long as the credential is valid, this transfer would succeed.

d. In the _writeLenderStatus function, if the conditions are met (valid credential, not previously a known lender, canSetKnownLender is true), the account would be marked as a known lender:

  ```solidity
      // Mark account as a known lender if they have a valid credential, are not
      // already known, and the function counts as a deposit.
      if (
        canSetKnownLender.and(hasValidCredential).and(
          !isKnownLenderOnMarket[accountAddress][msg.sender]
        )
      ) {
        isKnownLenderOnMarket[accountAddress][msg.sender] = true;
        emit AccountMadeFirstDeposit(accountAddress);
      }
  ```

Recommended Mitigation Steps

Add a minimum amount check in the onTransfer function as well.
Introduce a cumulative balance check, only granting known lender status when an account's total balance reaches a certain threshold.
Implement a "maturity period" for known lender status, requiring accounts to maintain a minimum balance for a period of time.
Consider separating the concepts of "known lender" and "valid depositor," applying different restrictions for different operations.

Assessed type

Access Control

laurenceday commented 2 months ago

This one is hard to judge. It's theoretically valid. However, a minimum amount check or anything of the sort would either be easily attackable or impose a really strict bar.

With that said, per the C4 guidelines:

Availability is not affected, the basic functions of the protocol aren't impacted, but beyond this it's difficult to differentiate this from side features. We're happy to leave this one to the judge, but would gratefully call this one a QA: Medium seems neither here nor there.

So, we're disputing (because acknowledging would leave this as is), but it's with a VERY light hand.

3docSec commented 1 month ago

I agree with the sponsor here. The report finds a way around the minimumDeposit check, but fails to elaborate on what impact a lender can cause after getting the "known lender" flag through following this path -> Low

c4-judge commented 1 month ago

3docSec changed the severity to QA (Quality Assurance)