The reserveRatioBips will be incorrect if a market APR is reduced again two weeks after the initial reduction of over 25%

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/MarketConstraintHooks.sol#L204-L288

Vulnerability details

Impact

The reserveRatioBips will be incorrect if a market APR is reduced again two weeks after the initial reduction of over 25%. The borrower of the market has to repay more assets to ensure the market is not delinquent.

Proof of Concept

The Wildcat protocol specifies that a borrower can reduce the APR of their market as follows:

The interest rate on a market is fixed at any given point in time (i.e. markets do not make use of a utilisation-rate based curve), however the borrower is free to adjust this rate step-wise should they wish, under the following formula:

Should a borrower wish to increase the APR of a market in order to encourage additional deposits, they are able to do so without constraint.

Should they wish to decrease the APR, they are able to do so by up to 25% of the current APR in a given two week period: a decrease of more than this requires that twice the amount is returned to the market reserves for that two week period to permit lenders to opt out ('ragequit') if they choose.

To illustrate:

A borrower can reduce a market APR from 10% to 7.5% with no penalty, and two weeks thereafter will be able to reduce it again to 5.625%, and so on.

However, should a borrower reduce a market APR from 10% to 7.4% (a 26% reduction), they will be required to return 52% of the outstanding supply to the market for two weeks. After that time has passed, the reserve ratio will drop back to the prior level and the assets can be borrowed again.

Note that the above only applies if your market is in an 'open-term' setting: i.e. there is no hook enabled which is preventing withdrawals at the time of the proposed change. If this is the case, you will not be able to reduce the APR while that hook is active (otherwise that enables a fairly obvious rug mechanic).

A borrower can reduce the market APR by up to 25% first, and two weeks later, the borrower can reduce it no more than 25% again with no penalty.
However, it a borrower reduces the market APR by more than 25% first, the borrower can not reduce it no more than 25% with no penalty after two weeks.

Copy below codes to WildcatMarket.t.sol and run forge test --match-test test_setAnnualInterestAndReserveRatioBips_RestoreReserveRatioFail:

  function test_setAnnualInterestAndReserveRatioBips_RestoreReserveRatioFail() external {
    //@audit-info current annualInterestBips is 10%
    assertEq(market.annualInterestBips(), 1000);
    //@audit-info current reserveRatioBips is 20%
    assertEq(market.reserveRatioBips(), 2000);

    //@audit-info alice deposits 50K
    vm.prank(alice);
    market.depositUpTo(50_000e18);
    //@borrower sets annualInterestBips from 10% to 7% (30% reducing)
    vm.prank(borrower);
    market.setAnnualInterestAndReserveRatioBips(700,0);

    //@audit-info current annualInterestBips is 7%
    assertEq(market.annualInterestBips(), 700);
    //@audit-info current reserveRatioBips is 60% (30% * 2)
    assertEq(market.reserveRatioBips(), 6000);
    fastForward(2 weeks);
    //@borrower sets annualInterestBips from 7% to 6% (14.3% reducing)
    vm.prank(borrower);
    market.setAnnualInterestAndReserveRatioBips(600,0);
    //@audit-info current annualInterestBips is 6%
    assertEq(market.annualInterestBips(), 600);
    //@audit-info current reserveRatioBips is 80% instead of restoring to the original value (20%)
    assertEq(market.reserveRatioBips(), 8000);
  }

As we can see, the first APR reduction(10% -> 7%) is expired, and the second APR reduction (7% -> 6%) is no more than 25%. The reserveRatioBips should be restored to 20%, however it is wrongly set to 80%.

Tools Used

Manual review

Recommended Mitigation Steps

When a market APR has been reduced more than 25% first

• If the market APR is reduced again no more than 25% after two weeks, the reserveRatioBips should be reset to the original reserve ratio
• If the market APR is reduced again more than 25% after two weeks, the reserveRatioBips should be updated to twice the new APR reduction rate (accounted from the previous reduced APR)

To illustrate:
Initial annualInterestBips is 10%, reserveRatioBips is 20%.
annualInterestBips is reduced to 7%, now reserveRatioBips is 60%

• two weeks later, annualInterestBips can be reduced to 6% and reserveRatioBips is restored to 20%($\frac{0.07 - 0.06}{0.07} < 0.25$)
• or two weeks later, annualInterestBips can be reduced to 4.2% and reserveRatioBips is set to 80% ($\frac{0.07 - 0.042}{0.07} * 2$)

However, it seems impossible to fix this issue in the current logic by slight improvement.

Since Reducing APR logic works for all market and it is not tied to any specific hook, It is recommended to move all codes in MarketConstraintHooks#onSetAnnualInterestAndReserveRatioBips() to WildcatMarketConfig.sol, and modify codes to mitigate this issue based on above suggestions.

Assessed type

Context

[laurenceday]

Not an issue.

Wildcat V1 had a function called resetReserveRatio which deleted the temporary reserve ratio struct if the two week expiry had elapsed:

  function resetReserveRatio(address market) external virtual override sphereXGuardExternal {
    TemporaryReserveRatio memory tmp = temporaryExcessReserveRatio[market];
    if (tmp.expiry == 0) {
      revertWithSelector(AprChangeNotPending.selector);
    }
    if (block.timestamp < tmp.expiry) {
      revertWithSelector(ExcessReserveRatioStillActive.selector);
    }

    emit TemporaryExcessReserveRatioExpired(market);
    WildcatMarket(market).setReserveRatioBips(uint256(tmp.originalReserveRatioBips).toUint16());
    delete temporaryExcessReserveRatio[market];
  }

This functionality has been brought into Wildcat V2 within the onSetAnnualInterestAndReserveRatioBips hook:

    // Get the existing temporary reserve ratio from storage, if any
    TemporaryReserveRatio memory tmp = temporaryExcessReserveRatio[market];

    if (tmp.expiry > 0) {
      bool canExpire = (annualInterestBips >= intermediateState.annualInterestBips).and(
        block.timestamp >= tmp.expiry
      );
      bool canCancel = annualInterestBips >= tmp.originalAnnualInterestBips;
      if (canExpire.or(canCancel)) {
        // If the update period has expired and the provided value doesn't reduce it further,
        // or it is not expired but the new value undoes the reduction for the current update
        // period, reset the temporary reserve ratio.
        if (canExpire) {
          emit TemporaryExcessReserveRatioExpired(market);
        } else {
          emit TemporaryExcessReserveRatioCanceled(market);
        }
        delete temporaryExcessReserveRatio[market];
        return (newAnnualInterestBips, tmp.originalReserveRatioBips);
      }

A borrower has always had to hit a reset button after expiry in order to delete the temporary limit prior to performing another APR update: in Wildcat V2 this simply corresponds to calling the setAnnualInterestAndReserveRatioBips function again with the same APR value as is currently in place.

i.e. in the example above, calling it with 7% two weeks after reducing it to 7% will delete the struct and permit another adjustment to 6% without penalty - isomorphic to just calling reset. Means we don't have to carry around another function.

[3docSec]

The current implementation seems quite error prone with the borrower having to trigger another action to have the market functioning properly in this case I admit. However because the protocol works with KYC'ed borrowers, it is safe to assume that they are well-informed on how they are supposed to interact with the protocol -> Low

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b