Wrong calculations causes the borrowers to owe more credit than necessary

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/AccountingLibrary.sol#L294

Vulnerability details

Impact

Wrong calculations causes the borrowers to owe more credit than necessary

Proof of Concept

Whenever a user has a borrow offer, lenders can use buyCreditMarket() to lend to that user. Then, there are different calculations taking place to compute the amount of credit the borrower owes, the amount of cash the lender should give out as well as the fees associated with that whole deal.

The lender can specify the exact amount of cash he wants to give and from it, the credit the borrower will owe is computed. The amount of cash specified, however, is not necessarily equal to the amount of cash the borrower will receive as there are fees associated with the deal. There are 2 types of fees in this deal - a fragmentation fee and a swap fee. The fragmentation fee is paid whenever a credit position used for a borrow is split. That fee is paid by whoever split the position so in our case, that would be the lender as he is the one lending out (the active party). The swap fee is paid by the cash receiver, this is the borrower in our case.

Here is one of the most important things in that whole lending/borrowing functionality:

if (params.exactAmountIn) {
            cashAmountIn = params.amount;
            (creditAmountOut, fees) = state.getCreditAmountOut({
                cashAmountIn: cashAmountIn,
                maxCashAmountIn: params.creditPositionId == RESERVED_ID
                    ? cashAmountIn
                    : Math.mulDivUp(creditPosition.credit, PERCENT, PERCENT + ratePerTenor),
                maxCredit: params.creditPositionId == RESERVED_ID
                    ? Math.mulDivDown(cashAmountIn, PERCENT + ratePerTenor, PERCENT)
                    : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        }

The lender has specified the amount of cash he wants to spend for the deal. From that, we compute the creditAmountOut and the fees using getCreditAmountOut(). Here is that function:

function getCreditAmountOut(
        State storage state,
        uint256 cashAmountIn,
        uint256 maxCashAmountIn,
        uint256 maxCredit,
        uint256 ratePerTenor,
        uint256 tenor
    ) internal view returns (uint256 creditAmountOut, uint256 fees) {
        if (cashAmountIn == maxCashAmountIn) {
            // no credit fractionalization

            creditAmountOut = maxCredit;
            fees = getSwapFee(state, cashAmountIn, tenor);
        } else if (cashAmountIn < maxCashAmountIn) {
            // credit fractionalization

            if (state.feeConfig.fragmentationFee > cashAmountIn) {
                revert Errors.NOT_ENOUGH_CASH(state.feeConfig.fragmentationFee, cashAmountIn);
            }

            uint256 netCashAmountIn = cashAmountIn - state.feeConfig.fragmentationFee;

            creditAmountOut = Math.mulDivDown(netCashAmountIn, PERCENT + ratePerTenor, PERCENT);
            fees = getSwapFee(state, netCashAmountIn, tenor) + state.feeConfig.fragmentationFee;
        } else {
            revert Errors.NOT_ENOUGH_CREDIT(maxCashAmountIn, cashAmountIn);
        }
    }

We will focus our attention on the else if block, that is the code responsible for split positions (that will pretty much always be the case we end up in if the lender specifies exactAmountIn as true and the borrow happens with a credit position instead of doing a simple borrow).

Here is that else if block:

else if (cashAmountIn < maxCashAmountIn) {
            // credit fractionalization

            if (state.feeConfig.fragmentationFee > cashAmountIn) {
                revert Errors.NOT_ENOUGH_CASH(state.feeConfig.fragmentationFee, cashAmountIn);
            }

            uint256 netCashAmountIn = cashAmountIn - state.feeConfig.fragmentationFee;

            creditAmountOut = Math.mulDivDown(netCashAmountIn, PERCENT + ratePerTenor, PERCENT);
            fees = getSwapFee(state, netCashAmountIn, tenor) + state.feeConfig.fragmentationFee;
        }

Since the lender split the position, there is now the fragmentationFee involved. There is also a swapFee. Let's do some calculations to see what the computed values would be. Let's imagine that cashAmountIn is 1000e6 and ratePerTenor is 1e17 (10%). The fragmentationFee is 5e6 and the swapFee is 0.005e18 (both according to the deployment scripts). Then, we have the following computations:

1. netCashAmountIn will equal $1000e6 - 5e6 = 995e6$
2. creditAmountOut is equal $995e6 * 1.1e18 / 1e18 = 1094500000$
3. fees are equal to $4975000 + 5e6 = 9975000$ (tenor is a year in our example so the swap fee will be equal to the yearly swap fee)
4. Then, this is what the borrower receives:

   state.data.borrowAToken.transferFrom(msg.sender, borrower, cashAmountIn - fees);

5. That equals $1000e6 - 9975000 = 990025000$, thus the credit he would have to pay is $990025000 * 1.1e18 / 1e18 + 4975000 = 1094002500$ (10% of the amount borrowed + the swap fee he owes as he is the cash receiver)

If we compare that to the creditAmountOut we computed earlier, we can see that the creditAmountOut computed by the system is larger than the actual one it should be by 497500 (equal to around 0.50\$). The borrower has to pay 0.50\$ more than what should actually be required. While that is not a significant amount, that would occur every single time whenever a user lends to a borrower with exactAmountIn as true leading to tens and hundreds of thousands of losses for borrowers overtime. Furthermore, the amount would be larger for larger deals, thus this issue should be of high severity.

Now, to explain why is that. creditAmountOut is calculated using netCashAmountIn however we can see that netCashAmountIn does not actually include the swap fee which is also deducted from the borrower's received cash. If we calculate the netCashAmountIn as follows:

uint256 netCashAmountIn = cashAmountIn - (state.feeConfig.fragmentationFee + swapFee);

Then, we would properly calculate the amount owed (I am aware that the swap fee is calculated using the netCashAmountIn so that creates a little bit of a paradox with the current code, it would need some refactoring).

Paste the following POC into BuyCreditMarket.t.sol (simple POC to show that creditAmountOut is equal to my calculations, the rest if for you to properly calculate and see the inaccuracy):

    function testBorrowersPayMoreThanNecessary() public {
        _setPrice(1e18);
        _deposit(alice, weth, 1500e18);
        _deposit(alice, usdc, 2000e6);
        _deposit(bob, usdc, 1500e6);
        _deposit(candy, weth, 3000e18);

        _buyCreditLimit(alice, type(uint256).max, YieldCurveHelper.pointCurve(365 days, 0));
        uint256 debtPositionIdAliceCandy = _sellCreditMarket(candy, alice, RESERVED_ID, 2000e6, 365 days, true);
        uint256 creditPositionIdAliceCandy = size.getCreditPositionIdsByDebtPositionId(debtPositionIdAliceCandy)[0];

        assertEq(size.getCreditPosition(creditPositionIdAliceCandy).credit, 2000e6);

        _sellCreditLimit(alice, YieldCurveHelper.pointCurve(365 days, 1e17));

        uint256 debtPositionId = _buyCreditMarket(bob, alice, creditPositionIdAliceCandy, 1000e6, 365 days, true);
        uint256 creditPositionId = size.getCreditPositionIdsByDebtPositionId(debtPositionId)[1];

        assertEq(size.getCreditPosition(creditPositionId).credit, 1094500000);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Refactor the code based on my last few lines of the issue before the POC.

Assessed type

Math

[aviggiano]

The credit seller pays for swap fees

[hansfriese]

It works as intended. In your example, the borrower should pay credits of netCashAmountIn * 110% = 995e6 * 1.1. After that, the swapFee(= 4975000) will be taken from netCashAmountIn.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid