Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/SellCreditMarket.sol#L168-L181 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/AccountingLibrary.sol#L228-L263

Vulnerability details

Impact

Whenever a SellCreditMarket is executed with params.exactAmountIn = false, some amount of protocol swap fees will always be sent to the lender instead of the protocol.

Meaning whenever a borrower borrows cash, or when a credit owner sells his credit to exit cash early, some portion of swap fees always goes to the credit buyer(lender incase of borrowing).

Proof of Concept

Example, a borrower wants to borrow 1000 USDC using SellCreditMarket any say fee is 5% APR and tenor is 5 years, then ratePerTenor = 20%. So, he calls it with RESERVED_ID and exactAmountIn = false because he wants to borrow exactly 1000 cash.

} else {
            cashAmountOut = params.amount;

            (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
            });
        }

creditAmountIn and fees are calculated using getCreditAmountIn

if (cashAmountOut == maxCashAmountOut) {
            // no credit fractionalization

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

Now, we have ratePerTenor = 0.2e18(20%) and swapFeePercent = 0.1e18(10%). We get:

cashAmountOut = 1000
creditAmountIn = 1334
fees = 100
Positions are created with the calculated amounts.

This means the borrower has to pay 1334 back for borrowing 1000 at 20%. Here, we do not get creditAmountIn = 1200 because the fees are to be paid by the cash reciever(borrower) and in this case, the borrower is also borrowing the fees and hence 20% for each borrowed fees.

state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut); 
        state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees); 
        //@audit fees are paid by the lender at the time of execution so the borrower has to pay back the fees along with its interest accrued

When fees = 100 is borrowed, think it of as a new borrow execution, then interest have to be paid to lender, and also swap fees need to be taken which is fees = 10% of 100 = 10. And for this fees = 10 is also to be borrowed. So, when a borrow is done with amount = cash, it means the each fees taken are subsequent borrows.

See docs for better explaination on creditAmountIn calculation.

Interest to be paid to lender on each subsequent borrows(fee is also borrowed) is calculated correctly but the fees calculation only include the fees for the first borrowed amount and the rest are ignored.

fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT)

Keep in mind that even though the fees for subsequent borrows are ignored while calculating fees, it is still paid by the borrower and is already included in the creditAmountIn calculation, so it means the lender recieves the ignored fees.

The correct fees calculation should be:

at 10% fees ;
-----------------------------------------------
fees for first borrow(amount borrowed = 1000)
    = 10% of 1000
    = 100
-----------------------------------------------
since 100 is also borrowed and so on 
fees on second borrow(amount borrowed = 100)
    = 10% of 100
    = 10
-----------------------------------------------
fees on third borrow(amount borrowed = 10)
    = 10% of 10
    = 1
-----------------------------------------------
Total fees = 100 + 10 + 1
           = 111

But the function returns only 100 for fees, this means 11 amount of fees for the protocol went to the lender instead. Because it is already included in the credtiAmountIn. OR precisely in e16 precision: 11111111 - 10000000 = 1111111

To prove this, lets do the opposite i.e. borrower calls SellCreditAmount with exactAmountIn = false, indicating he wants to repay the exact credit amount specified and calculate the amount of cash he recieves. Lets call this with amount = 1334, so that creditAmouintIn = 1334 exactly the same as our example. Now, with 20% ratePerTenor and 10% fees we get;

if (params.exactAmountIn) { 
            creditAmountIn = params.amount; // 1334

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

 function getCashAmountOut(
        State storage state,
        uint256 creditAmountIn,
        uint256 maxCredit,
        uint256 ratePerTenor,
        uint256 tenor
    ) internal view returns (uint256 cashAmountOut, uint256 fees) {
        uint256 maxCashAmountOut = Math.mulDivDown(creditAmountIn, PERCENT, PERCENT + ratePerTenor);
                                 //@audit 1334 * 1e18 / 1e18 + 0.2e18 = 1111
        if (creditAmountIn == maxCredit) {
            // no credit fractionalization

            fees = getSwapFee(state, maxCashAmountOut, tenor); //@audit 10% of 1111 = 111

            if (fees > maxCashAmountOut) {
                revert Errors.NOT_ENOUGH_CASH(maxCashAmountOut, fees);
            }

            cashAmountOut = maxCashAmountOut - fees; //@audit 1111 - 111 = 1000

fees = 111
cashAmountOut = 1000

From this we can see that when the cash recieved is exactly the same i.e. 1000, credit amount to pay back is also exactly the same i.e. 1334, but the fees are different, 100 and 111.

And this is because there is miscalculation of fees when exactAmountIn = false.

With the same credit = 1334, try more examples such as a lender early exiting by selling 1334 of his credit and we will get the same results i.e. 111 as fees.

Estimation of how much fees would actually be loss using deploy.sol values:

at 0.5% swap fee and tenor = 5 years

amount                              fees loss        USD $ price        
(in USDC e6 precision)
1000                                641025           $ 0.64
10000                               6410256          $ 6.41 
100000                              64102564         $ 64.10
1000000                             641025641        $ 641.02

we have to note here that the borrower doesn't need to necessarily borrow 10000000 for the protocol to loss $641, but with every small borrows the fee loss is accumulating.
...

And since the fee is not fixed and can be changed by the protcol using updateConfig, If the protocol decides to increase the fees to 1%, amount loss will be doubled at 1.5% it will be trippled and so on. Hence, reporting this as a high severity.

Tools Used

manual

Recommended Mitigation Steps

Change the fees calculation in getCreditAmountIn to:

if (cashAmountOut == maxCashAmountOut) {
            // 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(cashAmountOut, swapFeePercent, PERCENT - swapFeePercent) - state.feeConfig.fragmentationFee;

Assessed type

Math

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

Protocol swap fees accidentally sent to lender instead of the protocol due to wrong fees calculation in SellCreditMarket. #323