The Wildcat Protocol - caventa's results

Lines of code

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

Vulnerability details

Impact

ScaleFactorAndFees should be updated until the most current time, rather than updated to expiry date

Proof of Concept

Sufficient liquidity is required to process withdrawals, which becomes available when the borrower repays the loan.

If a borrower doesn't repay the loan, it can lead to liquidity shortages, making it impossible for depositors to complete withdrawal batches. In such cases, fees and the scale factor should continue to accrue, not just until the expiry date of the withdrawal batch, but until the actual time when sufficient funds are available to process withdrawals.

Let's take protocol fee as an example

Added a testProtocolFee test unit to WildcatMarketWithdrawals.t.sol

function testProtocolFee() external { _depositBorrowWithdraw(alice, 1e18, 8e17, 1e18); // Deposit / Borrow / Withdraw fastForward(parameters.withdrawalBatchDuration); uint32 expiry = uint32(block.timestamp); updateState(pendingState()); market.updateState();

{
  uint32[] memory unpaidBatchExpiries = market.getUnpaidBatchExpiries();
  assertEq(unpaidBatchExpiries.length, 1); // 1 pending batch created
}

fastForward(parameters.withdrawalBatchDuration * 1000); // Forward the time

asset.mint(address(market), 8e20);
market.processUnpaidWithdrawalBatch(); // process unpaid batch

{
  uint32[] memory unpaidBatchExpiries = market.getUnpaidBatchExpiries();
  assertEq(unpaidBatchExpiries.length, 0);
}

}

In the WildcatMarketBase.sol, add 2 console.log statements

