Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 this issue Sep 20, 2024 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_12_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

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:

  • The APR can not be increased
  • The APR can not be locked two weeks later
  • The APR can not be reduced no more than 25% two weeks later

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).

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

  • (1) A borrower can reduce APR from 10% to 7.5% with no penalty:
    • The borrower can lock the APR at 7.5% two weeks later by calling setAnnualInterestAndReserveRatioBips() on the market with the current APR(7.5%)
    • Or the borrower can increase the APR anytime with no restriction
    • Or the borrower can reduce APR up to 25% again with no penalty two weeks later
  • (2) A borrower can reduce APR from 10% to 7% as long as the market meets at least 60% reserve ratio requirement:
    • The borrower should be able to lock the APR at 7% two week later by calling setAnnualInterestAndReserveRatioBips() on the market with the current APR(7%)
    • Or the borrower should be able to increase the APR anytime with no restriction
    • **Or the borrower should be able to reduce the APR no less than 5.25%(up to 25% reduction) with no penalty two weeks later

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:

  • The APR should be able to be increased even the market is delinquent
  • The APR should be able to be locked two weeks later even the market is delinquent
  • The APR should be able to be reduced up to 25% two weeks later even the market is delinquent

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

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_12_group AI based duplicate group recommendation bug Something isn't working edited-by-warden primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 20, 2024
howlbot-integration bot added a commit that referenced this issue Sep 20, 2024
@d1ll0n
Copy link

d1ll0n commented Oct 2, 2024

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:

  • The increase in reserve ratio is intended to ensure lenders are able to withdraw if the borrower is changing the terms of the loan.
  • The non-delinquency restriction is in place so that the borrower can not adjust the market to a reserve ratio that would cause the market to become delinquent (or leave it delinquent if it already is).
  • The borrower shouldn't be able to lock in the reduced APR and reset the reserve ratio if they are delinquent / failing to actually provide lenders the opportunity to withdraw following an APR reduction (InsufficientReservesForOldLiquidityRatio, i.e. for a reduction in the reserve ratio)

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).

@d1ll0n d1ll0n added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Oct 2, 2024
@3docSec
Copy link

3docSec commented Oct 3, 2024

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 c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Oct 3, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2024

3docSec changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2024

3docSec marked the issue as grade-b

@piken
Copy link

piken commented Oct 14, 2024

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
Copy link

3docSec commented Oct 14, 2024

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

@C4-Staff C4-Staff added grade-a and removed grade-b labels Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_12_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants