Incorrect calculation of swapFees in `getCreditAmountIn` function

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/AccountingLibrary.sol#L248-L249 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/AccountingLibrary.sol#L253-L256 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/BuyCreditMarket.sol#L195-L196 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/SellCreditMarket.sol#L201-L202 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/SellCreditMarket.sol#L173-L175

Vulnerability details

Vulnerability Details

In the getCreditAmountIn function, the swap fee is incorrectly accounted for, leading to an underestimation of fees. The issue arises from how the swap fee is deducted and then recalculated in the formula.

Problem Explanation

Reference document for the original formula.

https://docs.size.credit/technical-docs/contracts/3.3-market-orders

1. Formula for Cash Amount Out (cashAmountOut):

   The formula for the amount of credit (A) before fees: $$A = \frac{(V + f) (1 + r)}{1 - k\Delta T}$$

   Here, k is the swap fee percentage, ΔT is the remaining time to maturity, V is the cash amount out, f is the fragmentation fee, and r is the rate per tenor. This formula correctly charges the swap fee by deducting the credit by k%.

   Explanation

   • The $$\frac{A}{1 + r}$$ is the amount of cash the credit buyer values the amount of credit he is buying, before fees
   • Then fees are applied and
     • KΔT is charged since the swap fee is a function of the remaining lifetime of the loan and is applied to the cash side of the trade
     • The higher K and ΔT and the higher the amount A to be sold to get the desired amount out
   • f is charged to cover the gas costs related to credit fragmentation
     • The higher f and the higher the amount A to be sold to get the desired amount out

2. Current Implementation Issues:

   maxCashAmountOut: Math.mulDivDown(creditPosition.credit, PERCENT - state.getSwapFeePercent(tenor), PERCENT + ratePerTenor),

   In scenarios where cashAmountOut is equal to maxCashAmount:

   creditAmountIn = maxCredit;
   fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT);

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/AccountingLibrary.sol#L248-L249

In scenarios where cashAmountOut is less than maxCashAmountOutFragmentation:

   creditAmountIn = Math.mulDivUp(
       cashAmountOut + state.feeConfig.fragmentationFee, PERCENT + ratePerTenor, PERCENT - swapFeePercent
   );
   fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT) + state.feeConfig.fragmentationFee;

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