function _getUpdatedState() internal returns (MarketState memory state) {

    state = _state;
    // Handle expired withdrawal batch
    if (state.hasPendingExpiredBatch()) {
      uint256 expiry = state.pendingWithdrawalExpiry;

      // Only accrue interest if time has passed since last update.
      // This will only be false if withdrawalBatchDuration is 0.
      if (expiry != state.lastInterestAccruedTimestamp) {
        (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
          .updateScaleFactorAndFees(
            protocolFeeBips,
            delinquencyFeeBips,
            delinquencyGracePeriod,
            expiry
          );
        emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);

+       console.log('protocol fee until expired + ', protocolFee);
      }
      _processExpiredWithdrawalBatch(state);
    }
    // Apply interest and fees accrued since last update (expiry or previous tx)
    if (block.timestamp != state.lastInterestAccruedTimestamp) {
      (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
        .updateScaleFactorAndFees(
          protocolFeeBips,
          delinquencyFeeBips,
          delinquencyGracePeriod,
          block.timestamp
        );
      emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);

+     console.log('protocol fee until current date + ', protocolFee);
    }

After running the test,

protocol fee until expired = 21917808219178 protocol fee until current date = 21929678975278525

The difference is 21907761163359347

We should assign the protocol fee to the borrower, as their repayment is necessary for closing the withdrawal batch. Imposing additional protocol fees will incentivize the borrower to make their repayment earlier.

If the calculations are done up to the expiration date, then the timing of the protocol fee payment won't result in any extra charges. Therefore, the borrower won't be incentivized to repay earlier.

The same principle applies to the scale factor as well.

Tools Used

Manual and added a test unit

Recommended Mitigation Steps

Change WildcatMarketBase#_getUpdatedState

function _getUpdatedState() internal returns (MarketState memory state) {
    state = _state;
    // Handle expired withdrawal batch
    if (state.hasPendingExpiredBatch()) {
      uint256 expiry = state.pendingWithdrawalExpiry;
      // Only accrue interest if time has passed since last update.
      // This will only be false if withdrawalBatchDuration is 0.
      if (expiry != state.lastInterestAccruedTimestamp) {
        (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
          .updateScaleFactorAndFees(
            protocolFeeBips,
            delinquencyFeeBips,
            delinquencyGracePeriod,
-           expiry
+           block.timestamp
          );
        emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
      }
      _processExpiredWithdrawalBatch(state);
    }
    // Apply interest and fees accrued since last update (expiry or previous tx)
-    if (block.timestamp != state.lastInterestAccruedTimestamp) {
-      (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
-        .updateScaleFactorAndFees(
-          protocolFeeBips,
-          delinquencyFeeBips,
-          delinquencyGracePeriod,
-          block.timestamp
-        );
-      emit ScaleFactorUpdated(state.scaleFactor, baseInterestRay, delinquencyFeeRay, protocolFee);
-    }
  }

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatMarketController.sol#L468-L488

Vulnerability details

Impact

The borrower can manipulate the setAnnualInterestBips function to reduce their debt

Proof of Concept

The setAnnualInterestBips function can be modified by both the controller and the borrower, as seen in WildcatMarketConfig and WildcatMarketController.

See WildcatMarketConfig#setAnualInterestBips

 function setAnnualInterestBips(uint16 _annualInterestBips) public onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();

    if (_annualInterestBips > BIP) {
      revert InterestRateTooHigh();
    }

    state.annualInterestBips = _annualInterestBips;
    _writeState(state);
    emit AnnualInterestBipsUpdated(_annualInterestBips);
  }

See WildcatMarketController#setAnnualInterestBips

 function setAnnualInterestBips(
    address market,
    uint16 annualInterestBips
  ) external virtual onlyBorrower onlyControlledMarket(market) {
    // If borrower is reducing the interest rate, increase the reserve
    // ratio for the next two weeks.
    if (annualInterestBips < WildcatMarket(market).annualInterestBips()) {
      TemporaryReserveRatio storage tmp = temporaryExcessReserveRatio[market];

      if (tmp.expiry == 0) {
        tmp.reserveRatioBips = uint128(WildcatMarket(market).reserveRatioBips());

        // Require 90% liquidity coverage for the next 2 weeks
        WildcatMarket(market).setReserveRatioBips(9000);
      }

      tmp.expiry = uint128(block.timestamp + 2 weeks);
    }

    WildcatMarket(market).setAnnualInterestBips(annualInterestBips);
  }

While it's acceptable for the controller to adjust settings, it becomes problematic when the borrower has the ability to make modifications. Even though increasing the reserveRatioBips to 9000 may seem like a constraint on the borrower, it doesn't actually put them at a disadvantage.

For instance, in both the original and modified scenarios, the borrower can still borrow 500 tokens, regardless of the reserve ratio:

Original Scenario:

Total Supply = 10,000 Original reserveRatioBips = 500 Borrowed amount = 500

Modified Scenario:

Total Supply = 10,000 Modified reserveRatioBips = 9000 Borrowed amount = 500

What really benefits the borrower is the ability to reduce the interest rate. For example, if the interest rate is set to 1000, the value of the interest accrued after 1 hour would be a significant number. But if the rate is set to 0, no interest would accumulate. Added a test unit to WildcatMarketBaseTest.sol for this.

function testInterest() external {
    {
      vm.prank(parameters.borrower);
      controller.setAnnualInterestBips(address(market), DefaultInterest);
      MarketState memory state =  market.currentState();
      uint timeDelta = 86400; // Set to 1 hour
      uint interest = FeeMath.calculateLinearInterestFromBips(state.annualInterestBips, 86400);
      assertEq(interest, 273972602739726027397260);
    }

    {
      vm.prank(parameters.borrower);
      controller.setAnnualInterestBips(address(market), 0); // Set interest to 0
      MarketState memory state =  market.currentState();
      uint timeDelta = 86400; // Set to 1 hour
      uint interest = FeeMath.calculateLinearInterestFromBips(state.annualInterestBips, 86400);
      assertEq(interest, 0);
    }
  }

Tools Used

Manual and unit test

Recommended Mitigation Steps

While raising the reserve ratio may not necessarily deter borrowers, reducing the interest rate could benefit them by paying less interest.

I am unsure which way to handle this issue.

Suggested Solution:

To prevent this, one option is to restrict the access to WildcatMarketController#setAnnualInterestBips so that only the controller can modify it, thereby removing the ability of the borrower to manipulate interest rates.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The current implementation of the closeMarket function in the WildcatMarket contract contains flawed logic that prevents the market from closing unless borrowed tokens are repaid in the previous transaction

Proof of Concept

The WildcatMarket#closeMarket function contains a conditional check for unpaid batches of withdrawals (_withdrawalData.unpaidBatches.length() > 0). This check impedes the market from closing unless the borrower has first repaid their debt and all unpaid withdrawal batches have been processed via market.processUnpaidWithdrawalBatch().

See WildcatMarket#closeMarket

function closeMarket() external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0;
    if (_withdrawalData.unpaidBatches.length() > 0) { // @audit
      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);
  }

Added a test unit to WithdrawalsTest.t.sol to verify this

function testCloseMarket() external {
      _depositBorrowWithdraw(alice, 100, 5, 100);
      fastForward(parameters.withdrawalBatchDuration);
      updateState(pendingState());
      market.updateState();

      _depositBorrowWithdraw(alice, 100, 5, 100);
      fastForward(parameters.withdrawalBatchDuration * 2);
      updateState(pendingState());
      market.updateState();

      market.processUnpaidWithdrawalBatch(); // process unpaid batch

      {
        uint32[] memory unpaidBatchExpiries = market.getUnpaidBatchExpiries();
        assertEq(unpaidBatchExpiries.length, 1); // length = 1
      }

      assertEq(IERC20(address(asset)).balanceOf(address(market)), 190); // 100 - 5 + 100 - 5 = 190

      vm.expectRevert();
      vm.prank(address(controller));
      market.closeMarket(); // Try to close market but fail

      market.processUnpaidWithdrawalBatch(); // process unpaid batch again

      {
        uint32[] memory unpaidBatchExpiries = market.getUnpaidBatchExpiries();
        assertEq(unpaidBatchExpiries.length, 1); // length still = 1
      }

      vm.expectRevert();
      vm.prank(address(controller));
      market.closeMarket(); // Try to close market again but still fail

      asset.mint(address(market), 10);
      assertEq(IERC20(address(asset)).balanceOf(address(market)), 200); // 100 - 5 + 100 - 5  +10 = 200

     // AFTER THERE IS SUFFICIENT BALANCE

     market.processUnpaidWithdrawalBatch(); // process unpaid batch again

    {
      uint32[] memory unpaidBatchExpiries = market.getUnpaidBatchExpiries();
      assertEq(unpaidBatchExpiries.length, 0); // length become 0
    }

    vm.prank(address(controller));
    market.closeMarket(); // Able to close market
  }

The scenarios are

1. During the first expiration period, deposit 100 tokens, borrow 5 tokens, and then withdraw 100 tokens.
2. In the second expiration period, deposit another 100 tokens, borrow another 5 tokens, and withdraw 100 tokens again.
3. With two outstanding withdrawal batches, process the first one.
4. Attempting to process the second batch and close the market should fail due to insufficient funds.
5. Mint 10 more token
6. Now, attempting to process the second batch and close the market should succeed.

Tools Used

Manual and added a test unit

Recommended Mitigation Steps

The unit tests outlined above indicate that to process batches and close the market, additional tokens must be added to the contract. There are three possible methods for doing so:

a) Manually transferring tokens to the contract. b) Having lenders deposit more tokens into the contract. c) Allowing the borrower to repay tokens to the contract.

The first and second options are not viable, as there's no incentive for anyone to manually transfer or deposit additional tokens. Therefore, the most practical way is to require the borrower to repay tokens to the contract. In order to do so, the asset transfer logic should be separated from the existing closeMarket function. This allows the borrower to settle their debt independently in the previous transaction before closing market in the new transaction.

Therefore, I would suggest changing WildcatMarket.sol

Add a new forceRepay function

function forceRepay() external onlyController nonReentrant {
    // Make sure to update the market state before calculating debts and assets
    MarketState memory state = _getUpdatedState();
    
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();

    if (currentlyHeld < totalDebts) {
        // Transfer remaining debts from borrower to the market
        asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
        // Transfer excess assets from the market to the borrower
        asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
}

2. Remove repay logic from WildcatMarket#closeMarket

 function closeMarket() external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0; // a: where does this is used?
    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);
  }

After these code modifications, the process to close the market would be:

1. Controller calls forceRepay to force the borrower to settle all his debts.
2. market.processUnpaidWithdrawalBatch(); is executed as many times as needed to clear all unpaid withdrawal batches.
3. Finally, closeMarket is called to close the market.

Assessed type

Invalid Validation