The Wildcat Protocol - InAllHonesty's results

Lines of code

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

Vulnerability details

Impact

The documentation specifies the following: The grace period is a rolling limit: once delinquency has been cured within a market, the grace tracker will count back down to zero from whatever value it had reached, and any penalty APR that is currently in force will only cease to do so after the grace tracker value is once again below the grace period.

This implies that lenders are entitled to receiving the penalty APR for the difference between grace tracker value and grace period.

If borrower racks up a big delinquency time he can just close the market to avoid paying that difference worth of penalty APR to the lenders defeating the whole purpose of the rolling limit concept.

It's medium because the borrower can do this as often as it makes economical sense, causing a most likely low financial impact to his lenders.

Proof of Concept

Please copy the following in the WildcatMarketWithdrawals.t.sol file:

  function test_justCloseMarketIfYouDelinquent() external  asAccount(parameters.controller){
    // Borrow 80% of deposits then request withdrawal of 100% of deposits
    _deposit(bob, 5e17);
    _depositBorrowWithdraw(alice, 1e18, 8e17, 9e17);
    // Borrower is delinquent because state.liquidityRequired() > totalAssets(); We fastforward a couple of days to add some delinquency time.
    fastForward(10 days);
    market.updateState();

    MarketState memory state = market.currentState();
    // Check that current market isDelinquent
    assertEq(state.isDelinquent, true);

    // Repay an amount sufficient to cover all interest + fees
    asset.mint(address(market), 1e18);
    // Process payment
    market.processUnpaidWithdrawalBatch();
    market.updateState();
    uint32[] memory unpaidBatchExpiries = market.getUnpaidBatchExpiries();
    
    // Check there aren't any unpaid batches
    assertEq(unpaidBatchExpiries.length, 0);
    
    MarketState memory state2 = market.currentState();
    console2.log("timeDelinquent = %s", state2.timeDelinquent); //10 days of delinquency
    assertGt(state2.timeDelinquent, 0);

    // Borrower closes the market to avoid to pay the DelinquencyFee while both Bob and Alice had some amounts lended, that could have benefited from the penalty APR.
    market.closeMarket();

    market.updateState();
    MarketState memory state3 = market.currentState();
    
    // Check if the market closed
    assertEq(state3.isClosed, true);
  }

Tools Used

Manual review

Recommended Mitigation Steps

This problem can be solved in multiple ways:

1. Drop the rolling grace period concept altogether. The penalty APR applies only for the period of delinquency.

OR

2. Introduce a one time penalty in the closeMarket function that applies only if there's still some period of delinquency left when the borrower calls the closeMarket function. This one time penalty could be equal to each lender's amount multiplied with the penalty APR (just the penalty APR) adjusted for the actual delinquency time remaining.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L137-L188 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L77-L121

Vulnerability details

Impact

Rebasing tokens break the accounting of the protocol in various places causing loss of funds for lender/borrower depending on whether the underlying asset rebases it's supply up or down.

Proof of Concept

The logic is as follows:

1. Alice, a whitelisted borrower, want to borrow 90 stETH from her lenders. She configures a Market Capacity of 100 stETH, with a Reserve Ratio of 10%, 5% lender APR, 5% penalty rate, 3 days grace, 10 days withdrawal cycle.
2. Bob provides the 100 stETH.
3. Time passes and Bob wants some of his stETH back. Bob queues an withdrawal for 10 stETH by calling queueWithdrawal.
4. This starts a new withdrawal batch, that saves the scaled withdrawal amount to state.

        _withdrawalData.accountStatuses[expiry][msg.sender].scaledAmount += scaledAmount;
        batch.scaledTotalAmount += scaledAmount;
        state.scaledPendingWithdrawals += scaledAmount;

5. The 10 days withdrawal cycle pass and executeWithdrawal is called. In the meantime the 10 stETH rebased 9-10 times given that it rebases daily. The difference is not taken into account anywhere.

Several other variables are affected in case of daily rebases.

Tools Used

Manual review

Recommended Mitigation Steps

If feasible, use a token whitelist, or at least limit supported tokens in your UI and document the risks of nonstandard tokens for both the lender and the borrower.

Assessed type

ERC20

LOW-1 Use of balanceOf(address(this)) for accounting purposes

totalAssets() function uses IERC20(asset).balanceOf(address(this)) which is a really bad practice.

Even if right now it doesn't break the accounting of withdrawableFees, borrowableAssets, availableLiquidity, given that everyone could influence it by arbitrarily sending the underlying token ... I propose to limit that by computing the totalAssets value using other components like deposits and withdraws.

This design choice originates from the lack of a specific repay method for the borrower.

Recommendation: Track the totalAssets using inflows and outflows and create a specific repay function for the borrower in order to negate any possible external influence.

LOW-2 MinimumDelinquencyGracePeriod and MaximumDelinquencyGracePeriod should have hardcoded limits.

Those two variables have a significant influence on the flow of the borrower/lender relation. A MaximumDelinquencyGracePeriod of 356 days gives too much leniency to the borrower, rendering the delinquency penalty useless, while a MinimumDelinquencyGracePeriod of 0 will make lenders game the system in order to have the delinquency penalty active 100% of the time.

Recommendation: Set a hard limit of let's say 1 day for the MinimumDelinquencyGracePeriod and 7-10 days for the MaximumDelinquencyGracePeriod

LOW-3 Given that an Annual Interest = 0 is a special situation happening after a market gets closed using the closeMarket function, being able to call setAnnualInterestBips(0) shouldn't be possible.

