Charging more Credit than what it should be really charged to CreditSellers when Selling Credit with a market order for a specific amount of cash.

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/SellCreditMarket.sol#L177 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/AccountingLibrary.sol#L253-L255

Vulnerability details

Impact

CreditSellers are forced to pay more Credit for the requested amount of cash when SellingCredit with a Market Order.

Proof of Concept

For this report, we are going to follow the execution path when a CreditSeller is a Borrower who wants to take a Loan for an exact amount of cash from a CreditBuyer (Lender) using a MarketOrder.

• Important to mention that this problem is present in all instances when the CreditSeller is the caller and is requesting a specific amount of cash.

When a CreditSeller is selling Credit, the CreditSeller is exchanging credit for cash, therefore, the CreditSeller must be responsible for covering the swapFees, as well as fragmentationFees (if any, when selling partial existing credit positions).

• When a CreditSeller requests an exact amount of cash, the contracts have to compute how much Credit should be charged to the CreditSeller for the requested amount of cash, and, the fees that would need to be paid for receiving that amount of cash.   - Why? Because those fees are taken out of the CreditBuyer's balance, as well as the exact amount of cash requested by the Credit Seller, in total, the CreditBuyer is giving cashAmountOut + fees, therefore, the CreditBuyer needs to be compensated in Credit for all the amount of cash that was taken out of his balance.

When computing the amount of credit to charge the CreditSeller for the requested amount of cash, the SellCreditMarket.executeSellCreditMarket() function invokes the getCreditAmountIn() function. The maxCredit parameter that is passed to this function is computed based on the cashAmountOut requested by the CreditSeller, the ratePerTenor, and, (erronously), the swapFeePercent.

• The parameter maxCredit, is the amount of credit that will be charged to the CreditSeller for the requested amount of cash that will be taken from the CreditBuyer.   - The formula used to compute the maxCredit: Math.mulDivUp(cashAmountOut, PERCENT + ratePerTenor, PERCENT - state.getSwapFeePercent(tenor)), aims to compute the amount of Credit that should be charged for the cashAmountOut and the corresponding swapFees that needs to be paid for receiving cashAmountOut.     - The problem is that this formula computes a bigger amount of Credit to repay for the cashAmountOut + swapFees that will be taken from the CreditBuyer.

The amount of Credit that should be charged to the CreditSeller would be for the exact amount of cash taken from the CreditBuyer, in this case, the total amount of cash that is taken from the CreditBuyer is the cashAmountOut + swapFees. The logic that should be applied to determine the amount of Credit to charge should be as follows:

• swapFees is computed based on the cashAmountOut the CreditSeller is receiving.
• maxCredit is computed based on the cashAmountOut the CreditSeller receives + the swapFees the CreditSeller needs to pay for receiving cashAmountOut.   - cashAmountOut + swapFees are taken from the CreditBuyer, therefore, CreditBuyer must be compensated for that amount of cash in Credit

Find below a coded PoC where it demonstrates the flaw with the current formula and shows that the amount of credit is higher than what it should really be.

Coded PoC

Create a new file under the test/ folder, and, add the below code in it.

Expand to see the Coded PoC

``` // SPDX-License-Identifier: MIT pragma solidity 0.8.23; import {Test, console2} from "forge-std/Test.sol"; import {Math, PERCENT, YEAR} from "@src/libraries/Math.sol"; contract H_01_PoC is Test { //@audit => Setting swapFeeApr to 5% //@audit => Same formula as AccountingLibrary.swapFeePercent() //https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/AccountingLibrary.sol#L164-L166 function getSwapFeePercent(uint256 tenor) internal view returns (uint256) { uint256 swapFeeAPR = 0.05e18; return Math.mulDivUp(swapFeeAPR, tenor, YEAR); } //@audit => Same formula as AccountingLibrary.getSwapFee() //https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/AccountingLibrary.sol#L173-L175 function getSwapFee(uint256 cash, uint256 tenor) internal view returns (uint256) { return Math.mulDivUp(cash, getSwapFeePercent(tenor), PERCENT); } function test_wrongFormulaToComputeMaxCredit_PoC() public { //@audit-info => Computing maxCashAmount using the formula that is used when calling `getCreditAmountIn()` uint256 cashAmountOut = 100000e6; //100k USDC uint256 ratePerTenor = 0.3e18; //30% uint256 tenor = 365 days; //1 year //@audit => The below formula is the same formula used to compute `maxCredit` when SellCreditMarket and request `cashAmountOut` //https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/SellCreditMarket.sol#L176-L177 //@audit => Current formula used to compute `maxCredit` for `cashAmountOut` when SellCreditMarket //@audit => The swapFees affects the percentage side of the equation. //@audit-issue => By affecting the percentage side, the result is a bigger amount of credit than what should really be! uint256 maxCredit = Math.mulDivUp(cashAmountOut, PERCENT + ratePerTenor, PERCENT - getSwapFeePercent(tenor)); //@audit => This would be the correct formula to compute the exact amount of Credit to charge for `cashAmountOut` and `swapFees` //@audit => The swapFees affects the cash value of the equation //@audit-ok => Exact credit to charge for the total cash (including fees) taken from the CreditBuyer. uint256 swapFeePercent = getSwapFeePercent(tenor); uint256 swapFees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT); uint256 maxCreditSecondOption = Math.mulDivUp(cashAmountOut + swapFees, PERCENT + ratePerTenor, PERCENT); console2.log("maxCredit: ", maxCredit); console2.log("maxCreditSecondOption: ", maxCreditSecondOption); uint256 spread = maxCredit - maxCreditSecondOption; console2.log("spread: ", spread); //@audit => The current formula of `maxCredit` would charge ~342 USDC more than if `swapFees` were added to the total cash the CreditSeller will take from the CreditBuyer! assertApproxEqAbs(spread, 342e6, 1e6); //@audit => The CreditSeller should be charged Credit worth the exact amount of cash (including fees) he is taking from the CreditBuyer. //@audit-issue => swapFees are accounted on the percentage side of the equation by reducing the swapFeePercent from the PERCENT, this will errounously compute a bigger amount of Credit to pay for cash + swapFees //@audit-recommendation => Account for the fees (swapFees and fragmentationFee) the CreditSeller pays on the total cash taken from the CreditBuyer, in this way, the formula will compute the credit to pay for the exact amount of cash taken. } } ```

Run the test with the command: forge test --match-test test_wrongFormulaToComputeMaxCredit_PoC -vvvv

Notice the value of spread is ~3.421e8, which is the same as ~342e6, which is the equivalent of ~342 USDC that are charged extra with the first formula (the formula of the current implementation).

Tools Used

Manual Audit

Recommended Mitigation Steps

The recommendation to fix this problem is to account for the fees on the cash side of the equation, instead of accounting for the fees on the percentage side. In this way, the formula will compute the exact amount of credit for the exact amount of cash that is taken from the CreditBuyer.

On the code, the recommendation would look like this:

function executeSellCreditMarket(State storage state, SellCreditMarketParams calldata params)
  external
  returns (uint256 cashAmountOut)
{
  ...

  if (params.exactAmountIn) {
      ...
  } else {
    cashAmountOut = params.amount;

    //@audit => Compute the exact amount of swapFees to pay
+   uint256 swapFeePercent = state.getSwapFeePercent(tenor);
+   uint256 swapFees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT);  

    (creditAmountIn, fees) = state.getCreditAmountIn({
      cashAmountOut: cashAmountOut,
      maxCashAmountOut: params.creditPositionId == RESERVED_ID
          ...
      maxCredit: params.creditPositionId == RESERVED_ID
          //@audit => Account for the swapFees on the cash side of the equation!
-         ? Math.mulDivUp(cashAmountOut, PERCENT + ratePerTenor, PERCENT - state.getSwapFeePercent(tenor))
+         ? Math.mulDivUp(cashAmountOut + swapFee, PERCENT + ratePerTenor, PERCENT)
          : creditPosition.credit,
      ...
    });
  }
}

Important: Make sure to apply the same logic of accounting the fees on the cash side of the equation instead of the percentage side on all the formulas where the fees are currently affecting the percentage side of the equation.

• On all instances where fees should be paid by the CreditSeller, make sure to calculate the exact value of swapFees and then operate with that value, manipulate the fees as if they were cash, don't manipulate fees as if they were percentages, in this way, all the computations will represent the exact correct value.
  • For example, when computing the maxCashAmountOut a CreditSeller can get for selling his credit, update the current formula: Math.mulDivDown(creditPosition.credit, PERCENT - state.getSwapFeePercent(tenor), PERCENT + ratePerTenor) for:

SellCreditMarket.executeSellCreditMarket()

function executeSellCreditMarket(State storage state, SellCreditMarketParams calldata params)
  external
  returns (uint256 cashAmountOut)
{
  ...

  if (params.exactAmountIn) {
      ...
  } else {
    cashAmountOut = params.amount;

+   uint256 swapFees;
+   uint256 netCashTakenFromCreditBuyer;
+   if(params.creditPositionId == RESERVED_ID) {
+     swapFees = state.getSwapFee(cashAmountOut,tenor);
      //@audit => These fees are taken from the CreditBuyer, so, he must be compensated with extra credit for the exact amount of fees
+     netCashTakenFromCreditBuyer = cashAmountOut + swapFees;
+   }

    (creditAmountIn, fees) = state.getCreditAmountIn({
        ...
        maxCredit: params.creditPositionId == RESERVED_ID
-           ? Math.mulDivUp(cashAmountOut, PERCENT + ratePerTenor, PERCENT - state.getSwapFeePercent(tenor))
            //@audit => `cashAmount + swapFees` is the exact amount of Cash taken from the CreditBuyer, so, CreditSeller must compensate in credit that exact amount of cash!
+           ? Math.mulDivUp(netCashTakenFromCreditBuyer, PERCENT + ratePerTenor, PERCENT)
            : creditPosition.credit,
        ...
    });
  }

  ...
}

AccountingLibrary.getCreditAmountIn()

function getCreditAmountIn(
    ...
) internal view returns (uint256 creditAmountIn, uint256 fees) {
    ...

    // slither-disable-next-line incorrect-equality
    if (cashAmountOut == maxCashAmountOut) {
        ...
    } else if (cashAmountOut < maxCashAmountOutFragmentation) {
        // credit fractionalization

        //@audit => Total fees the CreditSeller must cover when causing fractionalization
        //@audit => These fees are taken from the CreditBuyer, so, he must be compensated with extra credit for the exact amount of fees
        //@audit => `fees` includes swapFees and fragmentationFee
+       fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT) + state.feeConfig.fragmentationFee;

        creditAmountIn = Math.mulDivUp(
-           cashAmountOut + state.feeConfig.fragmentationFee, PERCENT + ratePerTenor, PERCENT - swapFeePercent
            //@audit => `cashAmount + fees` is the exact amount of Cash taken from the CreditBuyer, so, CreditSeller must compensate in credit that exact amount of cash!
+           cashAmountOut + fees, PERCENT + ratePerTenor, PERCENT

        );
-       fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT) + state.feeConfig.fragmentationFee;
    } else {
        ...
    }
}

Assessed type

Context

[aviggiano]

Duplicate of #213

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese marked the issue as duplicate of #213

[c4-judge]

hansfriese marked the issue as duplicate of #288