howlbot-integration[bot] commented 2 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-L249

Vulnerability details

Impact

High, inacurate fee calculations(even the slightest) would impact protocol earnings in the long run

Proof of Concept

For a given loan amount, $A$ , the amount claimable by the borrower $A_b$ is $Protocol fee$ , $F = A swapFeePercent$ $A_b = A - F = A(1 - swapFeePercent)$ ..1 And the Amount of credit sold $C$ is calculated as $Interest$ , $I = A ratePerTenor$ $Credit$ , $C = A + I = A(1 + ratePerTenor)$ ..2 the relationship between $A_b$, the amount to the borrower and $C$ ,the credit of the position can de derived From ..1, $A = A_b\frac{1}{(1 - swapFeePercent)}$ replacing $A$ in ..2 $C = A_b\frac{(1 + ratePerTenor)}{(1 - swapFeePercent)}$ & $A_b = C\frac{(1 - swapFeePercent)}{(1 + ratePerTenor)}$ ..3

When a borrower takes a loan offer on the market, via sellCreditMarket with params.exactAmountIn = false indicating that the provided amount(params.amount) is the exact amount they want to recieve(i.e. $A_b$), for simplicity we're assuming a new loan is being created i.e params.creditPositionId == RESERVED_ID. From here we see that the maxCredit is calculated using ..3 Screenshot 2024-06-28 181620 However, in the getCreditAmountIn function as seen here the cashAmountOut(i.e $A_b$) is used to calculate the fee rather than $A$. Screenshot 2024-06-28 182114

Proof of Code

Add test to test/local/actions/SellCreditMarket.t.sol

// /test/local/actions/SellCreditMarket.t.sol

import {UserView} from "@src/SizeViewData.sol";

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

        uint256 loanAmount = 100e6;
        uint256 tenor = 365 days;
        //credit
        uint256 futureValue = Math.mulDivUp(loanAmount, (PERCENT + 0.03e18), PERCENT);
        uint256 protocolFee = size.getSwapFee(loanAmount, tenor);
        //cashAmountOut
        uint256 cashAmountOut = loanAmount - protocolFee;

        UserView memory feeRecipientBefore = size.getUserView(feeRecipient);

        //exactAmountIn = false
        uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, cashAmountOut, 365 days, false);

        UserView memory feeRecipientAfter = size.getUserView(feeRecipient);
        uint256 feeRecieved = feeRecipientAfter.borrowATokenBalance - feeRecipientBefore.borrowATokenBalance;
        assertLt(feeRecieved, protocolFee); //feeRecieved is less than expected Fee
        assertEq(futureValue, size.getDebtPosition(debtPositionId).futureValue); //futureValue is correct
    }

Its worth noting that this issue doesn't exist with exactAmountIn = true and credit value is used to create loan.

Also ,the following existing test case passes but fails when we assertEq(futureValue, size.getDebtPosition(debtPositionId).futureValue); because the wrong amount($A$ instead of $A_b$) is used to create the loan.

expand to view code

```solidity // /test/local/actions/SellCreditMarket.t.sol function test_SellCreditMarket_sellCreditMarket_used_to_borrow() public { _deposit(alice, usdc, 200e6); _deposit(bob, weth, 100e18); _buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(365 days, 0.03e18)); Vars memory _before = _state(); uint256 amount = 100e6; uint256 tenor = 365 days; uint256 futureValue = Math.mulDivUp(amount, (PERCENT + 0.03e18), PERCENT); uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false); uint256 futureValueOpening = Math.mulDivUp(futureValue, size.riskConfig().crOpening, PERCENT); uint256 minimumCollateral = size.debtTokenAmountToCollateralTokenAmount(futureValueOpening); uint256 swapFee = size.getSwapFee(amount, tenor); Vars memory _after = _state(); assertGt(_before.bob.collateralTokenBalance, minimumCollateral); assertEq(_after.alice.borrowATokenBalance, _before.alice.borrowATokenBalance - amount - swapFee); assertEq(_after.bob.borrowATokenBalance, _before.bob.borrowATokenBalance + amount); assertEq(_after.variablePool.collateralTokenBalance, _before.variablePool.collateralTokenBalance); assertEq(_after.bob.debtBalance, size.getDebtPosition(debtPositionId).futureValue); -> assertEq(futureValue, size.getDebtPosition(debtPositionId).futureValue);//futureValue is incorrect } ```

Tools Used

Manual Review

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) {
        uint256 swapFeePercent = getSwapFeePercent(state, tenor);
+       uint256 loanAmount = Math.mulDivUp(cashAmountOut, PERCENT, PERCENT - swapFeePercent);

        // ...code...

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

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

Assessed type

Error

c4-judge commented 2 months ago

hansfriese marked the issue as satisfactory