The Wildcat Protocol - J4X's results

Lines of code

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

Vulnerability details

Impact

As per the information and documentation, the protocol does not exclude rebasing tokens, which could be used as underlying assets for newly created markets. This issue primarily pertains to rebasing tokens that increase a holder's balance over time.

When a borrower closes a market, a snapshot is taken of the current fees, interest, and other parameters, and a final scale factor is calculated for all lenders. If there is insufficient liquidity to fulfill this factor, the remaining needed tokens are transferred from the borrower to the market. Subsequently, lenders have the option to burn their market tokens to retrieve the amount of underlying assets they are entitled to based on the scaleFactor.

The problem arises when there is a arbitraril long time gap between the market's closure and the lender's withdrawal of their tokens. As rebasing tokens continue to accrue interest during this period, the market's balance keeps increasing. Unfortunately, these additional tokens cannot be retrieved from the market and remain there indefinitely. The borrower cannot withdraw anything from the market anymore, and the lenders can only withdraw based on the snapshot taken when the market was closed.

Proof of Concept

An example proof of concept could unfold as follows:

1. A borrower creates a vault with a rebasing underlying token.
2. Lenders deposit funds into the vault.
3. The borrower utilizes the capital and at some point recoups the investment.
4. The borrower intends to repay the lenders and calls closeMarket().
5. The final scale factor is calculated, and all liquidity is now in the market.
6. While lenders don't reclaim their tokens, the market's balance continues to accrue tokens.
7. When lenders claim their tokens, all newly accrued tokens remain in the market and cannot be retrieved.

Tools Used

Manual Review

Recommended Mitigation Steps

Simple Solution: The simple approach involves either excluding rebasing tokens from the protocol or adding a warning to the documentation. This warning would inform users that in this edge case, any newly accrued tokens between the market's closure and the withdrawal will be lost.

Complex Solution: The more intricate solution entails calculating the withdrawalScaleFactor by dividing the amount of underlying tokens in the vault by the amount of market tokens. This approach should ensure accuracy, as after closing, the market should have exactly scaleFactor * marketToken.totalSupply() of the underlying tokens. However, implementing this solution introduces potential new vulnerabilities and rounding issues. The decision on which solution to adopt ultimately rests with the protocol team.

Assessed type

ERC20

Low

[L-01] Lenders can also deposit when not authorized on the controller

WildcatMarketController Line 169

Issue Description: The contest description incorrectly states that "Lenders that are authorized on a given controller (i.e. granted a role) can deposit assets to any markets that have been launched through it.". However, this is not the case as borrowers need to call updateLenderAuthorization() when deauthorizing a lender. If a borrower forgets to call this function, the lender can be deauthorized on the controller but still deposit new funds into the market until the lender or someone else calls updateLenderAuthorization.

Recommended Mitigation Steps: It is recommended to update the documentation to state that this is only correct if updateLenderAuthorization() was called afterward. If the intent is to have the functionality work as described in the contest description, an atomic removal of lenders would need to be implemented.

---

[L-02] No controllers can be deployed if certain tokens are chosen as feeAsset

WildcatMarketController Line 345

Issue Description: The MarketControllerFactory allows for setting an originationFeeAsset as well as originationFeeAmount, which are used to send a fee to the recipient each time a new market is deployed. When a market is deployed, the originationFeeAmount of the originationFeeAsset is transferred from the borrower to the feeRecipient. There is one additional check in place that verifies if the originationFeeAsset address is 0 and only transfers a fee if it is not zero.

if (originationFeeAsset != address(0)) {
	originationFeeAsset.safeTransferFrom(borrower, parameters.feeRecipient, originationFeeAmount);
}

The issue with this implementation is that some tokens, like LEND, may revert in case of a zero transfer. This means that if a token like LEND is set as the originationFeeAsset, and later on the fee is reduced to zero, this function will always fail to execute, preventing any new markets from being deployed.

Additionally, not checking for zero transfers could lead to gas waste in the case of a token that does not revert but simply transfers nothing during a zero transfer.

Recommended Mitigation Steps: To fix this issue, an additional check needs to be added to the if clause, ensuring that originationFeeAmount is greater than zero:

if (originationFeeAsset != address(0) && originationFeeAmount > 0) {
	originationFeeAsset.safeTransferFrom(borrower, parameters.feeRecipient, originationFeeAmount);
}

---

[L-03] getDeployedControllers() will not return the last index

WildcatMarketControllerFactory Line 138

Issue Description: The getDeployedControllers() function takes a start and end index of the controllers to be retrieved. However, due to the current implementation of the function, it only returns controllers from the start index to end-1. In other words, the controller at the end index is not included in the returned list.

POC: This simple POC can be added to the WIldcatMarketController test to check for the issue.

function test_getControllers() external {
//Deploy 10 contracts
	for(uint256 i = 0; i < 10; i++)
	{
		controllerFactory.deployController();
	}
	  
	//Want to access 2-7 (6 controllers)
	address[] memory controllers = controllerFactory.getDeployedControllers(2, 7);
	
	//Check that the length is correct
	require(controllers.length == 5, "We have not received an incorrect number");
	
	//We only received controllers 2-6 due to the implementation
}

The same issue also exists in the functions WildcatMarketController.getAuthorizedLenders(), WildcatMarketController.getControlledMarkets() , WildcatArchController.getRegisteredBorrowers(), WildcatArchController.getRegisteredControllerFactories(), WildcatArchController.getRegisteredControllers() and WildcatArchController.getRegisteredMarkets().

Recommended Mitigation Steps: To resolve this issue, the getDeployedControllers() function should be rewritten as follows:

function getDeployedControllers(
uint256 start,
uint256 end
) external view returns (address[] memory arr) {
	uint256 len = _deployedControllers.length();
	end = MathUtils.min(end, len-1);
	uint256 count = end - start + 1;
	arr = new address[](count);
	for (uint256 i = 0; i < count; i++) {
		arr[i] = _deployedControllers.at(start + i);
	}
}

The same change should be applied to other affected functions as well.

---

[L-04] Rebasing tokens will lead to borrowers needing to pay a lower APR

Issue Description: Rebasing tokens, which are not excluded from the contest, can be used as underlying assets for markets deployed using the protocol. Rebasing tokens can be implemented in various ways, but the critical point is when the balance of addresses holding the tokens gradually increases. As borrowers/market contracts hold these tokens while they are lent, the newly accrued tokens may either be credited to the borrower, or inside the market itself, which would count as the borrower adding liquidity. This can result in the borrower needing to pay a lower Annual Percentage Rate (APR) than initially set.

Recommended Mitigation Steps: This issue can be mitigated in several ways:

• Option 1: Disallow rebasing tokens from the protocol to prevent this situation.
• Option 2: Add a warning to the documentation, informing users that when lending rebasing tokens, the rebasing interest their tokens gain while inside the market will be counted as the borrower paying down their debt.
• Option 3 (Complicated): Implement functionality for rebasing tokens by checking the market's balance at each interaction and adding the change to a separate variable that tracks rebasing awards.

---

[L-05] Reserve ratio can be set to 100%

MarketControllerFactory Line 85

Issue Description: The protocol allows borrowers to set a reserve ratio that they must maintain to avoid being charged a delinquency fee. In the current implementation, this parameter can be set to 100%, rendering the entire functionality redundant, as borrowers would not be able to withdraw any funds from the market. Additionaly the market would fall into delinquency immediately after the start.

Recommended Mitigation Steps: To mitigate this issue, modify the check on maximumReserveRatioBips to revert if constraints.maximumReserveRatioBips >= 10000.

---

[L-06] scaleFactor can theoretically overflow

FeeMath Line 169

Issue Description: The scaleFactor of a market is multiplied by the fee rate to increase the scale. In a very rare edge case, where a market has a 100% interest (e.g., a junk bond) and is renewed each year with the borrower paying lenders the full interest, the scale factor would overflow after 256 years (as the scale factor doubles every year) when it attempts to increase during the withdrawal amount calculation.

Recommended Mitigation Steps: While this issue is unlikely to occur in practice, a check should be added in the withdrawal process to prevent an overflow. If an overflow is detected, lenders should be forced to withdraw with a scale factor of uint256.max, and the borrower should close the market.

---

[L-07] Misleading ERC20 queries balanceOf() and totalSupply()

WildcatMarketToken Line 16 WildcatMarketToken Line 22

Issue Description: The WildcatMarketToken contract includes standard ERC20 functions, balanceOf() and totalSupply(). However, these functions return the balance of the underlying tokens instead of the market tokens. This discrepancy between the function names and their actual behavior could lead to confusion or issues when interacting with other protocols.

Recommended Mitigation Steps: To address this issue, it is recommended to rename the existing functions to balanceOfScaled() and totalScaledSupply(), and additionally implement balanceOf() and totalSupply() functions that return the balance of the market token.

---

[L-08] Closed markets can't be reopened

WildcatMarket Line 142

Issue Description: Markets include a functionality where users can close markets directly, effectively transferring all funds back into the market and setting the isClosed parameter of the state to true. While this prevents new lenders from depositing into the market, it only allows lenders to withdraw their funds and interest. The issue is that, once a borrower uses this function, the market cannot be reopened. If the borrower wants to have another market for the same asset, they must deploy a new market with a new prefix to avoid salt collisions. If a borrower does this often it might end up in the Market names looking like "CodearenaV1234.56DAI" due to new prefixes being needed each time. Additionally the markets list would get more and more bloated.

Recommended Mitigation Steps: To mitigate this issue, borrowers should be allowed to reset a market. This would require all lenders to withdraw their funds before the reset, but it would reset all parameters, including the scale factor, allowing the market to be restarted.

---

[L-09] Choosable prefix allows borrowers to mimic other borrowers.

WildcatMarketBase Line 97

Issue Description: When a new market is deployed, its name and prefix are generated by appending the underlying asset's name and symbol to the name and symbol prefixes provided by the borrower. In the contest description this is explained as Code4rena deploying a market and passing Code4rena as name prefix and C4 as symbol prefix.

A malicious borrower could exploit this functionality to deploy markets for other well-known assets with the same Code4rena prefixes, potentially tricking lenders into lending them money and mimicking other borrowers. This could lead to confusion and potentially fraudulent activities.

Recommended Mitigation Steps: The recommended solution for this is to let each borrower choose a borrower identifier (that gets issued to them by wildcat). This in the Code4rena example would be the "Code4rena" and "C4" strings. Now the hashes (hashes instead of strings to save gas on storage cost) of those identifiers get stored onchain whenever a new borrower gets added. Whenever Code4rena deploys a new market they pass their identifier as well as a chosen Postfix which would allow them to deploy multiple markets for the same asset (for example with different interest rates). The protocol then verifies if the identifiers match the hash and revert if that is not the case. The market mame would then for example look like this "Code4renaShortTermDAI".

[L-10] Interest continues to accrue up to the expiry.

Issue Description: The whitepaper on page 12 states that interest ceases to be paid at the time when tokens get burned or when withdrawals are queued in the line "Interest ceases to be paid on Bob's deposit from the moment that the whcWETH tokens were burned, regardless of the length of the withdrawal cycle". However, the actual code behavior differs. In the Wildcat protocol's implementation, interest continues to accrue until the expiration of the withdrawal period, as evident in the code snippet below:

if (state.hasPendingExpiredBatch()) {
  uint256 expiry = state.pendingWithdrawalExpiry;
  // Interest accrual only if time has passed since last update.
  // This condition is only false when 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);
  }
  _processExpiredWithdrawalBatch(state);
}