Put this in WildcatMarketConfig.t.sol:

  function test_annualInterestBipsZero() external returns (uint256) {
    assertEq(market.annualInterestBips(), parameters.annualInterestBips);
    vm.prank(parameters.controller);
    market.setAnnualInterestBips(0);
    assertEq(market.annualInterestBips(), 0);
  }

Recommendation: in WildcatMarketConfig.sol

    function setAnnualInterestBips(uint16 _annualInterestBips) public onlyController nonReentrant {
++      if (_annualInterestBips == 0) revert InterestZero();
        MarketState memory state = _getUpdatedState();

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

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

Overview

The Wildcat protocol facilitates the creation of credit lines that require less collateral from borrowers compared to the usual solutions available in web3. The protocol provides significant control to borrowers. These are able to control key parameters like interest rates, underlying assets, penalties, reserve ratios etc.

The protocol is responsible for vetting the borrowers and in turn borrowers are responsible for vetting their lenders, everything being conducted under the umbrella of a Service Agreement concluded between the protocol and all of its users (both lenders and borrowers). The relation between borrowers and lenders is governed by a Master Loan Agreement that needs to be signed by the borrower when a new market is deployed and by the lender at the first interaction they have with the market.

After a market is deployed Wildcat has no control over it.

Understanding the Ecosystem

Borrowers go through a thorough vetting process carried by Wildcat and could be considered trusted accounts. Borrowers use approved factory contracts to deploy market controllers. Borrowers have control over market creation and can set all the initial market parameters:

• asset
• namePrefix
• symbolPrefix
• maxTotalSupply
• annualInterestBips
• delinquencyFeeBips
• withdrawalBatchDuration
• reserveRatioBips
• delinquencyGracePeriod

Borrowers can deploy multiple markets and whitelist lenders for each market. Lenders receive market tokens based on the amount they deposit. Special protections are in place to prevent APR manipulation by borrowers. Withdrawals are carried from the reserve amount, established through the reserveRatio, pro-rated in case the amount in sufficient. Borrowers are considered delinquent if they fail to maintain the reserve ratio. There is a grace period in which the borrower needs to deposit funds to replenish the reserve. In case the borrower doesn't do that, they'll have to pay a penalty APR. The delinquency is tracked using a grace tracker that functions as a rolling limit (once delinquency has been cured within a market, the grace tracker will count back down to zero from whatever value it had reached),

Codebase Analysis

The codebase is well-structured and follows best practices for smart contract development. There are some comments attached that go hand in hand with the great documentation in facilitating a great understanding of the project. It would have been better if the protocol provided some form of flow charts, which could have been very helpful to someone with a limited time or background in auditing this type of protocols. I took the liberty of drafting one to get a better grasp of the project.

<img src="https://user-images.githubusercontent.com/95440897/277175194-7f25912d-b4ce-432c-9552-ab91a278d270.png" alt="A nice sketch representing contracts and functions">

Architecture Recommendations

The overall architecture is great. I was surprised to see critical protocol variables withdrawableFees, borrowableAssets, availableLiquidity based on IERC20(asset).balanceOf(address(this)) which can obviously be inflated by anyone. The project does this because transferring the underlying token to the market contract is the only way a borrower can repay his debt. I advise creating a function that can be used by the borrower to repay his debt and change the way totalAssets() is derived by tracking inflows and outflows from the authorized parties.

Moreover, the protocol should limit the tokens that can be used. It’s more cautious to ensure that borrowers aren’t asking for weird ERC20 tokens at protocol level by making a list of approved tokens (one that doesn’t include rebasing tokens for example).

Centralization Risks

The overall centralization risk is low. Indeed, the protocol vets every single user and can take back any permission at any moment, but the markets are not controlled by the protocol after deployment and they are not upgradable. The owner of the protocol doesn’t have access to the borrower’s/lender’s funds.

Systemic Risks

Even if this is out of scope and the protocol is fully aware of this, we cannot simply ignore the risk of having a bad borrower.

The protocol has an onboarding process that requires different documents from the borrowers, which will be assessed by lawyers and 3rd party services. The borrowers will have a process to vet the lenders, and both parties will sign a Service Agreement and at least one Master Loan Agreement.

1. People steal, scam and hack not because there isn’t a legal biding document that prevents them from doing it, the law establishes the illegality of those acts anyway, but people keep doing it because they don’t care. If the amount is large enough there’s always a chance a borrower will run with the money. Getting money back is always difficult.
2. In order to do this, borrowers will be inclined to provide fake documents. Given that the stealing opportunity is high, people will invest in high quality fakes in order to scam the protocol and lenders.
3. In the Master Loan Agreement (MLA) terminology definition from the documentation it is specified that MLA is offered by Wildcat as protection for lenders, those kinds of statements invite damaged lenders to sue the protocol for compensation when they realize that Master Loan Agreement didn’t quite protected their funds from getting stolen.

Recommendations

Consider an even more modular architecture, some of the functions are really big. Breaking them into smaller functions could help with the maintainability, readability and ease of building on top of the project.

Limit the number of tokens.

Create a repay method that can be called only by the lender to avoid using the current way of calculating totalAssets.

Conclusion

Overall Wildcat is a unique and innovative project that attempts to bring undercollateralized lending into the web3 space. It was a fun project to audit. I sincerely hope this shift from trustless to trustful will succeed proving everyone that trust between two parties still has a place in web3.

Time spent:

48 hours