Lender's tokens (if he in chainalysis sanctions list) transfers to wrong address

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L99 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L166-L170 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L172-L176

Vulnerability details

Impact

Due to the incorrect order of arguments in createEscrow function in WildcatSanctionsSentinel contract, lender's tokens/assets is sent directly to the borrower. The variable account is assigned the borrower's address, while the variable borrower is assigned the creditor's address.

Example: The lender gets added to the chainalysis sanctions list, and someone calls the nukeFromOrbit() function. This function calls internal function _blockAccount(state, lendersAddress).

  function _blockAccount(MarketState memory state, address accountAddress) internal {
      ...
      if (scaledBalance > 0) {
        account.scaledBalance = 0;
        // @audit-issue Wrong arguments order.
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
          accountAddress,    // <---- correct variable should be borrower
          borrower,          // <---- correct variable should be accountAddress(lender)
          address(this)
        );
        emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
        _accounts[escrow].scaledBalance += scaledBalance;
      ...
    }
  }

// createEscrow function
function createEscrow(
    address borrower,  <--- account's address was passed instead borrower's
    address account,   <--- borrower's address was passed instead account's
    address asset
  ) public override returns (address escrowContract) {
     ....
    tmpEscrowParams = TmpEscrowParams(borrower, account, asset);
    new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }();

// constructor of WildcatSanctionsEscrow contract
 constructor() {
    sentinel = msg.sender;
    (borrower, account, asset) = WildcatSanctionsSentinel(sentinel).tmpEscrowParams();
 }

After the escrow contract is created, anyone can call the WildcatSanctionsEscrow.releaseEscrow() function. This function call internal function canReleaseEscrow() and check, that the return value of this function is always true. Function canReleaseEscrow() sends borrower and account addresses to isSanctioned function. The problem is that the values in these variables are swapped with each other.

 function canReleaseEscrow() public view override returns (bool) {
        // send lender's address as first argument!
        // send borrower's address as second argument!
    return !WildcatSanctionsSentinel(sentinel).isSanctioned(borrower, account);
  }

WildcatSanctionsSentinel.sol

  function isSanctioned(address borrower, address account) public view override returns (bool) {
    return
      !sanctionOverrides[borrower][account] &&
      IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account);
  }

This function will return false, because in reality, the check will be performed with incorrect addresses (because they are swapped with each other).

sanctionOverrides[borrower][account] -> sanctionOverrides[LENDER][BORROWER] -> False
IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(BORROWER) -> False
False && False -> False

Look at releaseEscrow function

 function releaseEscrow() public override {

    if (!canReleaseEscrow()) revert CanNotReleaseEscrow(); <------ canReleaseEscrow return always true

    uint256 amount = balance();

    IERC20(asset).transfer(account, amount); <---- remember, in account variable stored borrower address

    emit EscrowReleased(account, asset, amount);
  }

So tokens will be transfered to borrower.

Proof of Concept

Create file transferFromEscrowToWrongAccount.sol in test/market/

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.20;
import '../BaseMarketTest.sol';
import { WildcatSanctionsEscrow, IWildcatSanctionsEscrow } from 'src/WildcatSanctionsEscrow.sol';

contract WildcatMarketConfigTest is BaseMarketTest {
    event EscrowReleased(address indexed account, address indexed asset, uint256 amount);

  function test_transferFromEscrowToWrongAccount() public {
    _deposit(alice, 1e18);
    sanctionsSentinel.sanction(alice);
    address escrow = sanctionsSentinel.getEscrowAddress(alice, borrower, address(market));
    market.nukeFromOrbit(alice);
    vm.expectEmit(address(escrow));
    // Token transfers to borrower, not to Alice
    emit EscrowReleased(borrower, address(market), 1e18);
    IWildcatSanctionsEscrow(escrow).releaseEscrow();
  }

}

Tools Used

Manual review

Recommended Mitigation Steps

Change order or function arguments in WildcatSanctionsSentinel.createEscrow()

function createEscrow(
-    address borrower,
-    address account,
+    address account,
+    address borrower
    address asset
  ) public override returns (address escrowContract) {
    if (!IWildcatArchController(archController).isRegisteredMarket(msg.sender)) {
      revert NotRegisteredMarket();
    }
....

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as duplicate of #515

[c4-judge]

MarioPoneder marked the issue as satisfactory