Recommended Mitigation Steps: To address this issue, there are two potential courses of action:

If the intended behavior is for interest to continue accruing until the withdrawal period expires, then the documentation should be updated to align with the current code behavior.

If the documentation accurately reflects the intended interest-accrual behavior (i.e., interest should stop accruing when withdrawals are queued), then the conditional statement as shown in the code snippet should be removed from the function _getUpdatedState().

Non-Critical

[NC-01] Badly named constant BIP

MathUtils Line 6

Issue Description: The variable BIP is used to represent the maximum BIP in the protocol, where at most different rates can be set to 10,000, equivalent to 100%. The variable name could be misleading, as it may incorrectly suggest that it represents one BIP, which should be equal to 1.

Recommended Mitigation Steps: To enhance clarity, rename the variable to MAX_BIP.

---

[NC-02] Incorrect documentation on capacity resizing

Issue Description: The whitepaper (on page 5) states that the "maximum capacity of a vault can be adjusted at will up or down by the borrower depending on the current need." However, the maxCapacity can only be reduced down to the current liquidity inside the market, as per the code. This discrepancy between the documentation and the code could lead to misunderstandings.

if (_maxTotalSupply < state.totalSupply()) {
	revert NewMaxSupplyTooLow();
}

Recommended Mitigation Steps: Revise the whitepaper to accurately reflect that the "maximum capacity of a vault can be adjusted at will, but only down to the current supply of the market."

---

[NC-03] Incorrect documentation on authentication process

Whitepaper, page 7

Issue Description: The whitepaper states that "Vaults query a specified controller to determine who can interact." However, the interaction rights are set by the controller on the market. There is no callback from the market to the controller, except for the initial call of the lender when authorization is null. The whitepaper should be updated to align with the actual code implementation.

Recommended Mitigation Steps: Update the documentation to describe the functionality as it exists in the code.

---

[NC-04] Incorrect documentation of registerControllerFactory()

WildcatArchController Line 106

Issue Description: The documentation in the GitBook states that the function registerControllerFactory() only reverts if "The controller factory has already been registered." This is inaccurate because the function also uses the onlyOwner() modifier, which causes it to revert if called by someone other than the owner.

Recommended Mitigation Steps: Revise the documentation to include the statement "Called by someone other than the owner."

---

[NC-05] Incorrect documentation of removeControllerFactory()

WildcatArchController Line 113

