code-423n4 / 2024-08-wildcat-findings

3 stars 1 forks source link

The APR can neither be increased nor locked if the market becomes delinquent following a reduction of over 25% in APR #48

Open howlbot-integration[bot] opened 1 month ago

howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L146-L150

Vulnerability details

Impact

if the market becomes delinquent following a reduction of over 25% in APR:

Suppose the initial APR of a market is 10%, and its reserveRatioBips is 20%:

However, the market could become delinquent after the APR is reduced because some lenders might want to exit. If this occurs alongside situation (2), the borrower would be unable to lock the APR at 7% or set the APR to 5.25% two weeks later, or increase the APR.

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

  function test_setAnnualInterestAndReserveRatioBips_RestoreOrLockCallRevert() 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 borrows 20K assets, and set annualInterestBips from 10% to 7% (30% reducing)
    vm.startPrank(borrower);
    market.borrow(20_000e18);
    market.setAnnualInterestAndReserveRatioBips(700,0);
    vm.stopPrank();

    //@audit-info current annualInterestBips is 7%
    assertEq(market.annualInterestBips(), 700);
    //@audit-info current reserveRatioBips is 60%
    assertEq(market.reserveRatioBips(), 6000);
    assertTrue(!market.currentState().isDelinquent);
    //@audit-info alice withdraw 10K assets 
    vm.prank(alice);
    market.queueWithdrawal(10_000e18);
    //@audit-info market becomes delinquent after alice's withdrawal
    assertTrue(market.currentState().isDelinquent);
    vm.startPrank(borrower);
    //@audit-info borrower has no way to restore annualInterestBips back to 10%
    vm.expectRevert(IMarketEventsAndErrors.InsufficientReservesForOldLiquidityRatio.selector);
    market.setAnnualInterestAndReserveRatioBips(1000,0);
    //@audit-info borrower can not lock annualInterestBips at 7% two weeks later
    fastForward(2 weeks);
    vm.expectRevert(IMarketEventsAndErrors.InsufficientReservesForOldLiquidityRatio.selector);
    market.setAnnualInterestAndReserveRatioBips(700,0);
    vm.stopPrank();
  }

Tools Used

Manual review

Recommended Mitigation Steps

If the APR was reduced over 25% initially, since the borrower has repaid enough asset in this APR reduction to allow lenders opting out:

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

d1ll0n commented 1 month ago

However, the market could become delinquent after the APR is reduced because some lenders might want to exit. If this occurs alongside situation (2), the borrower would be unable to lock the APR at 7% or set the APR to 5.25% two weeks later, or increase the APR.

A few points here:

With respect to restoring the APR back to the original, it'd be ideal if that was allowed, but the restriction is at the market level as a general rule that markets can not be made delinquent by changes to their config.

Overall I'd consider this a low/informational, certainly not a high especially given this only has the effect of incurring penalties for delinquent borrowers (if the reserve ratio is X, they are obligated to keep sufficient assets in the market for X% of the supply at all times).

3docSec commented 1 month ago

I consider this a valid L - it is reasonable that the delinquent borrower is required to bring the loan back to a healthy state before applying settings

c4-judge commented 1 month ago

3docSec changed the severity to QA (Quality Assurance)

c4-judge commented 1 month ago

3docSec marked the issue as grade-b

piken commented 3 weeks ago

First, I respectfully disagree that the delinquent borrower is required to bring the loan back to a healthy state before applying settings, because the borrower can apply settings under delinquent status anyway as long as they initially reduces APR no more than 25%.

Second, it is reasonable that a borrower should be allowed to increase APR without any restriction, this could help the market to attract more assets and bring the market to be healthy. The current implementation works as below:

  1. The borrower can increase the APR anytime with no restriction even the market is delinquent
  2. The borrower can increase the APR anytime after initial no more than 25% APR reducing with no restriction even the market is delinquent
  3. However, the borrower can not increase the APR if the market becomes delinquent after initial over 25% APR reducing

As we can see, the above situations are handled inconsistently.

I agree that the borrower should not be allowed to decrease reserve ratio by increasing the APR. However, the protocol should not prevent the borrower from increasing the APR to attract more collateral, even if the market is in a delinquent status.

3docSec commented 3 weeks ago

There's no need to press on the point of how the protocol "should" behave: I am sold on that one, and that's why this issue was not closed as invalid / intended behavior. Because of the reasons discussed in previous comments, however, I don't see fit for H/M severity, because while the intent of the protocol is likely as you understood it, it makes little sense for a borrower to increase the rate of a loan they fell already behind with