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-L202 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/AccountingLibrary.sol#L228-L249

Vulnerability details

Impact

Impact of Inconsistent Fee Deduction in SellCreditMarket The inconsistent fee deduction logic in SellCreditMarket.sol can lead to unexpected behavior and potential financial disadvantages for users, particularly when they choose to sell credit with exactAmountIn = false. src/libraries/actions/SellCreditMarket.sol#executeSellCreditMarket

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

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

        (cashAmountOut, fees) = state.getCashAmountOut({
            // ...
        });
    } else {
        cashAmountOut = params.amount;

        (creditAmountIn, fees) = state.getCreditAmountIn({
            // ...
        });
    }

    // ...

    state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut);
    state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees);
}

src/libraries/AccountingLibrary.sol#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);
}

Proof of Concept

Scenario: Let's assume:

exactAmountIn = false
User wants to sell credit for cashAmountOut = 100
ratePerTenor = 0 (for simplicity)
swapFeePercent = 2 (2% fee)

Transaction Flow:

executeSellCreditMarket called:
- `params.exactAmountIn = false, so cashAmountOut is set to 100.
- `getCreditAmountIn is called with cashAmountOut = 100.
getCreditAmountIn calculation:
- `creditAmountIn is calculated based on cashAmountOut = 100 (ignoring fees).
- `fees are calculated as 2 but not deducted from cashAmountOut.
Funds Transfer:
- `100 units are transferred to the user (incorrect).
- `2 units are transferred as fees (correct).

Discrepancy: The user receives 100 units of cashAmountOut even though a 2% fee was applied. After deducting the fee, they should have received 98 units.

Proof: The bug stems from the fact that getCreditAmountIn calculates the fee but doesn't deduct it from the cashAmountOut before it's used to determine creditAmountIn. When exactAmountIn is false, the user receives a higher cashAmountOut than they should.

Impact: This inconsistency creates a financial disadvantage for users who utilize the exactAmountIn = false option, as they effectively incur a hidden cost in the form of an inaccurately calculated fee deduction.

Tools Used

Vs

Recommended Mitigation Steps

Deduct fees before calculating creditAmountIn when exactAmountIn is false. This ensures that the user receives the specified cashAmountOut after fees.
Calculate cashAmountOut before deducting fees when exactAmountIn is true. This makes the behavior consistent with the exactAmountIn = false case.