c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

Asset balance is overestimated by amount of withdrawableFees in delinquency check in _writeState() while collecting protocol fees.

Suppose current market state: 1) assetBalance = 1000 2) accruedProtocolFees = 200 3) requiredReserves = 600 4) unclaimedWithdrawals = 300

Obviously after repaying protocolFee market should be marked delinquent because 900 > 800 is true However it will perform check 900 > 1000 is false, leaving market non-delinquent

Proof of Concept

Here it firstly updates state.accruedProtocolFees, than writes state, than transfers asset. Note that asset transfer is after _writeState.

  function collectFees() external nonReentrant {
    ...
    state.accruedProtocolFees -= withdrawableFees;
    _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
    emit FeesCollected(withdrawableFees);
  }

_writeState() checks whether market is delinquent:

  function _writeState(MarketState memory state) internal {
@>  bool isDelinquent = state.liquidityRequired() > totalAssets();
    state.isDelinquent = isDelinquent;
    _state = state;
    emit StateUpdated(state.scaleFactor, isDelinquent);
  }

Let's have a look on what values are checked. Asset balance is checked against liabilities. Note that state.accruedProtocolFees was previously reduced by withdrawableFees totalAssets() is current balance of asset:

  function totalAssets() public view returns (uint256) {
    return IERC20(asset).balanceOf(address(this));
  }

  function liquidityRequired(
    MarketState memory state
  ) internal pure returns (uint256 _liquidityRequired) {
    uint256 scaledWithdrawals = state.scaledPendingWithdrawals;
    uint256 scaledRequiredReserves = (state.scaledTotalSupply - scaledWithdrawals).bipMul(
      state.reserveRatioBips
    ) + scaledWithdrawals;
    return
      state.normalizeAmount(scaledRequiredReserves) +
      state.accruedProtocolFees +
      state.normalizedUnclaimedWithdrawals;
  }

Issue is that above check occurs before asset transfer. In fact asset balance is higher by the amount of withdrawableFees than actual balance during delinquency check

  function collectFees() external nonReentrant {
    ...
    state.accruedProtocolFees -= withdrawableFees;
    _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
    emit FeesCollected(withdrawableFees);
  }

That's how asset is overestimated and can not mark market delinquent.

Tools Used

Manual Review

Recommended Mitigation Steps

Check it after transfer

  function collectFees() external nonReentrant {
    MarketState memory state = _getUpdatedState();
    if (state.accruedProtocolFees == 0) {
      revert NullFeeAmount();
    }
    uint128 withdrawableFees = state.withdrawableProtocolFees(totalAssets());
    if (withdrawableFees == 0) {
      revert InsufficientReservesForFeeWithdrawal();
    }
    state.accruedProtocolFees -= withdrawableFees;
-   _writeState(state);
    asset.safeTransfer(feeRecipient, withdrawableFees);
+   _writeState(state);
    emit FeesCollected(withdrawableFees);
  }

And change it in borrow() for future, despite now there is no issue

  function borrow(uint256 amount) external onlyBorrower nonReentrant {
    MarketState memory state = _getUpdatedState();
    if (state.isClosed) {
      revert BorrowFromClosedMarket();
    }
    uint256 borrowable = state.borrowableAssets(totalAssets());
    if (amount > borrowable) {
      revert BorrowAmountTooHigh();
    }
-   _writeState(state);
    asset.safeTransfer(msg.sender, amount);
+   _writeState(state);
    emit Borrow(amount);
  }