The Wildcat Protocol - ZdravkoHr's results

Banking, but worse - a protocol for fixed-rate, undercollateralised credit facilities.

General Information

Platform: Code4rena

Start Date: 16/10/2023

Pot Size: $60,500 USDC

Total HM: 16

Participants: 131

Period: 10 days

Judge: 0xTheC0der

Total Solo HM: 3

Id: 296

League: ETH

Wildcat Protocol

Findings Distribution

Researcher Performance

Rank: 8/131

Findings: 7

Award: $671.15

QA:
grade-b
Analysis:
grade-a

🌟 Selected for report: 0

🚀 Solo Findings: 0

Lines of code

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

Vulnerability details

Impact

According to the gitbook a borrower can close one of his markets at any time, given that the market has enough funds available to pay for all scheduled withdrawals and all fees accrued. This functionality is implemented in the WildcatMarket.closeMarket():

  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();
    }
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
      // Transfer excess assets to borrower
      asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
    _writeState(state);
    emit MarketClosed(block.timestamp);
  }

The above function is protected by the onlyController modifier, making it possible only for the controller to call it.

  modifier onlyController() {
    if (msg.sender != controller) revert NotController();
    _;
  }

The WildcatMarketController, however, does not call this function ever. This makes closing markets impossible.

Proof of Concept

Run the following test in WildcatMarketController.t.sol:

    function testFailCloseMarket() public {
        MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18);
        vm.startPrank(parameters.borrower);
        asset.approve(address(controller), 20e18);

        MarketControllerParameters memory marketParameters = controllerFactory
            .getMarketControllerParameters();

        WildcatMarket newMarket = WildcatMarket(
            controller.deployMarket(
                address(mockDAI),
                "Market1",
                "M1",
                type(uint128).max,
                2500,
                marketParameters.minimumDelinquencyFeeBips,
                marketParameters.minimumWithdrawalBatchDuration,
                1000,
                3 days
            )
        );

        newMarket.closeMarket();
    }

Tools Used

Foundry

Protect the closeMarket function with onlyBorrower instead of onlyController:

- function closeMarket() external onlyController nonReentrant {
+ function closeMarket() external onlyBorrower nonReentrant {
   ...
  }

Assessed type

Access Control

#0 - c4-pre-sort

2023-10-27T07:18:18Z

minhquanym marked the issue as duplicate of #147

#1 - c4-judge

2023-11-07T13:53:20Z

MarioPoneder changed the severity to 2 (Med Risk)

#2 - c4-judge

2023-11-07T14:04:59Z

MarioPoneder marked the issue as partial-50

#3 - c4-judge

2023-11-07T14:16:53Z

MarioPoneder changed the severity to 3 (High Risk)

Lines of code

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

Vulnerability details

Impact

According to the gitbook a borrower can change the max total capacity of his markets. This functionality is implemented in the WildcatMarketConfig.setMaxTotalSupply():

  function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();

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

    state.maxTotalSupply = _maxTotalSupply.toUint128();
    _writeState(state);
    emit MaxTotalSupplyUpdated(_maxTotalSupply);
  }

The above function is protected by the onlyController modifier, making it possible only for the controller to call it.

  modifier onlyController() {
    if (msg.sender != controller) revert NotController();
    _;
  }

The WildcatMarketController, however, does not call this function ever. This makes changing capacities impossible.

Proof of Concept

Run the following test in WildcatMarketController.t.sol:

    function testFailCapacityChange() public {
        MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18);
        vm.startPrank(parameters.borrower);
        asset.approve(address(controller), 20e18);

        MarketControllerParameters memory marketParameters = controllerFactory
            .getMarketControllerParameters();

        WildcatMarket newMarket = WildcatMarket(
            controller.deployMarket(
                address(mockDAI),
                "Market1",
                "M1",
                type(uint128).max,
                2500,
                marketParameters.minimumDelinquencyFeeBips,
                marketParameters.minimumWithdrawalBatchDuration,
                1000,
                3 days
            )
        );

        newMarket.setMaxTotalSupply(newMarket.maxTotalSupply() + 1);
    }

Tools Used

Foundry

Protect the setMaxTotalSupply function with onlyBorrower instead of onlyController:

