borrowers face a loss when selling or buying credit by market

[c4-bot-8]

Lines of code

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/AccountingLibrary.sol#L294-L297 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/AccountingLibrary.sol#L326-L329

Vulnerability details

Impact

During buying/selling credit through market fragmentation fee is an important aspect of the protocol when existing credit positions are involved but the issue is this fee is applied twice leading to invalid accounting & a loss for the borrowers

Proof of Concept

Sell Credit Market when a lender sells their existing credit position for cash to manage their debt position etc then the protocol charges a fractionalization fee

now in SellCreditMarket when selling there is a bool flag to determine whether the input amount is the credit amount or if the user has specified the cash amount they want by setting the var exactAmountIn to true or false

        if (params.exactAmountIn) {
            .
            .
            .
            (cashAmountOut, fees) = state.getCashAmountOut({
                creditAmountIn: creditAmountIn,
                maxCredit: params.creditPositionId == RESERVED_ID ? creditAmountIn : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        } else {
            .
            .
            .
            (creditAmountIn, fees) = state.getCreditAmountIn({
                cashAmountOut: cashAmountOut,
                maxCashAmountOut: params.creditPositionId == RESERVED_ID
                    ? cashAmountOut
                    : Math.mulDivDown(creditPosition.credit, PERCENT - state.getSwapFeePercent(tenor), PERCENT + ratePerTenor),
                maxCredit: params.creditPositionId == RESERVED_ID
                    ? Math.mulDivUp(cashAmountOut, PERCENT + ratePerTenor, PERCENT - state.getSwapFeePercent(tenor))
                    : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        }

now the issue is when this bool is false and when fragmentation occurs we call getCreditAmountIn in the accounting lib & state.feeConfig.fragmentationFee is applied twice firstly when calculating creditAmountIn & then the fee

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

whereas when credit amount is specified & getCashAmountOut is called then fragmentationFee is only applied when calculating fee even though netCashAmountIn in getCashAmountOut does not include the fragmentation fee

There is another inconsistency that when creditAmountOut is calculated during buy credit then fragmentationFee is reduced from cashAmountOut first

uint256 netCashAmountIn = cashAmountIn - state.feeConfig.fragmentationFee;

hence there is a loss for the credit seller when calling getCreditAmountIn

Buy Credit Market

Now there is similar inconsistency when buying credit through market too

The BuyCreditMarket.executeBuyCreditMarket function accounts for the fragmentationFee twice.

The BuyCreditMarket.executeBuyCreditMarket function calls either the AccountingLibrary.getCreditAmountOut or the AccountingLibrary.getCashAmountIn functions.

Both of the above functions account for the fragmentation fee in the internal accounting as shown below respectively.

AccountingLibrary.getCreditAmountOut

        uint256 netCashAmountIn = cashAmountIn - state.feeConfig.fragmentationFee; //@audit-info - account for hte fragmentation fee
        //@audit-info - account for the fragmentation fee in the form of creditAmountOut
        creditAmountOut = Math.mulDivDown(netCashAmountIn, PERCENT + ratePerTenor, PERCENT);

AccountingLibrary.getCashAmountIn

        uint256 netCashAmountIn = Math.mulDivUp(creditAmountOut, PERCENT, PERCENT + ratePerTenor);
        cashAmountIn = netCashAmountIn + state.feeConfig.fragmentationFee;

But both the above functions add the fragmentationFee again to calculate the total fee as shown below respectively

fees = getSwapFee(state, netCashAmountIn, tenor) + state.feeConfig.fragmentationFee;

fees = getSwapFee(state, netCashAmountIn, tenor) + state.feeConfig.fragmentationFee;

Then the fee gets deducted from the borrower (The current lender of the creditPosition buying the new credit position) in the BuyCreditMarket.executeBuyCreditMarket method, which means the borrower is paying for the fragmentation fees again as shown below and hence encountering a loss

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

Tools Used

Manual Review

Recommended Mitigation Steps

The fragmentation fee logic needs to be thought over and it should be consistent when buying/selling & whether credit/cash amount is specified as input or not.

The updated logic should ensure there is no double accounting of fragmentation fee when calculating the fee to be charged in both the BuyCreditMarket.executeBuyCreditMarket and SellCreditMarket.executeSellCreditMarket functions.

Hence the logic for fee calculation in terms of fragmentation fee should be corrected accordingly.

Assessed type

Math

[udsene]

Hi @hansfriese,

The original issue explains how the fragmentation fee is applied twice (It should have been only once), while executing the BuyCreditMarket.executeBuyCreditMarket and SellCreditMarket.executeSellCreditMarket functions. Hence this vulnerability leads to invalid accounting since the borrower is paying for fragmentation fee twice in the executeBuyCreditMarket operation and the lender is paying the fragmentation fee twice in the executeSellCreditMarket operation.

Hence this is loss of funds to the borrower and lender (the users of the protocol) respectively as described above.

Please reconsider this issue.

[hansfriese]

I believe you've misunderstood the mechanism. The fragmentation fee logic works as intended. Please check this primary issue for details.