Issue Description: The documentation in the GitBook](https://wildcat-protocol.gitbook.io/wildcat/technical-deep-dive/component-overview/wildcatarchcontroller.sol) states that the function removeControllerFactory() only reverts if "The controller factory address does not exist or has been removed." This is not entirely accurate, as the function also employs the onlyOwner() modifier, causing it to revert if called by someone other than the owner.

Recommended Mitigation Steps: Update the documentation to include the statement "Called by someone other than the owner."

---

[NC-06] Documentation of functions is missing

Issue Description: The GitBook](https://wildcat-protocol.gitbook.io/wildcat/technical-deep-dive/component-overview/wildcatarchcontroller.sol) documentation lists numerous functions in the component overview section that lack descriptions. This omission can make it challenging for users and developers to understand the purpose and usage of these functions.

Recommended Mitigation Steps: Provide descriptions for the missing functions in the documentation to enhance clarity and understanding.

---

[NC-07] Incorrect comment in _depositBorrowWithdraw()

BaseMarketTest Line 71

Issue Description: Inside the function the comments state that 80% of the market assets get borrowed and 100% of withdrawal get withdrawn. This is not the case as instead the provided parameters depositAmount, borrowAmount and withdrawalAmount are used. There are no require statements in the function that check for these requirements 80%/100% being fulfilled by the parameters.

Recommended Mitigation Steps: To address this issue, consider either adding require statements to verify that the parameters adhere to the stated percentages, or remove the comments if they no longer apply to the function's functionality.

Introduction

This analysis report focuses on the audit of the Wildcat Finance protocol hosted on Code4rena. The primary objectives of this audit were to identify potential vulnerabilities, propose mitigation strategies, and optimize gas usage within the codebase.

| Spec        | Value           |
| ----------- | --------------- |
| Name        | Wildcat Finance |
| Duration    | 10 days         |
| nSLOC       | 1879            |

Methodology

The audit process was divided into three distinct phases:

Recon

In the Recon phase, I conducted an initial review of the codebase to gain a broad understanding of its structure and functionality. This preliminary pass allowed me to familiarize myself with the code before diving into a deeper analysis. I also reviewed provided documentation (whitepaper and gitbook), gathered additional information from online sources, and participated in the audit's Discord channel. Additionally, I reviewed the preliminary audit report.

Analysis

The Analysis phase formed the core of the audit, involving a detailed examination of the codebase. My approach included meticulously documenting suspicious code sections, identifying minor bugs, and envisioning potential attack vectors. I explored various attack scenarios that could manifest within the code and delved into the relevant functions. Throughout this phase, I maintained ongoing communication with the protocol team to verify findings and clarify any ambiguities.

Reporting

After compiling initial notes during the Analysis phase, I refined and expanded upon these findings. I also developed test cases to complement the identified issues and integrated them into the final report. Furthermore, I generated a Quality Assurance (QA) report alongside this comprehensive analysis.

System Overview

The system can be explained based on three key actors that interact with it: the owner, borrowers, and lenders. Each of these actors has their unique pathways for interacting with the market.

Owner

In the current implementation, the owner is the protocol team. The owner's tasks and functionalities include allowing or disallowing borrowers from deploying new markets and controllers. This is accomplished by the owner interacting with the WildcatMarketControllerFactory and the WildcatArchController. The owner uses these to either register new deployments at the WildcatArchController or to set the ranges for the parameters at the WildcatMarketControllerFactory.

Recommendations In the current implementation, interest rates, protocol fees, and delinquency fees cannot exceed 100%, as the maximum limit the owner can set is 100%. This limitation restricts the protocol's ability to be used for real-world lending, as interest rates can often exceed 100% in various scenarios, such as lending in high inflation environments or high-risk lending. It is advisable to allow for more flexibility in interest rates to cater to real-world use cases.

Borrower

The borrower is an entity authorized by the owner and can deploy new markets. The borrower is able to deploy one controller, from which he can then deploy multiple markets with different parameters. The borrower also needs to allow lenders to deposit funds into markets, as users are not automatically able to deposit. The authorization updating feature can be seen in the following image:

Additionally in case the borrower gets knowledge of a lender being sanctioned by OFAC, the borrower can call to the function nukeFromOrbit() to move the lenders assets to an escrow and prevent him from directly receiving assets from the market. This process can be seen in the following diagram:

Recommendations In the current implementation the users authorization to access a market is stored in 2+ separate locations, which can have different values either between the controller and a market as well as between different markets deployed by the same controller. This opens the door to different vulnerabilities resulting out of inconsistencies. It is recommended to simplify the whole process to mitigate already existing and potential future vulnerabilities. As the authorization should anyways be the same for all markets deployed by a controller, it would make sense to offer a low gas view function on the controller which returns if a user is authorized and call that function for deposits / withdrawals at markets deployed through the controller. This would allow removing all the functionality/storage that needs to be saved in each market.

The implementation does not directly check for the provided prefixes to be related to the borrower. This can lead to borrowers masking as other borrowers without any way to prevent this. It would be recommended to adapt this so that a borrower's market's prefix is somewhat related to the borrower.

Lender

The lenders interactions with the protocol include the lender depositing into a market as well as withdrawing his funds. The withdrawal of funds is batched, as long as withdrawalBatchDuration is not 0. So the withdrawal process takes place in 2 iterations.

The deposit process is rather simple, as shown in the diagram below. A user can deposit underlying tokens up to a certain amount. Then the market will check if the addition to the pool would go over the maximum total supply, and will either let the user deposit the amount of tokens needed to get to the total supply, or the provided parameter.

While the lender has money invested into the pool, interest is accrued. This interest is credited to the lender using a scale factor. The scale factor increases over time, according to the interest rates set and lets a user withdraw his funds including the gained interest.

The withdrawal process is a lot more complicated and split into the 2 functions queueWithdrawal() and executeWithdrawal(). Using queueWithdrawal() a user can queue his withdrawal request, which will then need to wait until the batch time has passed and the user can withdraw his funds. These batches are used, so that in case of there being too less funds for all lenders to be compensated, the funds are split accordingly to the amount of tokens the lenders burnt.

When a lender has queued his withdrawal, and the batch period has passed, the lender can withdraw his funds. In case of thee being enough funds the lender just gets his funds + interest. In the case of there being too less liquidity for all lenders in the batch, the available liquidity gets split.

Recommendations A lender currently has to pass the withdrawal an amount of underlying tokens he wants to withdraw. As these are scaled compared to the market tokens the user has to calculate by himself how many underlying tokens his received market tokens are worth now. Especially as the APR can be changed or a borrower could need to pay the delinquency fee, this would be pretty complicated. It would be simpler for the end user if the end user could just enter the amount of market tokens he wants to withdraw, and the calculation to the underlying tokens would be done at the protocol side.

Potential attack vectors

There are multiple assets which can be targets of attacks on the protocol:

• A market's underlying asset holdings
• A market's tokens being held by users
• A market's functionality
• A controllers functionality
• A factory's functionality

Based on this the attack vectors where already mapped pretty well in the contest description. Additionally to the attack vectors mentioned in the contest description the following attack vectors exist:

• Borrower addresses, controller factories or markets are removed from the archcontroller without the intention of the owner
• Borrower can not borrow from a deployed market
• Lenders can not deposit at a market they are authorized for and that has not reached the total supply
• Attacker forces borrower to pay the delinquency rate
• Attacker steals other users market tokens

Testing

The testsuite is created in foundry and offers testcases for almost all functionalities. Overall the testsuite offers a coverage of around 90%. The tests are well structured and a lot of helper functions simplify the interaction with the contracts. Unfortunately these simplification also lead to some errors when writing new tests, as they for example sometimes mint new assets to users or change authorizations which is not directly stated in their name. This might be very helpful for unit tests, but can lead to unintended behavior in bigger testcases.

Recommendations:

• The testsuite is missing testcases which mimic real usecases of the project (many users, multiple deposits/withdrawals, changes of APR etc.)
• WildcatMarketControllerFactory.t does not test the deployment of new controllers at all
• The testuite is lacking unit tests for getter functions
• The BoolUtils.sol library is not tested at all
• The testsuite should include more fuzztests to test edge cases

Systemic Risk

The protocol team has chosen very well in keeping the protocol as hands off as possible. The current implementation limits the damage in case of the archcontroller / owner becoming corrupted. Still, the archcontroller is currently a single point of failure which, if corrupted, allows an attacker to remove borrowers and make them incapable of deploying new markets, effectively freezing the current protocol structure. Nevertheless this would not result in big issues for the end users, due to the hands off setup of the archcontroller.

Code Quality

The code adheres to high quality standards and is easily readable. Additionally NatSpec comments are added above the functions which allow for easier understanding. Combining the market out of different contracts, which each are separated by their field of functionality simplifies the code. Separating objects like the state into libraries, also makes the code more structured.

Conclusions

The audit spanned a full ten days and comprised three phases:

• Day 1-2: Documentation review, initial code inspection, and gaining an understanding of the system's functionalities.
• Days 3-9: In-depth code analysis, vulnerability identification, and bug hunting.
• Day 10: Reporting findings, including bug reports and this comprehensive analysis.

Total time spent: 35 hours.

Time spent:

35 hours