Inconsistent Behavior in Temporary Reserve Ratio Calculation After Interest Rate Changes

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/MarketConstraintHooks.sol#L227

Vulnerability details

Proof of Concept

The onSetAnnualInterestAndReserveRatioBips function in the MarketConstraintHooks contract exhibits inconsistent behavior when handling interest rate changes after the expiry of an initial temporary reserve ratio period. This inconsistency can lead to unexpected reserve ratio calculations

Two scenarios demonstrate this issue: Scenario 1: Further lowering the interest rate after expiry

if (tmp.expiry > 0) {
  bool canExpire = (annualInterestBips >= intermediateState.annualInterestBips).and(
    block.timestamp >= tmp.expiry
  );
  bool canCancel = annualInterestBips >= tmp.originalAnnualInterestBips;
  if (canExpire.or(canCancel)) {
    // This block is not entered if interest rate is lowered
  }
}

// Original values from 2 weeks ago are used
(uint16 originalAnnualInterestBips, uint16 originalReserveRatioBips) = (
  tmp.originalAnnualInterestBips,
  tmp.originalReserveRatioBips
);

uint16 temporaryReserveRatioBips = _calculateTemporaryReserveRatioBips(
  annualInterestBips,
  originalAnnualInterestBips,
  originalReserveRatioBips
);

In this scenario, when the interest rate is lowered further, the function uses the original values from when the temporary reserve ratio was first set (potentially 2 weeks ago) to calculate the new temporary reserve ratio. This could result in a higher reserve ratio than current market conditions might warrant.

Scenario 2: Slightly increasing then lowering the interest rate after expiry Step 1 (slight increase):

if (tmp.expiry > 0) {
  bool canExpire = (annualInterestBips >= intermediateState.annualInterestBips).and(
    block.timestamp >= tmp.expiry
  );
  bool canCancel = annualInterestBips >= tmp.originalAnnualInterestBips;
  if (canExpire.or(canCancel)) {
    // This block is entered due to the slight increase
    delete temporaryExcessReserveRatio[market];
    return (newAnnualInterestBips, tmp.originalReserveRatioBips);
  }
}

Step 2 (subsequent decrease):

// tmp.expiry is now 0 due to previous deletion
(uint16 originalAnnualInterestBips, uint16 originalReserveRatioBips) = (
  intermediateState.annualInterestBips,
  intermediateState.reserveRatioBips
);

uint16 temporaryReserveRatioBips = _calculateTemporaryReserveRatioBips(
  annualInterestBips,
  originalAnnualInterestBips,
  originalReserveRatioBips
);

In this scenario, the slight increase causes the temporary excess reserve ratio to be deleted. When the interest rate is subsequently lowered, it's treated as a new case, using current levels for calculations instead of the values from 2 weeks ago.

This inconsistency can lead to:

1. Different reserve ratios for the same end state, depending on the path taken.
2. Potential for manipulation by borrowers who could exploit this behavior.
3. Unintuitive and unpredictable system responses to interest rate changes.

Recommended Mitigation Steps

temporaryExcessReserveRatio should be removed after expiry and a fresh one initiated if needed.

Assessed type

Other

[laurenceday]

We wouldn't consider this a medium as it doesn't impact the functionality of the protocol, it's just a minor inconvenience in that borrowers need to handle resetting the reserve ratio upon expiry.

See here: https://github.com/code-423n4/2024-08-wildcat-findings/issues/46#issuecomment-2367760017 - this issue is effectively a duplicate.

That said, in retrospect it's good to have pointed out and worth fixing, so probably best to just merge them into one finding.

Would happily accept as QA, but leaving severity decision to the judge: disputing on that basis alone rather than validity.

[c4-judge]

3docSec marked the issue as duplicate of #46

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b