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

3 stars 1 forks source link

The protocol cannot collect the full fee when borrowers sell credit without a fragmentation fee. #23

Closed c4-bot-4 closed 4 months ago

c4-bot-4 commented 4 months ago

Lines of code

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

Vulnerability details

Impact

When borrowers want to sell credit A with rate r for T period, lenders need to pay V in cash at that time. The relationship between these values is correctly illustrated in the documentation.

V = A * (1 - k * T) / (1 + r) - f

Here, k is the swap fee, and f is the fragmentation fee. However, when f is 0, lenders pay less cash than they should, and the protocol receives less fee than expected. Since the fee is the main revenue source for the protocol and this loss is not ignorable, the impact becomes high. And this occurs every time credit is sold.

Proof of Concept

Let's denote some values for the explanation.

Let's write the formula again.

V = A * (1 - k * T) / (1 + r) - f

When a borrower wants to receive exactly cash V, the credit A and the fees for the protocol are calculated in the getCreditAmountIn function.

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

    } else {
        cashAmountOut = params.amount;

        (creditAmountIn, fees) = state.getCreditAmountIn({  // @audit, here
            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
        });
    }
}

Suppose creditPositionId is RESERVED_ID. Then, maxCashAmountOut becomes equal to cashAmountOut, i.e., V. And maxCredit becomes V * (1 + r) / (1 - K * T). In the getCreditAmountIn function, the fees become V * K * T.

function getCreditAmountIn(
    State storage state,
    uint256 cashAmountOut,
    uint256 maxCashAmountOut,
    uint256 maxCredit,
    uint256 ratePerTenor,
    uint256 tenor
) internal view returns (uint256 creditAmountIn, uint256 fees) {
    if (cashAmountOut == maxCashAmountOut) {
        creditAmountIn = maxCredit;
        fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT); // @audit, here
    } else if (cashAmountOut < maxCashAmountOutFragmentation) {

    } else {

    }
}

From the lender's side, they will receive A in the future, so they should pay A / (1 + r) at this point. However, the sum of V and the protocol fee is less than this value.

A / (1 + r) = V / (1 - K * T)
V + V * K * T < V / (1 - K * T)

This is because the current swap amount is A / (1 + r), not V. V is the net cash amount the borrower will receive. The protocol fee should apply to the swap amount i.e the protocol fee should be A * K * T / (1 + r). The protocol loss will be as follows:

A * K * T / (1 + r) - V * K * T = (A / (1 + r) - A * (1 - k * T) / (1 + r)) * K * T = (A / (1 + r)) * (K * T) * (K * T)

When the fee is 200 USD, the loss becomes 1 USD when the swap fee is 0.5%. You can check below log

borrowAToken amount sent from lender            ==>  100500000
borrowAToken amount should be sent from lender  ==>  100502512
*********************
fee for protocol                                ==>  500000
fee is not sent to the protocol by mistake      ==>  2512

Please add below test to the test/local/actions/SellCreditMarket.t.sol:

function test_SellCreditMarket_liquidator_profit_no_fragmentation_fee() public {
    NonTransferrableScaledToken borrowAToken = size.data().borrowAToken;
    IERC20Metadata debtToken = IERC20Metadata(address(size.data().debtToken));

    _deposit(alice, usdc, 200e6);
    _deposit(bob, weth, 100e18);
    _buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(365 days, 0.03e18));

    uint256 feeRecipientBalanceBefore = borrowAToken.balanceOf(size.feeConfig().feeRecipient);
    uint256 bobBalanceBefore = borrowAToken.balanceOf(bob);
    uint256 amount = 100e6;
    uint256 tenor = 365 days;
    uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);

    uint256 feeRecipientBalanceAfter = borrowAToken.balanceOf(size.feeConfig().feeRecipient);
    uint256 bobBalanceAfter = borrowAToken.balanceOf(bob);

    // The borrowAToken amount that the lender will receive after 365 days
    uint256 credit = size.getDebtPosition(debtPositionId).futureValue;

    // The borrowAToken amount to be sent to the borrower and protocol.
    uint256 spentFromAlice = bobBalanceAfter + feeRecipientBalanceAfter - bobBalanceBefore - feeRecipientBalanceBefore;
    // The borrowAToken amount that the lender should pay in order to receive credit 365 days later
    uint256 shouldSpentAmount = Math.mulDivDown(credit, PERCENT, (PERCENT + 0.03e18));

    console2.log('borrowAToken amount sent from lender            ==> ', spentFromAlice);
    console2.log('borrowAToken amount should be sent from lender  ==> ', shouldSpentAmount);

    uint256 feeForProtocol = feeRecipientBalanceAfter - feeRecipientBalanceBefore;
    uint256 liquidatorBenefit = shouldSpentAmount - spentFromAlice;

    console2.log('*********************');
    console2.log('fee for protocol                                ==> ', feeForProtocol);
    console2.log('fee is not sent to the protocol by mistake      ==> ', liquidatorBenefit);
}

Tools Used

Recommended Mitigation Steps

function getCreditAmountIn(
    State storage state,
    uint256 cashAmountOut,
    uint256 maxCashAmountOut,
    uint256 maxCredit,
    uint256 ratePerTenor,
    uint256 tenor
) internal view returns (uint256 creditAmountIn, uint256 fees) {
    if (cashAmountOut == maxCashAmountOut) {
        creditAmountIn = maxCredit;
-        fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT); 
+        fees = Math.mulDivUp(creditAmountIn, swapFeePercent, PERCENT + ratePerTenor);
    } else if (cashAmountOut < maxCashAmountOutFragmentation) {

    } else {

    }
}

Assessed type

Math

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Invalid

c4-judge commented 4 months ago

hansfriese marked the issue as not a duplicate