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

3 stars 1 forks source link

Seller/Borrower is grieved by making him pay the swapFee for the fragmentionFees that goes to the protocol. - SellCreditMarketOrder. #189

Closed howlbot-integration[bot] closed 4 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/SellCreditMarket.sol#L127-L203 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/AccountingLibrary.sol#L185-L217

Vulnerability details

Impact

Seller/Borrower is grieved by making him pay the swapFee for the fragmentionFees too (which he is paying to the protocol) because the cashAmountOut that the borrower receives is calculated by reducing the fragmentaionFees*swapFeePercent and the fragmentaionFees along with actual swapfee.

Proof of Concept

Lets take the example of getCashAmountOut() to describe this issue.

The sender is passing the params.amount as a parameter to executeSellCreditMarket() that he needs to give back in the future as credit and the getCashAmountOut() is calculating the amount seller receives now as part of this transaction.

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

 function executeSellCreditMarket(State storage state, SellCreditMarketParams calldata params)
        external
        returns (uint256 cashAmountOut)
    { .....
        if (params.exactAmountIn) {
            creditAmountIn = params.amount;

=>          (cashAmountOut, fees) = state.getCashAmountOut({
                creditAmountIn: creditAmountIn,
                maxCredit: params.creditPositionId == RESERVED_ID ? creditAmountIn : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        .....
    }

Now lets see how the cashAmountOut calculation is done in getCashAmountOut() when there is fragmentaion. https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/AccountingLibrary.sol#L185-L217

    function getCashAmountOut(
       .....
    ) internal view returns (uint256 cashAmountOut, uint256 fees) {
=>      uint256 maxCashAmountOut = Math.mulDivDown(creditAmountIn, PERCENT, PERCENT + ratePerTenor);
        if (creditAmountIn == maxCredit) {
            // no credit fractionalization
            ....
        } else if (creditAmountIn < maxCredit) {
            // credit fractionalization
=>            fees = getSwapFee(state, maxCashAmountOut, tenor) + state.feeConfig.fragmentationFee;
            if (fees > maxCashAmountOut) {
                revert Errors.NOT_ENOUGH_CASH(maxCashAmountOut, fees);
            }

            cashAmountOut = maxCashAmountOut - fees;
        } else {
            revert Errors.NOT_ENOUGH_CREDIT(creditAmountIn, maxCredit);
        }
    }

Now lets go through the calculaiton,

maxCashAmount =creditAmountIn/(1+r) , where r is the ratePerTenor.

That means maxCashAmount includes the swapfees , fragmentationFees and the actual cashAmountOut that the borrower receives.

=> maxCashAmount = swapFees + fragmentationFees + cashAmountOut

and also total fees is => fees = swapFees + fragmentationFees.

and we also know that swapFees are calculated by swapFees = getSwapFee(cashAmountOut + swapFees) =>swapFees = (cashAmountOut + swapFees) swapPercentage, =>swapFees = (cashAmountOut / (1 - swapPercentage) ) swapPercentage

because we know that cashAMountOut that the borrower finally receives is after paying the swapfee.

Now lets see how the total fees is calcualted here in the protocol,

fees = getSwapFee(state, maxCashAmountOut, tenor) + fragmentationFee

=> fees = getSwapFee(swapFees + fragmentationFees + cashAmountOut) + fragmentationFee

As we can see the protocol is considering the fragmentationFee for swapFee too inorder to calculate the total fees and again adding the fragmentationFee seperately casuing the user to grieve and the fee recipient gaining extra gains than deserved.

So Borrower is paying the swapFees for fragmentationFees too which is destined to go to the protocol whcih can also be interpreted as,

We can verify this concept by going through the buyCreditMarket where the fragmentaionFees are not included in credit for the cashAmount borrower receives.FragmentaionFees is always avoided from being calculated for the interest nor is used to find the swapFees that the feerecipient receive.

This miscalculation has lead to the borrowers grief in getCreditAmountIn function also, where the creditAmountIn is calculated wrong.

Tools Used

Manual Review

Recommended Mitigation Steps

coorecting the fees and cashAmountOUT calculation given the creditAmountIn

let maxCashAmount = credit/1+r. the present value of the credit.

fees = (maxCashAmount - fragmentationFees ) * swapPercentage + fragmentationFees.

cashAmountOUT = (maxCashAmount - fragmentationFees)*(1 - swapPercentage); (1) or cashAmountOUT = maxCashAmount - fees ; (2) (both results to same value.)

Proof- cashAmountOUT = maxCashAmount - ((maxCashAmount - fragmentationFees ) swapPercentage + fragmentationFees) cashAmountOUT = maxCashAmount - (maxCashAmountswapPercentage - fragmentationFeesswapPercentage +fragmentationFees) cashAmountOUT = maxCashAmount - maxCashAmountswapPercentage + fragmentationFeesswapPercentage - fragmentationFees cashAmountOUT = (maxCashAmount - fragmentationFees)(1 - swapPercentage); (1) = (2)


coorecting the fees and creditAmountIn calculation given the cashAMountOut

fees = (cashAMountOut/1-swapPercentage) * swapPercentage + fragmentaionFee

creditAmountIn = ( (cashAmountOUT/(1-swapPercentage)) + fragmentationFee ) * (1+r)

In both cases we can confirm , creditAmountIn = (cashAmountOut + Fees)* (1+r).

Assessed type

Math

aviggiano commented 4 months ago

I believe this is incorrect. The borrower must pay for fragmentation fees in this case since they are causing a credit split. Since they want "exact amount out", they will have to add this fee to the credit amount as future cash flow, to be repaid to the lender at the due date.

hansfriese commented 4 months ago

The current logic doesn't charge swapFees for fragmentationFees.

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Insufficient proof