The Wildcat Protocol - yumsec's results

Lines of code

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

Vulnerability details

Impact

In the file WildcatMarket.sol, function borrow calls _writeState before transferring token from contract to borrower. Thus, _writeState cannot reflect the actual vault liquidity or asset, then calculate the wrong parameter isDelinquent.

If isDelinquent is wrong, delinquency fee cannot be calculated correctly, which may bring losses to lender to borrower.

Proof of Concept

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);
    emit Borrow(amount);
}

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

State parameter isDelinquent compares liquidityRequired and totalAssets to determine if vault is under-collateralized. In the function borrow, this parameter is calculated before token transfer, which means borrow amount has not been subtracted from totalAssets() , leading to wrong calculation result.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Call _writeState(state); after token transfer.

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();
    }
    asset.safeTransfer(msg.sender, amount);
    _writeState(state);
    emit Borrow(amount);
}

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In the file WildcatMarket.sol, function collectFees calls _writeState before transferring token from contract to borrower. Thus, _writeState cannot reflect the actual vault liquidity or asset, then calculate the wrong parameter isDelinquent.

If isDelinquent is wrong, delinquency fee cannot be calculated correctly, which may bring losses to lender to borrower.

Proof of Concept

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);
    emit FeesCollected(withdrawableFees);
}

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

function withdrawableProtocolFees(
    MarketState memory state,
    uint256 totalAssets
  ) internal pure returns (uint128) {
    uint256 totalAvailableAssets = totalAssets - state.normalizedUnclaimedWithdrawals;
    return uint128(MathUtils.min(totalAvailableAssets, state.accruedProtocolFees));
}

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

State parameter isDelinquent compares liquidityRequired and totalAssets to determine if vault is under-collateralized. In the function borrow, this parameter is calculated before token transfer, which means borrow amount has not been subtracted from totalAssets() , leading to wrong calculation result.

Let’s take a look at an example:

1. Imagine vault A has 1000 WETH capacity, currently it has scaledTotalSupply 1000 WETH, totalAssets 200 WETH, liquidityRequired 200 WETH, normalizedUnclaimedWithdrawals 10 WETH, accruedProtocolFees 2 WETH.
2. Borrower wants to withdraw protocol fee, withdrawableFees is 2 WETH, which means borrower can withdraw all protocol fee from the vault. Because write state happens before fee transfer, isDelinquent = state.liquidityRequired() > totalAssets(), where liquidityRequired is 200 WETH and totalAssets() is still 200 WETH. Thus isDelinquent is false, which is wrong. Since protocol fee has been withdrawn, totalAssets() drops to 198 WETH, state.liquidityRequired() is still 200 WETH, isDelinquent should be true.
3. In this case, state records isDelinquent incorrectly, thus delinquency fee cannot be calculated correctly, causing losses to lenders.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Call _writeState(state) after transferring fee to fee recipient.

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;
    asset.safeTransfer(feeRecipient, withdrawableFees);
    _writeState(state);
    emit FeesCollected(withdrawableFees);
}

Assessed type

Other