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

3 stars 1 forks source link

AccountingLibrary's `getCreditAmountIn()` overstates `creditAmountIn` for the given `cashAmountOut` #245

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/AccountingLibrary.sol#L250-L257

Vulnerability details

When credit is being sold to receive cash and the caller specifies cashAmountOut = params.amount, the caller, being cash receiver, needs to pay swap fee by giving up extra credit, which needs to be calculated from the given cash amount. The calculation of the amount credit needed is now done incorrectly and the credit seller gives up an extra credit each time this logic is executed.

Impact

Credit seller is charged extra credit amount with regard to the stated protocol logic each time SellCreditMarket is used with params.exactAmountIn == false. This has no additional prerequisites, and the size of the wrongly charged fee has (swapFeeAPR * tenor) ^ 2 scale, which, even given small enough swapFeeAPR = 0.5% annual fee, can be substantial for large tenors. For example, 10 year loan will have (0.5% * 10) ^ 2 = 0.25% of the extra fee.

Proof of Concept

creditAmountIn is overstated as it's not correct to apply swapFeePercent in the 1 / (1 - rate) way to translate from net to gross amount with regard to swap fee:

AccountingLibrary.sol#L250-L257

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

Since 1 - a < 1 / (1 + a), 0 < a < 1 it's 1 / (1 - a) > (1 + a), the creditAmountIn ends up to be overstated by adding extra 1 / (1 - swapFeePercent) - (1 + swapFeePercent) = swapFeePercent^2 / (1 - swapFeePercent) factor, i.e. the additional (cashAmountOut + state.feeConfig.fragmentationFee) * (PERCENT + ratePerTenor) * swapFeePercent^2 / (1 - swapFeePercent) is incorrectly charged from the credit providing caller. For example, at 0.5% swap fee rate and for 10% APY 5 year loan it is additional (1 + 0.1 * 5) * (0.005 * 5)^2 / (1 - 0.005 * 5), circa 0.1% = 10 b.p. undocumented fee.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider calculating creditAmountIn directly as a future value of the exact cash to be spent, cashAmountOut + fees, e.g. (leaving the fees expression as is, being a subject for another issue):

AccountingLibrary.sol#L250-L257

        } 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;
+           creditAmountIn = Math.mulDivUp(
+               cashAmountOut + fees, PERCENT + ratePerTenor, PERCENT
+           );
+           if (creditAmountIn > maxCredit) {
+               revert Errors.NOT_ENOUGH_CREDIT(creditAmountIn, maxCredit);
+           }
        }

Assessed type

Math

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Invalid