code-423n4 / 2024-06-size-validation

1 stars 0 forks source link

Potential APR Manipulation and Variability Issues in `BuyCreditMarket` function #617

Closed c4-bot-9 closed 2 months ago

c4-bot-9 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/BuyCreditMarket.sol#L101

Vulnerability details

Description

The BuyCreditMarket function enables lenders to lend with either a fixed APR or a dynamic APR that adjusts based on market conditions and borrower-specified multipliers. In dynamic APR setups, borrowers can opt for potentially lower rates by specifying a negative APR during the yield curve construction, which inversely adjusts the APR based on the market rate. The APR is adjusted according to the formula apr + marketRateMultiplier * marketRate. This mechanism is designed to reflect current market conditions when a borrower sells credit in the market.

However, this flexibility also introduces significant risks due to the absence of a maximum APR cap and the possibility for lenders to manipulate market rates. Lenders might exploit this by back-running calls to setVariablePoolBorrowRate, positioning themselves to always benefit from the highest possible APR. This can lead borrowers to incur higher costs unexpectedly, as there is no safeguard to limit the maximum APR that a borrower could end up paying. The lack of an APR ceiling means that any fluctuations in the market rate can drastically alter the cost of borrowing, potentially leading to transaction rejections if the actual APR strays from the lender’s minimum expected APR, or in borrowers facing unexpectedly high interest.

Example 1:

Example 2:

Suppose Bob wants to borrow money using a dynamic APR that adjusts with market conditions. He chooses a setup where the initial APR is set to (-2%), hoping to leverage decreasing market rates to lower his total borrowing costs. The market rate multiplier is set at 0.5.

Situation

Calculation

Bob’s total APR would be calculated as follows: Total APR = -2% + (0.5 x 10%) = -2% + 5% = 3%

In this situation, Bob benefits because the negative initial APR effectively neutralizes the impact of the current market rate, leading to an APR of 3%.

Changing Market Rate

However, let's consider what happens if the market rate increases or decreases:

Impact

This lack of predictability and potential for manipulation could lead to unfair advantages, transaction failures, and a loss of trust among platform users. Lenders might exploit the system by timing their transactions around favorable market rate adjustments, and borrowers might end up paying higher rates than initially expected.

Proof of Concept

function test_BuyCreditMarket_buyCreditMarket_exactAmountIn() public {
        _deposit(alice, weth, 1000e18);
        _deposit(bob, usdc, 1000e6);
         uint256[] memory tenors = new uint256[](1);
        int256[] memory aprs = new int256[](1);
        uint256[] memory marketRateMultipliers = new uint256[](1);
        aprs[0] = -1e18;
        tenors[0] = 30 days;
        marketRateMultipliers[0] = 0.5e18;
        YieldCurve memory curve = YieldCurve({tenors: tenors, aprs: aprs, marketRateMultipliers: marketRateMultipliers});
        _sellCreditLimit(alice, curve);
        uint256 amountIn = 40e6;
        uint256 tenor = 30 days;
        _setVariablePoolBorrowRate(uint128(3e18));
        // backruns the tx here
        _buyCreditMarket(bob, alice, amountIn, tenor, true);
    }

Tools Used

Recommended Mitigation Steps

  1. Implement Maximum APR Cap: Introduce a maxAPR var in borrowOffer to protect borrowers from unexpectedly high borrowing costs.

Assessed type

Other

ARNO-0 commented 2 months ago

Hello @hansfriese,

This is incorrectly judged as a known issue 7 from Solidified. It differs from that issue based on the following points:

Note: If the poc fails, then add this:

_updateConfig("variablePoolBorrowRateStaleRateInterval", 1);
hansfriese commented 2 months ago

I agree the validator's comment is incorrect. But it's an intended design.

ARNO-0 commented 2 months ago

@hansfriese I disagree because, according to the Size documentation, when lenders use rate hooks, they can always get the maximum desired APR by specifying apr = some value in the formula apr + marketRateMultiplier * marketRate. For example, if a lender wants at least a 5% APR on top of the market rate (e.g., 7%), the lender is protected. However, in the case of borrowers, they always want to pay the minimum APR possible according to the market rate. This does not protect lenders from high APR rises apr = some negative value will become useless.

hansfriese commented 2 months ago

If the borrower is concerned about a high borrow rate due to the elevated market rate, he can simply use a constant rate. Additionally, I won't respond further as I have revalidated the issue.

ARNO-0 commented 2 months ago

@hansfriese If the rate hook feature is provided for the ease of trading for both borrowers and lenders, why wouldn't both parties use it? If there were a bug causing lenders to receive less APR than expected when using the rate hook, would you still say the same thing? The issue is not that borrowers are concerned about high APR; rather, the concern is that borrowers can still receive an high APR even after using this feature. This feature has a bug for borrower you totally ignore this point. This situation is not manageable by borrower in any circumstance.

hansfriese commented 2 months ago

@aviggiano Hope to hear your thoughts on it.

hansfriese commented 2 months ago

The documentation already acknowledges this risk. Although it specifically mentions lenders, I believe it applies to all users who utilize rate hooks. Therefore, I still maintain it is an intended behavior.

This is an experimental feature. Lenders should take care to understand the risks when experimenting with price hooks, as the Aave rate may be prone to manipulation.

ARNO-0 commented 2 months ago

@hansfriese, I respect you for responding further even though you said you wouldn't. Thank you so much.

However, I have a request: could @aviggiano comment on the issue? The core of the issue is that lenders have the option of setting at least a minimum APR, but borrowers do not have the option of setting at most a maximum APR. As I mentioned earlier, APR can grow organically to a high value, causing users to face losses with no protection against it. I don't mind if you don't respond further.

aviggiano commented 2 months ago

Hey, thank you for raising this issue.

The warden explains how users can be negatively impacted by unexpected changes in the variable pool borrow rate, depending on their yield curve parameters, and how this could be mitigated by a ceiling parameter on yield curves.

Although an interesting analysis, I believe this is a known issue. The Known Risks section of the README says:

The Variable Pool Borrow Rate feed is trusted and users of rate hook adopt oracle risk of buying/selling credit at unsatisfactory prices.

This is precisely what is happening in the described scenario.

By creating a yield curve with a 0.5 multiplier and constant aprs terms, big changes in the variable rate would mean the final APR calculated could be very different from the actual market APR, leading to unsatisfactory prices for the user.

Also, as the documentation explains, users who opt in for the market rate multipliers bear the risk of their specific configuration, so I would see the issue you are describing as a user mistake.

Nevertheless, I agree that your recommendation for a ceiling parameter to the final calculation is a good one, so that users can further protect themselves against a big movement. This is why I believe this is a valid QA report.