The Wildcat Protocol - kodyvim's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L142

Vulnerability details

Impact

No viable means to close markets.

Proof of Concept

Markets can be closed if there is no unpaid withdrawal batches and all debts would be paid. https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L142

function closeMarket() external onlyController nonReentrant {<@
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0;
    if (_withdrawalData.unpaidBatches.length() > 0) {
      revert CloseMarketWithUnpaidWithdrawals();
    }
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
      // Transfer excess assets to borrower
      asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
    _writeState(state);
    emit MarketClosed(block.timestamp);
  }
}

The issue is that closeMarket uses onlyController for access control within the controller contract their no implementation that utilizes closeMarket. This breaks the closeMarket functionality since no one can actually access that function.

Tools Used

Manual Review

Recommended Mitigation Steps

Since borrowers creates and manage market they should be able to close them when it's no longer need and all debts are paid. Change the modifier to onlyBorrower

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L165

Vulnerability details

Impact

Sanctioned users can transfer to escape sanctions

Proof of Concept

Lenders can be sanctioned if a sanctioned lender tries to withdraw there funds would be blocked and escrowed. but lender can simply escape sanction by transferring their funds to another account they control before the queue for withdrawal. https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L165

function executeWithdrawal(
    address accountAddress,
    uint32 expiry
  ) external nonReentrant returns (uint256) {
        ...SNIP
    if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
@>    _blockAccount(state, accountAddress);
      address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
        accountAddress,
        borrower,
        address(asset)
      );
      asset.safeTransfer(escrow, normalizedAmountWithdrawn);
      emit SanctionedAccountWithdrawalSentToEscrow(
        accountAddress,
        escrow,
        expiry,
        normalizedAmountWithdrawn
      );
    } else {
      asset.safeTransfer(accountAddress, normalizedAmountWithdrawn);
    }

    emit WithdrawalExecuted(expiry, accountAddress, normalizedAmountWithdrawn);

    // Update stored state
    _writeState(state);

    return normalizedAmountWithdrawn;
  }

One could argue that borrower would nukeFromOrbit to manually block the lender, then the borrower would have to nukeFromOrbit from all markets in which the lender participates in which could be inefficient as it defeats the purpose of automatically blocking the lender when he/she tries to withdraw and could also be susceptible to frontrun.

Since _transfer does not check whether they user is sanctioned, they could simply transfer to another account.

Tools Used

Manual Review

Recommended Mitigation Steps

Within _transfer check if the from has been sanctioned, if sanctioned revert.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L166 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L172

Vulnerability details

Impact

Escrowed funds would be wrongly sent to the borrower and can be triggered by anyone.

Proof of Concept

Escrow contract are used to lock funds for sanctioned user. This issue is that when creating an escrow within market it mismatches the address of the borrower and user when call createEscrow. One Instance: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L166

   function executeWithdrawal(
    address accountAddress,
    uint32 expiry
  ) external nonReentrant returns (uint256) {
     ...SNIP
     if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
      _blockAccount(state, accountAddress);
      address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
 @>     accountAddress,
 @>     borrower,
        address(asset)
      );
      asset.safeTransfer(escrow, normalizedAmountWithdrawn);
      emit SanctionedAccountWithdrawalSentToEscrow(
        accountAddress,
        escrow,
        expiry,
        normalizedAmountWithdrawn
      );
    } else {
      asset.safeTransfer(accountAddress, normalizedAmountWithdrawn);
    }

    emit WithdrawalExecuted(expiry, accountAddress, normalizedAmountWithdrawn);

    // Update stored state
    _writeState(state);

    return normalizedAmountWithdrawn;
  }

Looking at createEscrow the params should be borrower followed by the account to be locked in.

function createEscrow(
@>  address borrower,
@>  address account,
    address asset
  ) public override returns (address escrowContract) {
    ...SNIP
 }

Due to this mismatch the borrower address would be seen as the lender address and vice versa within WildcatSanctionsEscrow.

• Escrowed funds can be released immediately by anyone since IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account); would always return false.
• Escrowed funds would be wrongly release to the borrower instead of to the lender.

Another Instance of same issue: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L172

function _blockAccount(MarketState memory state, address accountAddress) internal {
	...SNIP
      if (scaledBalance > 0) {
        account.scaledBalance = 0;
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
@>        accountAddress,
@>        borrower,
          address(this)
        );
        emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
        _accounts[escrow].scaledBalance += scaledBalance;
        ...SNIP
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Place the borrower and account in the correct order as in the createEscrow function.

Assessed type

Error