- function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {
+ function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyBorrower nonReentrant {
   ...
  }

Assessed type

Access Control

#0 - c4-pre-sort

2023-10-27T06:22:12Z

minhquanym marked the issue as duplicate of #162

#1 - c4-pre-sort

2023-10-27T06:58:24Z

minhquanym marked the issue as duplicate of #147

#2 - c4-judge

2023-11-07T13:52:15Z

MarioPoneder marked the issue as partial-50

#3 - c4-judge

2023-11-07T14:16:53Z

MarioPoneder changed the severity to 3 (High Risk)

Awards

6.5602 USDC - $6.56

Labels

bug
3 (High Risk)
partial-50
upgraded by judge
edited-by-warden
duplicate-266

External Links

Lines of code

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

Vulnerability details

Impact

In order for lenders to interact with a given market, they have to be authorized in the given market's controller. There are functions for authorizing and deauthorizing lenders. An _authorizedLenders EnumerableSets exists to keep track of authorized lenders.

The market uses different roles to give appropriate permissions to the users having them. One of them is the WithdrawOnly role. It is meant to be given to a deauthorized lender to stop him from future deposits. This is achieved by calling updateLenderAuthorization.

updateLenderAuthorization() calls WildcatMarketConfig.updateAccountAuthorization() with the lender and a flag as arguments. The flag is true if the lender is present in the set and false otherwise.

      WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));

WildcatMarketConfig.updateAccountAuthorization() then sets the account's role to AuthRole.DepositAndWithdraw if the flag is true, otherwise - to AuthRole.WithdrawOnly

    if (_isAuthorized) {
      account.approval = AuthRole.DepositAndWithdraw;
    } else {
      account.approval = AuthRole.WithdrawOnly;
    }

This is problematic because updateLenderAuthorization() can be called for any account by any account. This means that if it is called for an account that was never authorized in first place, that account will get the WithdrawOnly role and will be able to withdraw.

Proof of Concept

Run the following test in WildcatMarketController.t.sol

    function testUnauthorizedLender() public {
        MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18);
        address lender1 = address(0x1ed1);
        address unauthorizedLender = address(0x6afce2);
        uint256 initialAmount = 20e18;

        vm.startPrank(parameters.borrower);
        asset.approve(address(controller), initialAmount);

        MarketControllerParameters memory marketParameters = controllerFactory
            .getMarketControllerParameters();

        WildcatMarket newMarket = WildcatMarket(
            controller.deployMarket(
                address(mockDAI),
                "Market1",
                "M1",
                type(uint128).max,
                0,
                marketParameters.minimumDelinquencyFeeBips,
                marketParameters.minimumWithdrawalBatchDuration,
                1000,
                3 days
            )
        );

        address[] memory lenders = new address[](2);
        lenders[0] = lender1;
        controller.authorizeLenders(lenders);
        vm.stopPrank();

        uint256 lenderBalance = 10000e18;

        deal(address(mockDAI), lender1, lenderBalance);
        vm.startPrank(lender1);

        mockDAI.approve(address(newMarket), lenderBalance);

        newMarket.depositUpTo(lenderBalance);

        newMarket.transfer(unauthorizedLender, newMarket.balanceOf(lender1));

        vm.stopPrank();

        address[] memory markets = new address[](1);
        markets[0] = address(newMarket);

        vm.prank(unauthorizedLender);
        controller.updateLenderAuthorization(unauthorizedLender, markets);

        vm.prank(unauthorizedLender);
        newMarket.queueWithdrawal(9000e18);

        vm.warp(
            marketParameters.minimumWithdrawalBatchDuration + block.timestamp
        );
        newMarket.executeWithdrawal(
            unauthorizedLender,
            uint32(block.timestamp)
        );

        assertEq(mockDAI.balanceOf(unauthorizedLender), 9000e18);

    }

Tools Used

Foundry

My first thought was making updateLenderAuthorization internal and calling it each time a lender is authorized/deauthorized. However, this will make the deauthorization of blocked accounts impossible, since updateLenderAuthorization reverts if it accesses a blocked account.

Another possibility seemed like using low-level call to invoke the updateLenderAuthorization to avoid reverting. This is also not a solution. updateLenderAuthorization has to be public because otherwise unblocked lenders will not be able to get back their WithdrawOnly role in order to withraw their funds.

Therefore, my recommendation is to add another enumerable set that tracks the deauthorized lenders and in the updateLenderAuthorization call updateAccountAuthorization with:

  • true, if the account is present in the authorized lenders set
  • false, if the account is present in the deauthorized lenders set

Assessed type

Invalid Validation

#0 - c4-pre-sort

2023-10-27T08:40:21Z

minhquanym marked the issue as duplicate of #400

#1 - c4-pre-sort

2023-10-27T08:51:22Z

minhquanym marked the issue as duplicate of #155

#2 - c4-judge

2023-11-07T12:54:53Z

MarioPoneder marked the issue as satisfactory

#3 - c4-judge

2023-11-07T12:59:29Z

MarioPoneder marked the issue as selected for report

#4 - laurenceday

2023-11-09T09:44:59Z

Replica of https://github.com/code-423n4/2023-10-wildcat-findings/issues/266, a finding that has already been selected for the report as a High Risk. Mitigation in there.

#5 - MarioPoneder

2023-11-09T17:38:20Z

This finding and its duplicates do not discuss the impact of bypassing sanctions by abusing the vulnerability.
In contrast to #266 and its duplicates, therefore I agree with the pre-sort about two distinct findings having different severities.

Was selected for report due to PoC test case and overall discussion of the issue.

#6 - MiloTruck

2023-11-13T14:26:35Z

Hi @MarioPoneder, since this finding has identified the same root cause as #266, it should be a duplicate.

The proper way to handle a scenario where two issues have the same root cause, but one has failed to identify the highest severity impact is to duplicate all of them under one finding and award them with partial credit.

This is mentioned in the C4 documentation:

All issues which identify the same functional vulnerability will be considered duplicates regardless of effective rationalization of severity or exploit path.

However, any submissions which do not identify or effectively rationalize the top identified severity case may be judged as “partial credit” and may have their shares in that finding’s pie divided by 2 or 4 at judge’s sole discretion (e.g. 50% or 25% of the shares of a satisfactory submission in the duplicate set).

This has also been mentioned in the recent supreme court standardization:

Similar exploits under a single issue The findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.

Given the above, when similar exploits would demonstrate different impacts, the highest, most irreversible would be the one used for scoring the finding. Duplicates of the finding will be graded based on the achieved impact relative to the Submission Chosen for Report.

Reason being that having two distinct findings would essentially be rewarding the same bug twice in award calculation, making it unfair towards other findings.

This finding also has less duplicates than #266, so it could end up getting a higher payout even though it did not identify the highest severity impact.

#7 - MarioPoneder

2023-11-14T13:57:50Z

Thank you for the valuable input.
We already have 2 primary findings in this contents where some duplicates were awarded with partial credit.
In this case, the assessed impacts differ to an extent that even led to different severities of both primary findings, which seemed to justify their separation.

However, considering the fairness of the resulting reward distribution and the supreme court standardization, it's best to move forward with duplication and partial credit.

#8 - c4-judge

2023-11-14T13:59:03Z

MarioPoneder changed the severity to 3 (High Risk)

#9 - c4-judge

2023-11-14T14:02:43Z

MarioPoneder marked the issue as duplicate of #266

#10 - c4-judge

2023-11-14T14:03:14Z

MarioPoneder marked the issue as not selected for report

#11 - c4-judge

2023-11-14T14:03:22Z

MarioPoneder marked the issue as partial-50

Awards

6.6715 USDC - $6.67

Labels

bug
3 (High Risk)
satisfactory
duplicate-68

External Links

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L166-L169 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L172-L175 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L98

Vulnerability details

Impact

The protocol supports blacklisting of users. When a lender enters the blacklist, an escrow that holds his assets until whitelisting is created. Currently, there are 2 places in the code that create a new escrow - WildcatMarketBase._blockAccount() and WildcatMarketWithdrawals.executeWithdrawal(). The createEscrow function looks like this:

function createEscrow(address borrower, address account, address asset)

However, both _blockAccount and executeWithdrawal call it like this:

createEscrow(accountAddress, borrower, address(asset))

When a new escrow is created the borrower will be incorrectly set to the account and the account - to the borrower. Therefor, eligible lenders will never be able to get their funds back from the escrow.

Proof of Concept

  • Lender A makes a deposit of 1000 tokens.
  • Lender A queues a withdrawal.
  • By the time the withdrawal cycle ends, Lender A is already blacklisted.
  • An escrow with wrong arguments is created.
  • Now releaseEscrow can be called at any time, because the borrower is likely not blacklisted.
  • Finally, the assets will be sent to the borrower, not to the lender.

Tools Used

Manual Review

Fix the escrow creations:

- createEscrow(accountAddress, borrower, address(asset))
+ createEscrow(borrower, accountAddress, address(asset))

Assessed type

Error

#0 - c4-pre-sort

2023-10-27T02:24:55Z

minhquanym marked the issue as duplicate of #515

#1 - c4-judge

2023-11-07T11:50:06Z

MarioPoneder marked the issue as satisfactory

Findings Information

🌟 Selected for report: MiloTruck

Also found by: 0xCiphky, LokiThe5th, Madalad, Robert, ZdravkoHr, nonseodion

Labels

bug
2 (Med Risk)
satisfactory
sponsor confirmed
sufficient quality report
duplicate-499

Awards

