Usury from simple mistake

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L474-L481 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L186-L190

Vulnerability details

Impact

A borrower making a simple mistake might be forced to pay an extortionate interest rate for en extended period of time.

Severity rating I was hovering between Medium and High on this one. Medium because it is based on a user mistake. On the other hand, the mistake seems very likely to happen with very likely significant losses. It seems that this would happen on such a regular basis that it must be considered an unacceptable risk within the protocol, and as such I think a High rating is warranted.

Proof of concept

Suppose a borrower has borrowed most of what he can, and that the reserve ratio is at a normal relatively low level. He might want to attract more lenders by increasing the interest rate:

/**
* @dev Modify the interest rate for a market.
* If the new interest rate is lower than the current interest rate,
* the reserve ratio is set to 90% for the next two weeks.
*/
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);
}

As can be seen he is free to do so. However, he might accidentally enter a significantly too high annualInterestBips, up to 100%. Then if he wants to correct his mistake, i.e. lower the interest rate, the reserve ratio must be set to 90%. This reverts if it makes him delinquent, which it most likely will since he was borrowing without excessive margin to delinquency at a lower reserve ratio such as 20%. Therefore he will be forced to pay this exorbitant interest rate until he can return most of his loan, which he was not prepared to do, and which might be exceedingly difficult to do very quickly. Thus, he will be forced to pay an exorbitant interest rate until he can return most of his loan. This might take a long time.

Note that this is a trivial mistake that may very easily happen, and probably will happen regularly, and that when it does happen the preconditions for said severe outcome are very natural and likely.

Recommended mitigation steps

An idea might be to simply check whether it would be possible to reverse the interest rate change to its original value without reverting in setReserveRatioBips(). The problem with this would be that the borrower would have to reduce his borrowing to a reserve ratio of 90% before making any adjustments to the interest. Some more elaborate way to calculate the reserve ratio penalty depending on the current delinquency margin might be possible.

Or perhaps keep track of the minimum interest rate during some past time period (e.g. a week) and allow the interest rate to be freely set to any value above that minimum. Then he can just reset his mistake if he discovers it within this time period.

Assessed type

Other

[d1ll0n]

a. Any actor executing a transaction by accident isn't relevant behavior worth considering b. The reserve ratio is set when the APR is changed the first time, and if the market would be made delinquent by the reserve ratio change, the call reverts. So this scenario is not possible.

[c4-pre-sort]

minhquanym marked the issue as low quality report

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Insufficient quality

[d3e4]

@d1ll0n @laurenceday

a. Any actor executing a transaction by accident isn't relevant behavior worth considering b. The reserve ratio is set when the APR is changed the first time, and if the market would be made delinquent by the reserve ratio change, the call reverts. So this scenario is not possible.

a. It's not an accidental transaction, but a legitimate transaction with unfortunate but valid input. The mistake is either in the form of a typo or changing one's mind. b. The reserve ratio is the same after the APR is increased and doesn't revert the first time. The issue is that it reverts the second time when trying to correct the mistake.

The below test confirms this. Paste into WildMarketController.t.sol.

function test_Usury_from_simple_mistake() public {
  parameters.annualInterestBips = 900; // 9%
  parameters.reserveRatioBips = 2000; // 20%
  setUp();

  _deposit(alice, 50_000e18);
  vm.startPrank(borrower);
  uint256 initialBalance = 100_000e18;
  asset.mint(borrower, initialBalance); // For later loss calculation
  market.borrow(40_000e18);
  asset.transfer(bob, 40_000e18); // Use the loan for something

  // Borrower wants to increase the interest rate slightly to 10% but adds an extra zero
  controller.setAnnualInterestBips(address(market), 10000);

  // Borrower cannot correct the interest rate back to 10% because he only has 20% reserves, but needs 90%.
  vm.expectRevert(IMarketEventsAndErrors.InsufficientReservesForNewLiquidityRatio.selector);
  controller.setAnnualInterestBips(address(market), 1000);

  // Borrower would have to repay 35_000e18 of his loan in order to reset the interest rate,
  // but lets' say it takes him 73 days (1/5 year) to procure the necessary funds.
  // During this time he has to pay the 100% interest rate (10_000e18) plus fees.
  fastForward(73 days);
  asset.transfer(address(market), 45_000e18 - 1);
  vm.expectRevert(IMarketEventsAndErrors.InsufficientReservesForNewLiquidityRatio.selector);
  controller.setAnnualInterestBips(address(market), 1000);

  asset.transfer(address(market), 1); // 45_000e18 is needed
  controller.setAnnualInterestBips(address(market), 1000);

  // In order to reset the reserve ratio to 20% he has to wait 2 weeks.
  // He might have to pay additional interest during this time to whomever loaned him the 45_000e18.
  vm.expectRevert(IWildcatMarketControllerEventsAndErrors.ExcessReserveRatioStillActive.selector);
  controller.resetReserveRatio(address(market));
  fastForward(2 weeks);

  // And then also compensate for the new accrued interest.
  vm.expectRevert(IMarketEventsAndErrors.InsufficientReservesForOldLiquidityRatio.selector);
  controller.resetReserveRatio(address(market));
  asset.transfer(address(market), 230.136986301369863013e18);
  controller.resetReserveRatio(address(market));

  // He cannot borrow back everything he used to reset the interest rate.
  market.borrow(42_161.095890410958904109e18);
  vm.expectRevert(IMarketEventsAndErrors.BorrowAmountTooHigh.selector);
  market.borrow(1);

  // Return the loan and close market to get final cost of loan.
  asset.approve(address(market), type(uint256).max);
  vm.startPrank(bob);
  asset.transfer(borrower, 40_000e18);
  vm.startPrank(address(controller));
  market.closeMarket();

  // The mistake cost at least an extra 9_942e18 (we are only counting extra cost incurred in Wildcat).
  uint256 totalPaidWithMistake = initialBalance - asset.balanceOf(borrower);
  uint256 totalPaidWithoutMistake;
  uint256 percentCostWithoutMistake;
  (totalPaidWithoutMistake, percentCostWithoutMistake) = no_usury();
  uint256 lossDueToMistake = totalPaidWithMistake - totalPaidWithoutMistake;
  assertEq(lossDueToMistake, 9_942.191780821917808219e18);

  // This is 28% interest and fees on the 40_000e18 over less than 3 months, compared to 3% without the mistake.
  uint256 percentCost = totalPaidWithMistake * 100 / 40_000e18;
  assertEq(percentCost, 28);
  assertEq(percentCostWithoutMistake, 3);
}

/** Same as above but without the mistake. */
function no_usury() public returns (uint256 totalPaid, uint256 percentCost) {
  parameters.annualInterestBips = 900; // 9%
  parameters.reserveRatioBips = 2000; // 20%
  setUp();

  _deposit(alice, 50_000e18);
  vm.startPrank(borrower);
  uint256 initialBalance = 100_000e18;
  asset.mint(borrower, initialBalance); // For later loss calculation
  market.borrow(40_000e18);
  asset.transfer(bob, 40_000e18); // Use the loan for something

  // Borrower increases the interest rate slightly to 10%
  controller.setAnnualInterestBips(address(market), 1000);

  // Wait the same time as required to correct the typo interest rate.
  fastForward(73 days);
  fastForward(2 weeks);

  // Return the loan and close market to get final cost of loan.
  asset.approve(address(market), type(uint256).max);
  vm.startPrank(bob);
  asset.transfer(borrower, 40_000e18);
  vm.startPrank(address(controller));
  market.closeMarket();

  // This is 3% interest and fees on the 40_000e18 over less than 3 months.
  totalPaid = initialBalance - asset.balanceOf(borrower);
  percentCost = totalPaid * 100 / 40_000e18;
  assertEq(percentCost, 3);
}

[MarioPoneder]

Thank your for your comment and the added PoC!

Citing from the report:

A borrower making a simple mistake might be forced to pay an extortionate interest rate for en extended period of time.

The impacts described in the original submission are due to user error. Avoiding such user mistakes is a good UX suggestion.
This is not a bug of the protocol per se and user error & UX suggestions have to be graded as QA.
However, there are only downgrades from Medium to QA. Due to the initial issue being reported as High, the severity is overinflated.

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Overinflated severity

[d3e4]

Thank your for your comment and the added PoC!

Citing from the report:

A borrower making a simple mistake might be forced to pay an extortionate interest rate for en extended period of time.

The impacts described in the original submission are due to user error. Avoiding such user mistakes is a good UX suggestion. This is not a bug of the protocol per se and user error & UX suggestions have to be graded as QA. However, there are only downgrades from Medium to QA. Due to the initial issue being reported as High, the severity is overinflated.

I agree in the case of user mistakes in general, but I believe this case and context to be different. A normal QA user mistake would be something like sending assets to the wrong address. This is only an issue from the perspective of the user; as far as the contract is concerned it correctly sent the assets to the address provided. The user clearly understands and agrees with this, so the user is fully responsible for this.

In this case, however, the user can very easily be trapped in a highly undesirable situation which leads to a loss of funds. It is not a user mistake in the sense above, but rather it is that the contract allows for such a deceptive way to lose your money. It is like a booby trap for the user. No user would agree with this functionality if he was aware of it, and users should not be exposed to this kind of risk.

Just to illustrate with a silly example of how a user mistake can clearly be higher than QA, in case this is a matter of perceived principle. Suppose a contract obliges the user to give infinite approval even for depositing a small amount of assets. Then if the user doesn't call a certain function within 3 blocks after the deposit, the owner can call a function to transfer all of the user's assets to himself. If the user forgets or fails to to call this function quickly enough, he will lose everything. This is clearly a High, even though it is a "user mistake".

693 is obviously not as extreme as this, but it is similarly a kind of trap for the user.