3. Underestimating Swap Fee:

   The swap fee has already been deducted when calculating cashAmountOut using the formula $$( A \times (1 - k)$$. However, in the getCreditAmountIn function, the swap fee is recalculated using cashAmount * k%, leading to an underestimation of the actual swap fee.

Detailed Issue Breakdown

• Formula Breakdown: $$A = \frac{(V + f)(1 + r)}{1 - k}$$

  Here, the swap fee is correctly charged by reducing the credit by k%.

• Incorrect Fee Calculation in getCreditAmountIn:

  creditAmountIn = (cashAmountOut + f) * (1 + r) / (1 - k);
  swapFee = cashAmountOut * k%;

  This leads to an underestimation of the swap fee, as the swap fee is already deducted in the initial formula and then recalculated from the cash amount, leading to: $$ \text{swapFee} = \left( \frac{\text{credit} \times (1 - k)}{(1 + r)} - f \right) \times k $$

• Correct SwapFee Calculation:

  Given: $$ A \times (1 - k) = (V + f) \times (1 + r) $$

  First, solve for $$A$$: $$ A = \frac{(V + f) \times (1 + r)}{1 - k} $$

  The swap fee is given by: $$ A \times k = \text{swapFee} \times (1 + r) $$

  Substitute $$A$$ into the swap fee formula: $$ \text{swapFee} = \frac{\left( \frac{(V + f) \times (1 + r)}{1 - k} \right) \times k}{1 + r} $$

  Simplify the expression: $$ \text{swapFee} = \frac{(V + f) \times (1 + r) \times k}{(1 - k) \times (1 + r)} $$ $$ \text{swapFee} = \frac{(V + f) \times k}{1 - k} $$

  So, the swap fee in terms of $$V$$ is: $$ \text{swapFee} = \frac{(V + f) \times k}{1 - k} $$

  or in terms of $$A$$: $$ \text{swapFee} = \frac{A \times k}{1 + r} $$

Example (exactAmountOut)

Assume the swap fee is 1% yearly.

• Alice borrows $100 from Bob to be repaid with 20% interest in 1 year.
• Candy wants to buy credit and is willing to pay 50% yearly.
• Bob sells credit to Candy and wants to get exactly $50 in cash.

The fee recipient gets:

• \$5 in fragmentation fees plus \$0.5 = \$50 * 0.01 in swap fee.

Bob gets exactly $50 in cash.

Original Swap Fee Calculation:

• Candy pays \$55.5 = \$50 + \$5 + \$0.5 and now owns \$83.33 = (\$50 + \$5) * 1.5 / (1 - 0.01) in credit.

Correct Swap Fee Calculation:

• Candy pays \$50 + \$5 + (\$50 + \$5) * 1.5 / (1 - 0.01) * 0.01 / 1.5 = $55.55.

Impact

The incorrect calculation leads to paying less in swapfees.

Proof of Concept

A clearer comparison is between buyCreditMarket and sellCreditMarket when buying and selling all credits of the same creditPosition.

// Buy Credit Market
++ run buyCreditMarket ++
***@ maxCashAmountIn @***: 861450108
***@ buyCreditSwapFee @***: 4307251

// Sell Credit Market
++ run sellCreditMarket ++
***@ maxcashAmountOut @***: 857142857
***@ sellCreditSwapFee @***: 4285715

By the formula, the swap fee should be the same for both, but it isn't. The swap fee for sellCreditMarket should be equal to maxCashAmountOut: 857142857 * 0.005 (swapFeePercent use 0.5%) / 0.995 = 4307251 (with mulDivUp ) and that's it!

• Fee Transfer in Functions

The two functions transfer fees in slightly different ways:

buyCreditMarket: cashAmountIn is the total cash paid before deducting fees.

state.data.borrowAToken.transferFrom(msg.sender, borrower, cashAmountIn - fees);
state.data.borrowAToken.transferFrom(msg.sender, state.feeConfig.feeRecipient, fees);

sellCreditMarket: cashAmountOut is the seller received cash amount after deducting fees.

state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut);
state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees);

poc file.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;

import {UserView} from "@src/SizeView.sol";
import {DepositParams} from "@src/libraries/actions/Deposit.sol";
import {BaseTest} from "@test/BaseTest.sol";
import {Vars} from "@test/BaseTest.sol";
import {Errors} from "@src/libraries/Errors.sol";

import {PERCENT} from "@src/libraries/Math.sol";
import {CREDIT_POSITION_ID_START, 
        CreditPosition, 
        DEBT_POSITION_ID_START, 
        DebtPosition, 
        LoanStatus, 
        RESERVED_ID} from "@src/libraries/LoanLibrary.sol";
import {LoanOffer, OfferLibrary} from "@src/libraries/OfferLibrary.sol";
import {YieldCurve, YieldCurveLibrary} from "@src/libraries/YieldCurveLibrary.sol";
import {BuyCreditMarketParams} from "@src/libraries/actions/BuyCreditMarket.sol";
import {YieldCurveHelper} from "@test/helpers/libraries/YieldCurveHelper.sol";

import {Math, PERCENT, YEAR} from "@src/libraries/Math.sol";
import "forge-std/console.sol";

contract Test is BaseTest {

    function getUserViewData(address user) internal {
        UserView memory userView = size.getUserView(user);
        // console.log("===========================================================");
        console.log("userView borrowATokenBalance:", userView.borrowATokenBalance);
        console.log("userView collateralTokenBalance:", userView.collateralTokenBalance);
        console.log("userView debt balance:", userView.debtBalance);
        console.log("===========================================================");
    }

    function test_SwapFeePoc() public {
        _deposit(alice, usdc, 2000e6);
        _deposit(bob, weth, 100e18);
        _deposit(candy, usdc, 2000e6);
        _buyCreditLimit(
            alice,
            block.timestamp + 365 days,
            YieldCurveHelper.pointCurve(365 days, 0.2e18)
        );
        _buyCreditLimit(
            candy,
            block.timestamp + 365 days,
            YieldCurveHelper.pointCurve(365 days, 0.4e18)
        );

        uint256 creditPositionId = CREDIT_POSITION_ID_START;

        uint256 amount = 1000e6;
        uint256 tenor = 365 days;
        uint256 debtPositionId = _sellCreditMarket(
            bob,
            alice,
            RESERVED_ID,
            amount,
            tenor,
            false
        );

        CreditPosition memory creditPosition = size.getCreditPosition(
            creditPositionId
        );
        console.log("creditPosition lender:", creditPosition.lender);
        console.log("creditPosition credit:", creditPosition.credit);
        DebtPosition memory debtPosition = size.getDebtPosition(debtPositionId);
        console.log("debtPosition borrower:", debtPosition.borrower);
        console.log("debtPosition futureValue:", debtPosition.futureValue);

        console.log(" ---------- alice ----------");
        getUserViewData(alice);
        console.log(" ---------- bob ----------");
        getUserViewData(bob);
        console.log(" ---------- candy ----------");
        getUserViewData(candy);

        // uint256 creditPositionIdSecond = creditPositionId + 1;
        uint256 debtPositionIdSecond = 0;
        // Test sellCredit by changing buyCredit to false. 
        bool buyCredit = false;
        if (buyCredit) {
            _sellCreditLimit(alice, YieldCurveHelper.pointCurve(365 days, 0.4e18));
            console.log("++ run buyCreditMarket ++");
            uint256 maxCashAmountIn = 861450108;
            console.log("***@ maxCashAmountIn @***:", maxCashAmountIn);
            uint256 buyCreditSwapFee = size.getSwapFee(maxCashAmountIn, tenor);
            console.log("***@ buyCreditSwapFee @***:", buyCreditSwapFee);

            uint256 debtPositionIdSecond = _buyCreditMarket(
                candy,
                creditPositionId,
                maxCashAmountIn,
                true
            );
        } else {
            // (cashAmountOut + f) / swapFee = 0.995 / 0.005
            // (cashAmountOut + f) * 0.005 / 0.995 = swapFee
            // maxCashAmountOut: 857142857
            uint256 cashAmountOut = 857142857;
            console.log("++ run sellCreditMarket ++");
            console.log("***@ maxcashAmountOut @***:", cashAmountOut);
            uint256 sellCreditSwapFee = size.getSwapFee(cashAmountOut, tenor);
            console.log("***@ sellCreditSwapFee @***:", sellCreditSwapFee);
            debtPositionIdSecond = _sellCreditMarket(
                alice,
                candy,
                creditPositionId,
                cashAmountOut,
                tenor,
                false
            );
        }

        creditPosition = size.getCreditPosition(
            creditPositionId
        );
        console.log("creditPosition lender:", creditPosition.lender);
        console.log("creditPosition credit:", creditPosition.credit);
        DebtPosition memory debtPositionSecond = size.getDebtPosition(
            debtPositionIdSecond
        );
        console.log(
            "debtPositionSecond borrower:",
            debtPositionSecond.borrower
        );
        console.log(
            "debtPositionSecond futureValue:",
            debtPositionSecond.futureValue
        );
        console.log(" ---------- alice ----------");
        getUserViewData(alice);
        console.log(" ---------- bob ----------");
        getUserViewData(bob);
        console.log(" ---------- candy ----------");
        getUserViewData(candy);
    }
}

Run the test file and get the output:

buyCreditMarket