218.5317 USDC - $218.53

External Links

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/18bda5daf6a668cfe5dd6c0749a908e193e7b90c/src/libraries/LibStoredInitCode.sol#L112-L115 https://github.com/code-423n4/2023-10-wildcat/blob/18bda5daf6a668cfe5dd6c0749a908e193e7b90c/src/libraries/LibStoredInitCode.sol#L92-L95

Vulnerability details

Impact

The LibStoreInitCode.sol library is used to deploy intermediate contracts that hold others' contracts initialization code in their runtime code. This feature is used in deployMarket:

LibStoredInitCode.create2WithStoredInitCode(marketInitCodeStorage, salt);

The issue is that the create2WithStoredInitCode function does not check if the creation of the new contract is successful.

assembly {
      let initCodePointer := mload(0x40)
      let initCodeSize := sub(extcodesize(initCodeStorage), 1)
      extcodecopy(initCodeStorage, initCodePointer, 1, initCodeSize)
      deployment := create2(value, initCodePointer, initCodeSize, salt)
}

This means that if the deployment fails, the supposed "market" will still be registered and even worse, the borrower will have to pay the protocol fee:

originationFeeAsset.safeTransferFrom(borrower, parameters.feeRecipient, originationFeeAmount);

A deployment can fail if:

  • the code in the constructor reverts. This is possible if the BIP settings were initially set to value greater than 100% which would allow a borrower to provide such a setting:
     if ((parameters.protocolFeeBips > 0).and(parameters.feeRecipient == address(0))) {
       revert FeeSetWithoutRecipient();
     }
     if (parameters.annualInterestBips > BIP) {
       revert InterestRateTooHigh();
     }
     if (parameters.reserveRatioBips > BIP) {
       revert ReserveRatioBipsTooHigh();
     }
     if (parameters.protocolFeeBips > BIP) {
       revert InterestFeeTooHigh();
     }
     if (parameters.delinquencyFeeBips > BIP) {
       revert PenaltyFeeTooHigh();
     }
  • the gas to execute the deployment was not enough.
  • the borrower provides an invalid token. In the above cases, he can call the function again (which will result in him paying one more time) and deploy the correct market. But in this case, he will not be able to do so. That will leave the unusable market in the registered markets which is not ideal.

Proof of Concept

A PoC with foundry using the third revert case. Add it in WildcatMarketController.t.sol

  function testUnsuccessfulMarketDeployment() public {
    uint256 initialAmount = 20e18;
    uint80 feeAmount = 1e18;
    address invalidToken = address(0xaf3141);

    deal(address(asset), parameters.borrower, initialAmount);
    vm.prank(archController.owner());
    controllerFactory.setProtocolFeeConfiguration(address(0xfee), address(asset), feeAmount, 1000);

    uint256 balanceBefore =  asset.balanceOf(parameters.borrower);
    vm.startPrank(parameters.borrower);
    asset.approve(address(controller), initialAmount);

    MarketControllerParameters memory marketParameters = controllerFactory.getMarketControllerParameters();
    address newMarket = controller.deployMarket(invalidToken, "Market1", "M1", 4000e18, 0, marketParameters.minimumDelinquencyFeeBips, marketParameters.minimumWithdrawalBatchDuration, marketParameters.minimumReserveRatioBips, marketParameters.minimumDelinquencyGracePeriod);

    uint256 codeSize;

    assembly {
      codeSize := extcodesize(newMarket)
    }

    assertEq(codeSize, 0);
    assertEq(asset.balanceOf(parameters.borrower), balanceBefore - feeAmount);

    vm.stopPrank();
  }

Tools Used

Foundry

After deployment, check its success:

if eq(extcodesize(deployment), 0) { 
        revert(0, 0)
}

Assessed type

Token-Transfer

#0 - c4-pre-sort

2023-10-27T04:39:27Z

minhquanym marked the issue as primary issue

#1 - c4-pre-sort

2023-10-27T04:45:18Z

minhquanym marked the issue as sufficient quality report

#2 - c4-sponsor

2023-10-31T21:35:40Z

laurenceday (sponsor) confirmed

#3 - c4-judge

2023-11-07T14:50:00Z

MarioPoneder marked the issue as satisfactory

#4 - c4-judge

2023-11-07T15:02:43Z

MarioPoneder marked issue #499 as primary and marked this issue as a duplicate of 499

Awards

16.6643 USDC - $16.66

Labels

bug
2 (Med Risk)
satisfactory
duplicate-196

External Links

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L468-L488

Vulnerability details

Impact

Upon market creation, the borrower specifies annual interest BIP that has to be in the range of MinimumAnnualInterestBips and MaximumAnnualInterestBips. After that the borrower is free to change the annual interest to whatever value he wants to using WildcatMarketController.setAnnualInterestBips():

  function setAnnualInterestBips(
    address market,
    uint16 annualInterestBips
  ) external virtual onlyBorrower onlyControlledMarket(market) {
    // If borrower is reducing the interest rate, increase the reserve
    // ratio for the next two weeks.
    if (annualInterestBips < WildcatMarket(market).annualInterestBips()) {
      TemporaryReserveRatio storage tmp = temporaryExcessReserveRatio[market];

      if (tmp.expiry == 0) {
        tmp.reserveRatioBips = uint128(WildcatMarket(market).reserveRatioBips());

        // Require 90% liquidity coverage for the next 2 weeks
        WildcatMarket(market).setReserveRatioBips(9000);
      }

      tmp.expiry = uint128(block.timestamp + 2 weeks);
    }

    WildcatMarket(market).setAnnualInterestBips(annualInterestBips);
  }

The function does not check if the provided value is between the minimum and maximum range. This allows the borrower to sets whatever interest he wants to. While there is a mechanism that will be fired when a borrower decreases the interest APR - the reserve ratio will be set to 90% for 2 weeks - I still believe this issue is of medium severity because it violates one of the main invariants stated on the contest's page:

Market parameters should never be able to exit the bounds defined by the controller which deployed it.

An example when things may go wrong is if a borrower makes a mistake and sets the interest to a really high value, causing the scaleFactor to increase very fast and in result not being able to pay for that market anymore.

Proof of Concept

Change the MinimumAnnualInterestBips to a value bellow 1000 (because there are other deployed test markets with an interest rate of 1000 or more) and run the following test in WildcatMarketController.t.sol:

    function testInterestChange() public {
        MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18);
        vm.startPrank(parameters.borrower);
        asset.approve(address(controller), 20e18);

        MarketControllerParameters memory marketParameters = controllerFactory
            .getMarketControllerParameters();

        WildcatMarket newMarket = WildcatMarket(
            controller.deployMarket(
                address(mockDAI),
                "Market1",
                "M1",
                type(uint128).max,
                2500,
                marketParameters.minimumDelinquencyFeeBips,
                marketParameters.minimumWithdrawalBatchDuration,
                1000,
                3 days
            )
        );

        uint16 minBIPS = controller
            .getParameterConstraints()
            .minimumAnnualInterestBips;

        controller.setAnnualInterestBips(
            address(newMarket),
            minBIPS - minBIPS / 2
        );
    }

Tools Used

Foundry

   function setAnnualInterestBips(
        address market,
        uint16 annualInterestBips
    ) external virtual onlyBorrower onlyControlledMarket(market) {
        // If borrower is reducing the interest rate, increase the reserve
        // ratio for the next two weeks.

 +       assertValueInRange(
 +           annualInterestBips,
 +           MinimumAnnualInterestBips,
 +           MaximumAnnualInterestBips,
 +           AnnualInterestBipsOutOfBounds.selector
 +       );

        if (annualInterestBips < WildcatMarket(market).annualInterestBips()) {
            TemporaryReserveRatio storage tmp = temporaryExcessReserveRatio[
                market
            ];

            if (tmp.expiry == 0) {
                tmp.reserveRatioBips = uint128(
                    WildcatMarket(market).reserveRatioBips()
                );

                // Require 90% liquidity coverage for the next 2 weeks
                WildcatMarket(market).setReserveRatioBips(9000);
            }

            tmp.expiry = uint128(block.timestamp + 2 weeks);
        }

        WildcatMarket(market).setAnnualInterestBips(annualInterestBips);
    }

Assessed type

Invalid Validation

#0 - c4-pre-sort

2023-10-27T14:20:39Z

minhquanym marked the issue as duplicate of #443

#1 - c4-judge

2023-11-07T12:27:00Z

MarioPoneder marked the issue as satisfactory

Awards

10.1663 USDC - $10.17

Labels

bug
downgraded by judge
grade-b
QA (Quality Assurance)
duplicate-76
Q-23

External Links

Lines of code

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

Vulnerability details

Impact

Currently, the way that borrowers must repay funds to the markets is by directly transferring funds to them. After that they have to call updateState. This 2-step process adds additional overhead. This is a problem if the borrower initiates 2 different transactions because the second may be stuck in the mempool, or the borrower may just forget to call updateState. If a borrower repays until the reserve ratio is met, but updateState() is not called and the market is not used for a long time, the period from the repayment to the next market interaction will be wrongly claimed to be delinquency period.

Proof of Concept

  1. Market A becomes delinquent at timestamp A.
  2. Borrower repays the needed assets at timestamp B, but updateState() is not called either because it was stuck for a long time in the mempool, or because the borrower forgot to call it. Market should not be delinquent anymore.
  3. Interaction to the market happens at timestamp C.
  4. Now the whole period from timestamp A to timestamp C will be delinquent.
  5. The borrower has to pay more than needed.

Tools Used

Manual Review, Foundry

Add a function which allows the borrower to repay his assets and after that calls updateState

Assessed type

Timing

#0 - c4-pre-sort

2023-10-28T11:41:42Z

minhquanym marked the issue as duplicate of #76

#1 - c4-judge

2023-11-08T16:37:04Z

MarioPoneder changed the severity to QA (Quality Assurance)

#2 - c4-judge

2023-11-09T15:09:56Z

MarioPoneder marked the issue as grade-b

Findings Information

Labels

analysis-advanced
grade-a
high quality report
sponsor acknowledged
A-03

Awards

412.5049 USDC - $412.50

External Links

Submitted Issues

2 Highs

DescriptionImpactLikelihood
Wrong argument ordering in createEscrowHIGHHIGH
Markets can never be closedHIGHHIGH

5 Mediums

DescriptionImpactLikelihood
CREATE2 success is not checkedMEDIUMLOW
Everyone can get the WithdrawOnly roleMEDIUMHIGH
Borrower can set market interest outside of the minimimum and maximum rangeMEDIUMHIGH
Borrower may have to pay more fee for delinquency than neededHIGHLOW
Max total supply cannot be changedMEDIUMHIGH

QA Report

DescriptionSeverity
Lender's deposit may be stuck in the mempoolLOW
Market token name missing spaceNC
Escrow address is not stored in WildcatMarketNC
Mistake in a commentNC

Gas Optimizations

Add an early return

Auditing approach

During this 10-days audit the codebase was tested in various different ways. The first step after cloning the repository was ensuring that all tests provided pass successfully when run. After that began the phase when I got some high-level overview of the functionality by reading the code. There were some parts of the project that caught my attention. These parts were tested with Foundry POCs. Some of them were false positives, others turned out to be true positives that were reported. The contest's main page does a wonderful job providing main invariants that should never be broken and ideas for exploits. The gitbook is also very good resource, even though some inconsistencies and wrong numbers were found there. These were fixed by the sponsors. I also had a look at the external contracts that the protocol integrates with, for example, OpenZeppelin's EnumerableSet.

Mechanism review

Wildcat is a protocol that lets borrowers take uncollateralized loans from approved lenders. It inverts the typical model of lending and borrowing in DEFI - borrowers create vaults and lenders deposit their funds in these vaults. Via reserve ratio percents, lenders' liquidity plays the role of their own collateral. In order for a borrower to be eligible to deploy a market he has to first sign a Service Agreement offchain.

The "grandparent" of the protocol is the ArchController contract. It handles registration of borrowers, controllers and factories. A borrower that is registered in the ArchController can then use a registered factory to deploy a controller. The factory holds the settings for the controller that when deployed, makes a call to it to fetch them.

Deployments overview

All deployments in the protocol happen through the LibStoredInitCode library. Let's have a look at an example of deploying a controller. The idea is that we deploy a contract which holds the controller's creation code prefixed by a STOP opcode (0x00) to avoid executing the code as a smart contract:

  • Call LibStoredInitCode.deployInitCode(data) with the creation code of the controller.
  • The function makes use of assembly to deploy a new contract. It uses the memory location [data, data + 32] (the length of the controller's creation code) to not waste additional memory space.
  • In this location we store a prefix code.
  • The prefix code looks like this: 0x6100005f81600a5f39f300 (total of 11 bytes)
  • Get the size of the controller's creation code and add 1 to it (because we will prepend a STOP opcode)
  • According to this EIP the creation code cannot exceed 2 bytes. For example's sake let's assume that the controller's size is 0xac. Then the size + 1 will be 0xad.
  • We shift the new size to the left by 64 bits (8 bytes).
  • This leaves us with ad000000000000000 (total of 9 bytes)
  • Apply bitwise OR to the prefix code and the new size to position the size between 61 and 5f
  • The new result is 0x6100ad5f81600a5f39f300 (total of 11 bytes)
  • This result gets saved in the memory slot [data, data + 32]
  • Since the EVM stores whole words in the memory (32 bytes), it will be prepended by 21 bytes of zeroes.
  • The next step is to create the contract create(0, add(data, 21), createSize). Here we are ignoring the 21 bytes added by the EVM and start loading from 0x6100ad5f81600a5f39f300. We load createSize bytes. createSize is the original size + 11 (because the prepended code is 11 bytes).
  • A new contract is created. Let's further break down the creation code of the newly created contract (0x6100ad5f81600a5f39f300 + the rest of the controller's creation code).
ByteOpcodeStack
0x61PUSH10x00ad
0x5fPUSH00x00

The next opcode is 0x39 (codecopy). It will copy a part of the init code and will store it in memory. It will store it in 0x00, will start copying from 0x0a (10) and will copy 0x00ad bytes. Lets take a look at the beginning of the init code again: 0x6100ad5f81600a5f39f300. Starting from 0x0a, this will copy the code from the end of the prefix code (which is 0x00, or the STOP) to the end of the controller's creation code. The code is now copied in memory and we have the following stack:

ByteOpcodeStack
0x61PUSH10x00ad
0x5fPUSH00x00

The next and final opcode is 0xf3 (return). We return 0x0ad bytes loading from 0x00. Which is the controller's initialization code prepended with a stop opcode. This is now the runtime code of the newly created contract.

  • This newly created contract can now be used to deploy the actual controller.

Deposits and withdrawals overview

When a borrower creates a controller, he then authorizes the lenders that he wants to interact with. He also deploys a market using this controller. Each market works with only one ERC20 token. A fee for creating a new market may be enabled.

The authorized lenders can deposit their ERC20 funds in a market they are approved for by calling either deposit or depositUpTo and specified amount.

  • The state of the market is updated. This happens every time a non-view function is called.
  • If the market is closed or the depositor tries to deposit an amount of funds that will go over the market capacity, the transaction will be reverted.
  • The specified amount of tokens are transferred from the lender to the market. This means that the lender has to approve the market to spend his tokens before the deposit.
  • The ledner's account's balance is incremented by the scaled amount of the assets for internal accounting. In other words, lender gets minted market tokens.
  • The new tokens are added to the market's balance.

Lenders can also withdraw funds that they have deposited. First, they have to queue a new withdrawal:

  • Withdrawals happen in batches depending on the specified withdrawal cycle length on deployment.
  • If they do not have at least the WithdrawOnly role, the transaction will be reverted.
  • Update the state and handle expired batch, if existent.
  • An amount of market token is transferred from the lender to the market.
  • If a batch already exists, add the withdrawal to it, otherwise create a new batch.
  • Check if there is some liquidity in the market to be transferred to the unclaimed rewards pool.
  • When tokens are transferred to the unclaimed rewards pool, they are burned from the total market supply.
  • The minimum of the available liquidity and the requested amount is transferred to the unclaimed rewards pool.
  • When the withdrwal's cycle period ends, the request has to be executed by calling executeWithdrawal.
  • If there are enough funds in the unclaimed rewards pool, then all the lenders that requested in this batch can be paid. Otherwise, they will receive the same % of assets of the unclaimed pool as their % in the batch. For example, if Lender A requests 2000 tokens and Lender B requests 8000 tokens, but in the unclaimed pool there are only 600 tokens, Lender A will receive 120 tokens and Lender B will receive the rest.
  • In case that the lender is sanctioned, the assets will be send to an escrow contract.
  • If a batch cannot be fully paid, it is added to a FIFO queue.
  • Later on, when the market's funds increase, these batches will be paid in the order they were added to the queue.

Borrowing overview

The main idea of the protocol is to let borrowers borrow. This happens when a eligible account calls the borrow function:

  • If the market is closed, the transaction will be reverted.
  • If the borrower tries to take a loan greater than the allowed amount, the transaction will be reverted. The allowed amount is determined by using a reserve ratio percent.
  • The specified amount is transferred from the market to the borrower

Interest and taxes overview

There are several interest and taxes mechanisms in the protocol:

  • base interest APR - this is the % which the lender has to pay annually on top of the lender's funds.
  • protocol fee - a % of the base interest that has to be paid annually to the protocol's fee recipient.
  • delinqency fee - a % of interest that must be paid to the lender while the market is in a delinquent state. Delinquent state means that the current supply of assets is less than the required liquidity. The market has a grace period in which a delinquent market's owner does not have to pay. After the grace period ends, if still delinquent, delinquency interest accrual starts.

The whole interest system is cleverly managed by using a scaleFactor in RAYs.

When a lender deposits funds, the amount deposited (called normal amount) is divided by that scaleFactor and the result is recorded as a scaledAmount. As time passes and interest accrues, the scaleFactor is increased. Then, when converting back to a normal amount, the scaledAmount is multiplied by the scaleFactor. This means that as more time passes the same scaledAmount will be equal to a larger and larger normal amount.

Other borrower permissions

  • Borrower can close the market whenever he decides to, given that it is not currently delinquent, or that it wont become delinquent. Lenders can still withdraw from a closed market, but they cannot deposit anymore. Borrowing from a closed market is prohibited.
  • Borrower can change the interest APR. This sounds problematic, but preventive actions are taken in this scenario. If a borrower wants to increase the APR, they are free to do so. On the other hand, if they decrease it, the reserve ratio of market is set to 90% for 2 weeks.
  • The borrower can adjust the max capacity of their markets.

Codebase quality

This codebase's quality is good even though some vulnerabilities were found. It makes use of clever techniques for deploying contracts and internal balance tracking. The code's tests have a good coverage too. However, they are somehow convoluted and hard to be understand at first.

Architecture recommendations

During my analysis of the codebase, I thought some changes may be done to improve the protocol:

  1. Stop interest accrual for sanctioned lenders - the protocol uses Chainalysis oracle to check if users are sanctioned. If a user is sanctioned, he gets blocked from Wildcat. This means that all his market tokens are sent to an escrow created specially for him. When the sanction is lifted off, the user is free to take back all of his assets. Because the escrow holds market tokens, this means that sanctioned users benefit from interest on their assets just like any other lender. Initially, I though that was a finding but @d1llon explained that its intended to be like that. I continued discussing thing issue with @functionZer0. One of the main reasons for this decision is that possible solution for stopping interest accrual would be to force the lender to fully exit the market. However, there may be some cases where the market is not liquid enough for such operation. I think it's still possible to achieve this goal without making the lender exit his position. This will require modifying the escrow a bit. It can remember the normalized amount that was sent to it and at the time of withdrawal - adjust the scaled amount accordingly.
  2. Store the escrows' addresses in the market - currently, when an escrow is created, assets are sent to it but its address is not saved anywhere. This will probably be handled by a frontend, but still it makes querying its address a lot more harder. I would suggest storing the addresses in a mapping(address user => address escrow).
  3. Make use of sanctioned user's funds - realistically, if a user gets in the sanctions list its very unlikely that he will get out of it. Of course, the possibility of the oracle becoming malicious exists and that is why the sanctions overrides are implemented. I suggest adding a long period of time (10 years or something like that) where if the lender has not been unsanctioned during it, its funds get distributed to the current lenders depending on the portion of the whole market's balance that they hold. This utilization of idle funds will be a good incentives for lenders to both follow the rules and provide liquidity.
  4. Create functions for some actions that call updateState() - the market keeps important information in a struct called state. This state holds the interest, capacity, delinquency and other settings. In the current implementation, a function for returning borrowed assets does not exist. This means that the borrower has to do a direct ERC20 transfer in order to pay his debts. After transferring, updateState has to be called. If it is not, some problems may arise. This 2-step payment makes it harder for the borrower. There may be a situation when the second transaction gets stuck in the mempool and the state is not updated. Consider adding functions that call updateState() after the given operation.

Centralization risks

There are some centralization risks. For example, if the account of the archcontroller's owner gets compromised, a lot of things may go wrong. Protocol fee may be set to a too high one, and the fee recipient to a foreign entity. This will cause a huge lose of money.

There also exists the risk of a borrower losing his private key. The developers have thought of that scenario letting other accounts pay borrower's debt on his behalf by just transferring tokens. This may be used by the same borrower that lost his private keys.

Systematic risks

  1. The Chainalysis oracle becoming malicious may cause a discord. In the case of wrongly blocked lender, the borrower can just override the sanction himself. However, the interval between the sanction and the overriding is still bad experience for the lender that may have wanted to interact with a market.
  2. If the borrowers decides to misbehave, he may choose to not pay his debts. This situation would be solved offchain with the proper legislation, but such an action can undermine the reputation of the protocol. Another faulty action of the borrower would be removing sanctions of correctly blocked lenders.
  3. Of course, the possibility of exploiting the system through a vulnerability in external integrations, such as dependency contracts, always exists.

Time spent:

50 hours

#0 - c4-pre-sort

2023-10-29T15:04:01Z

minhquanym marked the issue as high quality report

#1 - laurenceday

2023-11-06T10:03:07Z

Enjoyed this one!

#2 - c4-sponsor

2023-11-06T10:03:12Z

laurenceday (sponsor) acknowledged

#3 - c4-judge

2023-11-09T12:23:28Z

MarioPoneder marked the issue as grade-b

#4 - c4-judge

2023-11-09T17:15:35Z

MarioPoneder marked the issue as grade-a

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter