executeSellCreditMarket() when creditPositionId != RESERVED_ID , Incorrect calculation of fees

[c4-bot-6]

Lines of code

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

Vulnerability details

Vulnerability details

when call executeSellCreditMarket() We can specify exactAmountIn=false and params.creditPositionId is same CreditPosition The code is as follows:

    function executeSellCreditMarket(State storage state, SellCreditMarketParams calldata params)
...

        } else {
            cashAmountOut = params.amount;

            (creditAmountIn, fees) = state.getCreditAmountIn({
                cashAmountOut: cashAmountOut,
                maxCashAmountOut: params.creditPositionId == RESERVED_ID
                    ? cashAmountOut
                maxCredit: params.creditPositionId == RESERVED_ID
                    ? Math.mulDivUp(cashAmountOut, PERCENT + ratePerTenor, PERCENT - state.getSwapFeePercent(tenor))
                    : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        }
...
    function getCreditAmountIn(
        State storage state,
        uint256 cashAmountOut,
        uint256 maxCashAmountOut,
        uint256 maxCredit,
        uint256 ratePerTenor,
        uint256 tenor
    ) internal view returns (uint256 creditAmountIn, uint256 fees) {
...
        } 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;
        } 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

            revert Errors.NOT_ENOUGH_CASH(maxCashAmountOutFragmentation, cashAmountOut);
        }
    }

The above code, mainly calculates creditAmountIn and fees based on cashAmountOut and fragmentationFee After that, cashAmountOut is transferred to msg.sender, fees is transferred to feeRecipient , and creditAmountIn is recorded as debt.

Note: fragmentationFee should be borne by msg.sender, and cashAmountOut is the value eventually transferred to msg.sender! So in fact, when calculating swapFeePercent, what is given to the user should be: to_borrower_token = cashAmountOut + fragmentationFee, it's just that the final transfer to msg.sender is minus fragmentationFee

The above code uses the formula:

1. creditAmountIn = (cashAmountOut + fragmentationFee) / (1 + ratePerTenor) / (1 - swapFeePercent ) ---> (correct)
2. fees = (cashAmountOut * swapFeePercent) ----> (wrong)

The second formula is wrong, using the data as an example:
Example. cashAmountOut = 100 ratePerTenor = 5% swapFeePercent = 1% fragmentationFee = 0.1

According to the current code formula. creditAmountIn = (100 + 0.1) 1.05 / 0.99 = 106.16666666667 fees = (cashAmountOut swapFeePercent) = 1

One of the formulas we want to ensure is: (to_borrower_token + fees ) (1 + ratePerTenor) = creditAmountIn But according to the above calculation it is not equal: ((100 + 0.1) + 1 ) 1.05 = 106.155 ≠ 106.16666666667

The second formula correctly should be: fees = (cashAmountOut + fragmentationFee) * swapFeePercent / (1 - swapFeePercent)

i.e.: fees = (100 + 0.1 ) * 0.01 / 0.99 = 1.01111111111

This ensures that: (to_borrower_token + fees ) (1 + ratePerTenor) = creditAmountIn => (100 + 0.1 + 1.01111111111) 1.05 = 106.166666667= 106.166666667

Impact

lender pays fees that are always less than the correct value

Recommended Mitigation

    function getCreditAmountIn(
        State storage state,
        uint256 cashAmountOut,
        uint256 maxCashAmountOut,
        uint256 maxCredit,
        uint256 ratePerTenor,
        uint256 tenor
    ) internal view returns (uint256 creditAmountIn, uint256 fees) {
...

        } 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;
+           Math.mulDivUp(cashAmountOut + state.feeConfig.fragmentationFee, swapFeePercent, (PERCENT - swapFeePercent))+ 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

            revert Errors.NOT_ENOUGH_CASH(maxCashAmountOutFragmentation, cashAmountOut);
        }
    }

Assessed type

Math

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[hansfriese]

Apply full credit since the warden has submitted by splitting the primary into two issues. Will invalidate #85 instead.

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #288