The Wildcat Protocol - 0xbepresent's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/MarketState.sol#L138 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/FeeMath.sol#L89

Vulnerability details

The borrower does not pay all debts when he closes the market

Lines of code

• https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142
• https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/MarketState.sol#L138
• https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/FeeMath.sol#L89

Impact

When borrower closes the market using the WildMarket::closeMarket(), he must pay all debts:

File: WildcatMarket.sol
142:   function closeMarket() external onlyController nonReentrant {
...
...
151:     uint256 totalDebts = state.totalDebts();
152:     if (currentlyHeld < totalDebts) {
153:       // Transfer remaining debts from borrower
154:       asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
155:     } else if (currentlyHeld > totalDebts) {
...
...

The totalDebts() function calculates how much the borrower needs to pay back to the market. The problem is that the function does not count all the incurring debt when the borrower was in delinquent process. The penalty is increased when the user is delinquent code line 97-111 and the penalty is being decreased when the time elapsed code line 115.

File: FeeMath.sol
089:   function updateTimeDelinquentAndGetPenaltyTime(
090:     MarketState memory state,
091:     uint256 delinquencyGracePeriod,
092:     uint256 timeDelta
093:   ) internal pure returns (uint256 /* timeWithPenalty */) {
094:     // Seconds in delinquency at last update
095:     uint256 previousTimeDelinquent = state.timeDelinquent;
096: 
097:     if (state.isDelinquent) {
098:       // Since the borrower is still delinquent, increase the total
099:       // time in delinquency by the time elapsed.
100:       state.timeDelinquent = (previousTimeDelinquent + timeDelta).toUint32();
101: 
102:       // Calculate the number of seconds the borrower had remaining
103:       // in the grace period.
104:       uint256 secondsRemainingWithoutPenalty = delinquencyGracePeriod.satSub(
105:         previousTimeDelinquent
106:       );
107: 
108:       // Penalties apply for the number of seconds the market spent in
109:       // delinquency outside of the grace period since the last update.
110:       return timeDelta.satSub(secondsRemainingWithoutPenalty);
111:     }
112: 
113:     // Reduce the total time in delinquency by the time elapsed, stopping
114:     // when it reaches zero.
115:     state.timeDelinquent = previousTimeDelinquent.satSub(timeDelta).toUint32();
116: 
117:     // Calculate the number of seconds the old timeDelinquent had remaining
118:     // outside the grace period, or zero if it was already in the grace period.
119:     uint256 secondsRemainingWithPenalty = previousTimeDelinquent.satSub(delinquencyGracePeriod);
120: 
121:     // Only apply penalties for the remaining time outside of the grace period.
122:     return MathUtils.min(secondsRemainingWithPenalty, timeDelta);
123:   }

The borrower will not pay all the debt incurred while he was in delinquent mode when borrower closes the market.

Proof of Concept

Please consider the next scenario taking in consideration the example from the documentation:

Example:

• A market with a 5 day grace period becomes delinquent for the first time, and the grace tracker begins counting up from zero.
• The borrower takes 7 days to cure the market of its delinquency.
• Once the delinquency is cured, the market calculates that 2 days of penalty APR must be applied and adjusts the scaling factor of market tokens accordingly.
• The grace tracker counts back down to zero from this point - subsequent market state updates will detect when the tracker drops below the market grace period and adjust scaling factors appropriately.
• A total of 4 days of penalty APR will be applied in total: the failure of a market to update its state in a timely fashion as the tracker drops below the grace period does not adversely impact the borrower.

Now adapting the example above to the next scenario:

1. Borrower decides to close the market on day 7 and he only pays the delinquent penalty for 2 days and the time left (the 2 days left) is not counted in the total debt.
2. Borrower only pay penalty for 2 days instead of 4 days.

Tools used

Manual review

Recommended Mitigation Steps

When borrower closes the market, consider to calculate all the debt when the borrower was in delinquency.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L64 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L33 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L74

Vulnerability details

Impact

The user can be marked as blocked using the WildcatMarketConfig::nukeFromOrbit() function, then funds will be sent to the escrow contract. The sanctioned user needs to be being unsanctioned in order to be able to claim the assets.

The problem is that the lender who is going to be marked as blocked can transfer his assets to another collude lender, avoiding the funds to be sent to the escrow contract therefore avoiding the assets to be locked until the user is unsanctioned. Consider the next scenario:

1. LenderA is sanctioned and the WildcatMarketConfig::nukeFromOrbit() function is called.
2. LenderA sees the transaction and he transfers his assets to a collude lender before the execution of the step 1 (nukeFromOrbit()).
3. Now LenderA is blocked and nothing is sent to the escrow contract.
4. The colluded lender can help to withdraw the LenderA funds from the market.

Proof of Concept

I created a test where Alice (Lender) transfer his funds to the collude lender (Bob) before the WildcatMarketConfig::nukeFromOrbit() function is called, therefore the lender Bob can get the Alice assets (sanctioned user). Test steps:

1. Lender Alice deposit 1e18
2. Alice is sanctioned and she transfer his assets to another collude lender (Bob) before the nukeFromOrbit() is called.
3. The nukeFromOrbit() is called and the escrow contract is not created since the sanctioned lender Alice has not funds because she transferred the assets in the step 2.
4. The collude lender (bob) can execute the withdrawal and he gets the alice's assets.

// File: test/market/WildcatMarketWithdrawals.t.sol:WithdrawalsTest
// $ forge test --match-test "test_SanctionedUser_transferAssetsToAnotherLender" -vvv
//
  function test_SanctionedUser_transferAssetsToAnotherLender() external {
    // Lender, who is going to be blocked (sanctioned) can transfer his assets to a collude lender in order to avoid the 
    // escrow restrictions.
    //
    _authorizeLender(bob);
    uint256 previousBalanceBob = asset.balanceOf(bob);
    // 
    // 1. Lender Alice deposit 1e18
    _deposit(alice, 1e18);
    assertEq(market.scaledBalanceOf(alice), 1e18);
    assertEq(market.scaledBalanceOf(bob), 0);
    //
    // 2. Alice is sanctioned and she transfer his assets to another collude lender (Bob)
    // before the nukeFromOrbit() is called.
    sanctionsSentinel.sanction(alice);
    vm.prank(alice);
    market.transfer(bob, 1e18);
    assertEq(market.scaledBalanceOf(alice), 0);
    assertEq(market.scaledBalanceOf(bob), 1e18);
    //
    // 3. The nukeFromOrbit() is called and the escrow contract is not created.
    market.nukeFromOrbit(alice);
    //
    // 4. The collude lender (bob) can execute the withdrawal and he gets the alice's assets
    _requestWithdrawal(bob, 1e18);
    fastForward(parameters.withdrawalBatchDuration);
    market.executeWithdrawal(bob, uint32(block.timestamp));
    assertEq(market.scaledBalanceOf(alice), 0);
    assertEq(market.scaledBalanceOf(bob), 0);
    assertEq(previousBalanceBob + 1e18, asset.balanceOf(bob));  // Bob has the alice's assets
  }

Tools used

Manual review

Recommended Mitigation Steps

Validates in the transfer function if the from address is sanctioned:

  function _transfer(address from, address to, uint256 amount) internal virtual {
    MarketState memory state = _getUpdatedState();
    uint104 scaledAmount = state.scaleAmount(amount).toUint104();

    if (scaledAmount == 0) {
      revert NullTransferAmount();
    }

++  if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, from)) {
++    revert TheFromAddressIsSactioned();
++  }

    Account memory fromAccount = _getAccount(from);
    fromAccount.scaledBalance -= scaledAmount;
    _accounts[from] = fromAccount;

    Account memory toAccount = _getAccount(to);
    toAccount.scaledBalance += scaledAmount;
    _accounts[to] = toAccount;

    _writeState(state);
    emit Transfer(from, to, amount);
  }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Once the sanctioned lender is being unsanctioned, the lender assets are transferred to the borrower instead of the lender, causing the lost of lender's assets

Lines of code

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

Impact

If the lender is sanctioned while there is a withdrawal process, the assets are transferred to the escrow contract in the code line 166-171:

File: WildcatMarketWithdrawals.sol
137:   function executeWithdrawal(
138:     address accountAddress,
139:     uint32 expiry
140:   ) external nonReentrant returns (uint256) {
...
...
163: 
164:     if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
165:       _blockAccount(state, accountAddress);
166:       address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
167:         accountAddress,
168:         borrower,
169:         address(asset)
170:       );
171:       asset.safeTransfer(escrow, normalizedAmountWithdrawn);
...
...
188:   }

The assets can be claimed using the WildcatSanctionsEscrow::releaseEscrow() function once the sanctioned lender is being unsanctioned. The problem is that the parameters are not correctly ordered when the createEscrow() function is called.

The escrow contract is called using the parameters order createEscrow(accountAddress, borrower, address(asset)):

File: WildcatMarketWithdrawals.sol
166:       address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
167:         accountAddress,
168:         borrower,
169:         address(asset)
170:       );

But the WildcatSanctionsSentinel::createEscrow() function has a different parameters order borrower, account, asset:

File: WildcatSanctionsSentinel.sol
95:   function createEscrow(
96:     address borrower,
97:     address account,
98:     address asset
99:   ) public override returns (address escrowContract) {

The account and borrower parameters are inversed. This behaivour causes that once the sanctioned lender is being unsanctioned, the assets will be transferred to the borrower instead the lender. Lender's assets will be lost.

Proof of Concept

I created a test where Alice (lender) is sanctioned and his assets are transferred to the escrow contract. Then Alice is being unsanctioned and Alice assets are transferred to the borrower instead Alice (lender):

import { IWildcatSanctionsEscrow } from '../../src/interfaces/IWildcatSanctionsEscrow.sol';
//
// File: test/market/WildcatMarketWithdrawals.t.sol:WithdrawalsTest
// $ forge test --match-test "test_executeWithdrawal_Sanctioned_ParamsAreNotCorrectlyOrdered"
//
  function test_executeWithdrawal_Sanctioned_ParamsAreNotCorrectlyOrdered() external {
    // Once the sanctioned lender is being unsanctioned, the lender balance is transferred to the borrower instead of the lender.
    // The problem is that the function params are not correctly ordered.
    // 
    _deposit(alice, 1e18);
    _requestWithdrawal(alice, 1e18);
    fastForward(parameters.withdrawalBatchDuration);
    uint256 previousBalanceAlice = asset.balanceOf(alice);
    uint256 previousBalanceBorrower = asset.balanceOf(borrower);
    //
    // 1. Alice is sanctioned and his balance is transferred to the escrow contract
    sanctionsSentinel.sanction(alice);
    address escrow = sanctionsSentinel.getEscrowAddress(alice, borrower, address(asset));
    market.executeWithdrawal(alice, uint32(block.timestamp));
    assertEq(IWildcatSanctionsEscrow(escrow).balance(), 1e18);
    //
    // 2. Alice is unsanctioned and Alice is able to call releaseEscrow() but assets are transferred
    // to the borrower instead alice
    sanctionsSentinel.unsanction(alice);
    IWildcatSanctionsEscrow(escrow).releaseEscrow();
    assertEq(previousBalanceAlice, asset.balanceOf(alice));
    assertEq(previousBalanceBorrower + 1e18, asset.balanceOf(borrower));
    assertEq(IWildcatSanctionsEscrow(escrow).balance(), 0);
  }

I added the next function to the test/helpers/MockSanctionsSentinel.sol file:

--- a/test/helpers/MockSanctionsSentinel.sol
+++ b/test/helpers/MockSanctionsSentinel.sol
@@ -14,4 +14,8 @@ contract MockSanctionsSentinel is WildcatSanctionsSentinel {
   function sanction(address account) external {
     MockChainalysis(chainalysisSanctionsList).sanction(account);
   }
+
+  function unsanction(address account) external {
+    MockChainalysis(chainalysisSanctionsList).unsanction(account);
+  }
 }

Tools used

Manual review

Recommended Mitigation Steps

Please correct the order of the parameters in the WildcatMarketWithdrawals::executeWithdrawal()#166 and WildcatMarketBase::_blockAccount()#172

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

When the market is deployed, the annualInterestBips is validated that the value is in range of MinimumAnnualInterestBips and MaximumAnnualInterestBips.

File: WildcatMarketController.sol
394:   function enforceParameterConstraints(
395:     string memory namePrefix,
396:     string memory symbolPrefix,
397:     uint16 annualInterestBips,
398:     uint16 delinquencyFeeBips,
399:     uint32 withdrawalBatchDuration,
400:     uint16 reserveRatioBips,
401:     uint32 delinquencyGracePeriod
402:   ) internal view virtual {
...
...
410:     assertValueInRange(
411:       annualInterestBips,
412:       MinimumAnnualInterestBips,
413:       MaximumAnnualInterestBips,
414:       AnnualInterestBipsOutOfBounds.selector
415:     );

The problem is the validation is not executed when the borrower changes the annualInterestBips using setAnnualInterestBips() function. The borrower can deploy a market using a validated annualInterestBips then changes it to a non validated using setAnnualInterestBips(). The controller may have a certain MinimumAnnualInterestBips value which can by bypassed.

Proof of Concept

I created a test where the Controller.MinimumAnnualInterestBips is 500 and the borrower calls setAnnualInterestBips() function using 0 as the annualInterestBips. The Controller.MinimumAnnualInterestBips is bypassed:

// File: test/market/WildcatMarket.t.sol:WildcatMarketTest
// $ forge test --match-test "test_BorrowerCanEvadeTheAnnualInterestBipsRestriction" -vvv
//
  function test_BorrowerCanEvadeTheAnnualInterestBipsRestriction() external asAccount(address(controller)) {
    //
    // 1. Alice deposits 1e18
    _deposit(alice, 1e18);
    //
    // 2. Assert the market.annualInterestBips() is 1000 and the MinimumAnnualInterestBips is 500
    assertEq(market.annualInterestBips(), 1000);
    MarketParameterConstraints memory constraints = controller.getParameterConstraints();
    assertEq(constraints.minimumAnnualInterestBips, 500);
    //
    // 3. Borrower calls setAnnualInterestBips() to 0 bps. Assert the new market.annualInterestBips() is 0. The constraints.minimumAnnualInterestBips is bypassed
    startPrank(borrower);
    controller.setAnnualInterestBips(address(market), 0);
    assertEq(market.annualInterestBips(), 0);
    assertEq(constraints.minimumAnnualInterestBips, 500);
  }

I modified the TestConstants.sol file in order to set the Controller.MinimumAnnualInterestBips to 500:

--- a/test/shared/TestConstants.sol
+++ b/test/shared/TestConstants.sol
@@ -26,5 +26,5 @@ uint16 constant MaximumDelinquencyFeeBips = 10_000;
 uint32 constant MinimumWithdrawalBatchDuration = 0;
 uint32 constant MaximumWithdrawalBatchDuration = 365 days;
 
-uint16 constant MinimumAnnualInterestBips = 0;
+uint16 constant MinimumAnnualInterestBips = 500;
 uint16 constant MaximumAnnualInterestBips = 10_000;

Tools used

Manual review

Recommended Mitigation Steps

Validates the new annualInterestBips is in range MinimumAnnualInterestBips-MaximumAnnualInterestBips in the setAnnualInterestBips() function:

  function setAnnualInterestBips(
    address market,
    uint16 annualInterestBips
  ) external virtual onlyBorrower onlyControlledMarket(market) {
++  assertValueInRange(
++    annualInterestBips,
++    MinimumAnnualInterestBips,
++    MaximumAnnualInterestBips,
++    AnnualInterestBipsOutOfBounds.selector
++  );
    // 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);
  }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Once the market is closed the reserveRatioBips is set to zero value.

File: WildcatMarket.sol
142:   function closeMarket() external onlyController nonReentrant {
...
...
146:     state.reserveRatioBips = 0;
...
...

The problem is that there could be a temporaryExcessReserveRatio[market] in progress, returning back the state.reserveRatioBips to a non zero value causing that the liquidityRequired() to be positive because there is a non zero reserve ratio. This behaivour will cause that the borrower can be in deliquency even when the market is closed.

Proof of Concept

Please consider the next scenario:

1. Borrower calls setAnnualInterestBips() and the tmp.reserveRatioBips is 100.
2. Borrower closes the market and reserveRatioBps is zero.
3. After 2 weeks the function resetReserveRatio() returns the value to a non zero (100).

Tools used

Manual review

Recommended Mitigation Steps

Add a restriction in the resetReserveRatio() function that it could not be called when the market is closed.

Assessed type

Access Control