Logs:
  creditPosition lender: 0x0000000000000000000000000000000000010000
  creditPosition credit: 1206030151
  debtPosition borrower: 0x0000000000000000000000000000000000020000
  debtPosition futureValue: 1206030151
   ---------- alice ----------
  userView borrowATokenBalance: 995000000
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================
   ---------- bob ----------
  userView borrowATokenBalance: 1000000000
  userView collateralTokenBalance: 100000000000000000000
  userView debt balance: 1206030151
  ===========================================================
   ---------- candy ----------
  userView borrowATokenBalance: 2000000000
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================
  ++ run buyCreditMarket ++
  ***@ maxCashAmountIn @***: 861450108
  ***@ buyCreditSwapFee @***: 4307251
  creditPosition lender: 0x0000000000000000000000000000000000030000
  creditPosition credit: 1206030151
  debtPositionSecond borrower: 0x0000000000000000000000000000000000020000
  debtPositionSecond futureValue: 1206030151
   ---------- alice ----------
  userView borrowATokenBalance: 1852142857
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================
   ---------- bob ----------
  userView borrowATokenBalance: 1000000000
  userView collateralTokenBalance: 100000000000000000000
  userView debt balance: 1206030151
  ===========================================================
   ---------- candy ----------
  userView borrowATokenBalance: 1138549892
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================

sellCreditMarket

Logs:
  creditPosition lender: 0x0000000000000000000000000000000000010000
  creditPosition credit: 1206030151
  debtPosition borrower: 0x0000000000000000000000000000000000020000
  debtPosition futureValue: 1206030151
   ---------- alice ----------
  userView borrowATokenBalance: 995000000
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================
   ---------- bob ----------
  userView borrowATokenBalance: 1000000000
  userView collateralTokenBalance: 100000000000000000000
  userView debt balance: 1206030151
  ===========================================================
   ---------- candy ----------
  userView borrowATokenBalance: 2000000000
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================
  ++ run sellCreditMarket ++
  ***@ maxcashAmountOut @***: 857142857
  ***@ sellCreditSwapFee @***: 4285715
  creditPosition lender: 0x0000000000000000000000000000000000030000
  creditPosition credit: 1206030151
  debtPositionSecond borrower: 0x0000000000000000000000000000000000020000
  debtPositionSecond futureValue: 1206030151
   ---------- alice ----------
  userView borrowATokenBalance: 1852142857
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================
   ---------- bob ----------
  userView borrowATokenBalance: 1000000000
  userView collateralTokenBalance: 100000000000000000000
  userView debt balance: 1206030151
  ===========================================================
   ---------- candy ----------
  userView borrowATokenBalance: 1138571428
  userView collateralTokenBalance: 0
  userView debt balance: 0
  ===========================================================

Tools Used

Manual, Foundry

Recommended Mitigation Steps

To fix the issue, ensure that the swap fee is correctly calculated and not applied twice.

--- src/libraries/AccountingLibrary.sol
+++ src/libraries/AccountingLibrary_fix.sol
@@ -246,14 +246,14 @@
             // no credit fractionalization

             creditAmountIn = maxCredit;
-            fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT);
+            fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT - swapFeePercent);
         } else if (cashAmountOut < maxCashAmountOutFragmentation) {
             // credit fractionalization

             creditAmountIn = Math.mulDivUp(
                 cashAmountOut + state.feeConfig.fragmentationFee, PERCENT + ratePerTenor, PERCENT - swapFeePercent
             );
-            fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT) + state.feeConfig.fragmentationFee;
+            fees = Math.mulDivUp(creditAmountIn, swapFeePercent, PERCENT + ratePerTenor) + state.feeConfig.fragmentationFee;
         } else {
             // for maxCashAmountOutFragmentation < amountOut < maxCashAmountOut we are in an inconsistent situation
             //   where charging the swap fee would require to sell a credit that exceeds the max possible credit

Assessed type

Math

[c4-judge]

hansfriese changed the severity to 3 (High Risk)

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #288