Lenders can avoid sanction restrictions in FixedTermLoanHooks by transferring to another account

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/FixedTermLoanHooks.sol#L848-L868
https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L812-L825

Vulnerability details

Impact

Lenders can evade sanctioning restrictions by sending their tokens to another of their addresses and continue earning interest in FixedTermLoanHooks.
Hooks templaets have inconsistent security levels.

Vulnerability Details

Although the documentation says that the two hooks templates AccessControlHooks & FixedTermLoanHooks are the "exact same" and lists the ways in which they are not; see [here](The https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/docs/hooks/templates/Fixed%20Term%20Loan%20Hooks.md?plain=1#L5C1-L5C55), this is not the case with respect to withdrawal security as we will see below.

The isKnownLender flag is set when a lender deposits and is never removed.
It serves two purposes:
1. Prevents the borrower from blocking withdrawals from the market by removing genuine lender's credentials
2. Prevents unknown accounts from being able to withdraw tokens from the market

In AccessControlHooks if the onQueueWithdrawal hook is enabled a withdrawing user must be a knownLender or have an active credential.

        LenderStatus memory status = _lenderStatus[lender];

        if (
          !isKnownLenderOnMarket[lender][msg.sender] && !_tryValidateAccess(status, lender, hooksData)
        ) {
          revert NotApprovedLender();
        }

In FixedTermLoanHooks however; isKnownLender is only checked if both the hook is enabled AND market.withdrawalRequiresAccess is active.
If the hook is active but the market.withdrawalRequiresAccess = FALSE users with no credentials are allowed to withdraw.

This allows a user who suspects they may be sanctioned in the future to transfer all of their scaledTokens to another account of theirs and withdraw any time without restriction; avoiding having their funds sent to an escrow.

        LenderStatus memory status = _lenderStatus[lender];

 >>>    if (market.withdrawalRequiresAccess) {
          if (
            !isKnownLenderOnMarket[lender][msg.sender] && !_tryValidateAccess(status, lender, hooksData)
          ) {
            revert NotApprovedLender();
          }
        }

POC

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

Users with no credentials can withdraw

  function test_POC_1() external {

    address bob = address(10);
    address market = address(1);

    DeployMarketInputs memory inputs;
    MarketState memory state;

    // Don't activate onQueueWithdrawal hook
    inputs.hooks = EmptyHooksConfig.setHooksAddress(address(hooks));
    hooks.onCreateMarket(
      address(this), 
      market,    
      inputs,        
      abi.encode(block.timestamp + 365 days, 1e18)
    );

    // Withdrawer does not have the isKnownLender credential
    bool isKnownLender = hooks.isKnownLenderOnMarket(bob, market);
    assertEq(isKnownLender, false);

    // Function will not revert though Bob is not a known lender
    vm.prank(market);
    vm.warp(block.timestamp + 366 days);
    hooks.onQueueWithdrawal(bob, 0, 1, state, '');
  }

Tools Used

Manual Review
Foundry Testing

Recommendations

Only known lenders should be able to withdraw if the hook is enabled:

        LenderStatus memory status = _lenderStatus[lender];

        if (market.withdrawalRequiresAccess) {
            // Requires access, must satisfy either condition
            if (
                !isKnownLenderOnMarket[lender][msg.sender] && 
                !_tryValidateAccess(status, lender, hooksData)
            ) {
                revert NotApprovedLender();
            }
        } else {
            // Doesn't require access, must be a known lender
            if (!isKnownLenderOnMarket[lender][msg.sender]) {
                revert NotApprovedLender();
            }
        }

Assessed type

Access Control

[laurenceday]

Transfers from sanctioned accounts are blocked in the _getAccount function within the WildcatMarketBase contract.

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketBase.sol#L244-L247

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketToken.sol#L80-L86

Invalid finding.

[c4-judge]

3docSec marked the issue as unsatisfactory:
Invalid

[3docSec]

Hi @laurenceday would you mind having another look at this finding?

The point is frontrunning sanctions by transferring to another "unknown" address and keep interacting with the protocol from there, so the sanctions check at transfer time won't save the day if I get this right

[d1ll0n]

That'd only be possible if they knew the sanctions would be recorded soon but haven't yet, which IMO is not really something we can do anything about, or likely enough to be worth doing anything about

[3docSec]

Okay so the finding is valid.
Wrt severity, I think L is appropriate, because there is no impact on functionality or funds. Blockbuster tokens like USDC have the same issue - before being blocked one can give away tokens to some other address and keep using them. It is reasonable to expect the new address will be sanctioned too.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b