howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

The function AccountingLibrary.getCreditAmountIn is used by sellCreditMarket to calculate the amount of credit required to achieve a specific cash amount output when the input params.exactAmountIn is false.

            creditAmountIn = Math.mulDivUp(
                cashAmountOut + state.feeConfig.fragmentationFee, PERCENT + ratePerTenor, PERCENT - swapFeePercent
            );

As seen in the code above, getCreditAmountIn calculates creditAmountIn as follows:

$$ creditAmountIn = \frac{(cashAmountOut + fragmentationFee) \times (1 + ratePerTenor\%)}{1 - swapFee\%} $$

This can be rewritten as:

$$ cashAmountOut + fragmentationFee = creditAmountIn \times \frac{1 - swapFee\%}{1 + ratePerTenor\%} $$

When calculating the fees, the following formula should holds (this is the formula used when params.exactAmountIn is true and getCashAmountOut is called):

$$ cashAmountOut + swapFee + fragmentationFee = \frac{creditAmountIn}{1 + ratePerTenor\%}$$

Thus:

$$\begin{aligned} swapFee &= (cashAmountOut + swapFee + fragmentationFee) - (cashAmountOut + fragmentationFee) \ &= \frac{creditAmountIn}{1 + ratePerTenor\%} - \frac{(creditAmountIn) \times (1 - swapFee\%)}{1 + ratePerTenor\%} \ &= creditAmountIn \times \frac{swapFee\%}{1 + ratePerTenor\%} \ &= \frac{(cashAmountOut + fragmentationFee) \times (1 + ratePerTenor\%)}{1 - swapFee\%} \times \frac{swapFee\%}{1 + ratePerTenor\%} \ &= (cashAmountOut + fragmentationFee) \times \frac{swapFee\%}{1 - swapFee\%} \end{aligned}$$

However, the swapFee is not calculated correctly in getCreditAmountIn:

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

        uint256 maxCashAmountOutFragmentation = 0;

        if (maxCashAmountOut >= state.feeConfig.fragmentationFee) {
            maxCashAmountOutFragmentation = maxCashAmountOut - state.feeConfig.fragmentationFee;
        }

        // slither-disable-next-line incorrect-equality
        if (cashAmountOut == maxCashAmountOut) {
            // no credit fractionalization

            creditAmountIn = maxCredit;
@>          fees = Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT);
        } 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 swapFee is calculated by Math.mulDivUp(cashAmountOut, swapFeePercent, PERCENT), but it should be $(cashAmountOut + fragmentationFee) \times \frac{swapFee\%}{1 - swapFee\%}$ or $creditAmountIn \times \frac{swapFee\%}{1 + ratePerTenor\%} $.

As a result, the swap fee is underestimated (by approximately swapFee percent) in the getCreditAmountIn route. If this route (where params.exactAmountIn is false) is used in a sellCreditMarket call instead of the getCashAmountOut route (where params.exactAmountIn is true), the lender would pay less cash to obtain the same credit position. Furthermore, the protocol would receive a lower swap fee.

Proof of Concept

// SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.0;

import {CREDIT_POSITION_ID_START, RESERVED_ID} from "@src/libraries/LoanLibrary.sol";
import {Math, PERCENT} from "@src/libraries/Math.sol";
import {BaseTest} from "@test/BaseTest.sol";

import {Vars} from "@test/BaseTest.sol";
import {YieldCurveHelper} from "@test/helpers/libraries/YieldCurveHelper.sol";
import {console2 as console} from "forge-std/console2.sol";

contract PoC is BaseTest {
    function setUp() public override {
        super.setUp();
        _labels();
    }

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

        uint256 amount = 100e6;
        uint256 tenor = 365 days;

        uint256 futureValue = Math.mulDivUp(amount, (PERCENT + 0.03e18), PERCENT - size.feeConfig().swapFeeAPR);

        Vars memory _before = _state();

        uint256 snapshot = vm.snapshot();
        _sellCreditMarket(bob, alice, RESERVED_ID, futureValue, tenor, true);
        {
            Vars memory _after = _state();
            (, uint256 creditPositionCount) = size.getPositionsCount();
            uint256 creditPositionId = CREDIT_POSITION_ID_START + creditPositionCount - 1;
            console.log("Senario 1: sellCreditMarket with exactAmountIn as true - go getCashAmountOut route");
            console.log(
                "  feeReceived: ", _after.feeRecipient.borrowATokenBalance - _before.feeRecipient.borrowATokenBalance
            );
            console.log(
                "  borrower szaUSDC increased: ", _after.bob.borrowATokenBalance - _before.bob.borrowATokenBalance
            );
            console.log("  borrower szDebtUSDC increased: ", _after.bob.debtBalance - _before.bob.debtBalance);
            console.log(
                "  lender szaUSDC decreased: ", _before.alice.borrowATokenBalance - _after.alice.borrowATokenBalance
            );
            console.log("  lender credit increased: ", size.getCreditPosition(creditPositionId).credit);
            console.log("");
        }

        vm.revertTo(snapshot);
        _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);
        {
            Vars memory _after = _state();
            (, uint256 creditPositionCount) = size.getPositionsCount();
            uint256 creditPositionId = CREDIT_POSITION_ID_START + creditPositionCount - 1;
            console.log("Senario 2: sellCreditMarket with exactAmountIn as false - go getCreditAmountIn route");
            console.log(
                "  feeReceived: ", _after.feeRecipient.borrowATokenBalance - _before.feeRecipient.borrowATokenBalance
            );
            console.log(
                "  borrower szaUSDC increased: ", _after.bob.borrowATokenBalance - _before.bob.borrowATokenBalance
            );
            console.log("  borrower szDebtUSDC increased: ", _after.bob.debtBalance - _before.bob.debtBalance);
            console.log(
                "  lender szaUSDC decreased: ", _before.alice.borrowATokenBalance - _after.alice.borrowATokenBalance
            );
            console.log("  lender credit increased: ", size.getCreditPosition(creditPositionId).credit);
            console.log("");
        }
    }
}

Place the above code in the file test/PoC.t.sol, then run the command forge test --mc PoC -vv. You will see the following result:

Logs:
  Senario 1: sellCreditMarket with exactAmountIn as true - go getCashAmountOut route
    feeReceived:  502513
    borrower szaUSDC increased:  99999999
    borrower szDebtUSDC increased:  103517588
    lender szaUSDC decreased:  100502512
    lender credit increased:  103517588

  Senario 2: sellCreditMarket with exactAmountIn as false - go getCreditAmountIn route
    feeReceived:  500000
    borrower szaUSDC increased:  100000000
    borrower szDebtUSDC increased:  103517588
    lender szaUSDC decreased:  100500000
    lender credit increased:  103517588

This proof of concept (PoC) compared two sellCreditMarket calls a user can choose to sell the same credit position. The first call input true for exactAmountIn and followed the getCashAmountOut route, while the second input false for exactAmountIn and followed the getCreditAmountIn route.

As seen in the log, the lender received the same credit and the borrower owned the same debt in both scenarios (the difference in the borrower's szaUSDC increase is dust amount due to precision loss). However, in the second scenario, the lender paid less cash, and the protocol received a smaller fee.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Calculate the swap fee in getCreditAmountIn using the formula: $ creditAmountIn \times \frac{swapFee\%}{1 + ratePerTenor\%} $

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

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