Malicious lenders can force borrowers into fragmenting their credit positions

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Malicious lenders can force borrowers into fragmenting their credit positions

Proof of Concept

Users can create a loan offer that other users might use to borrow tokens. The borrower might use a credit position he has in order for him to take the borrow. The borrower can also specify whether he wants the amount parameter he put as an input to equal cashAmountOut or creditAmountIn by using the exactAmountIn boolean. If he chooses that he wants amount to equal cashAmountOut (by setting the boolean as false), a malicious lender can force him into fragmenting his credit position and paying extra fees.

This is the code that is responsible for handling the case when the user has specified that he wants the amount parameter to equal cashAmountOut:

else {
            cashAmountOut = params.amount;

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

RESERVED_ID is used when doing a simple borrow, this is not the case here as the borrower wants to use a credit position he already has. If the cashAmountOut and maxCashAmountOut, then the borrower will have to pay a fragmentation fee. Even if he carefully inputted data that would not fragment him, the lender can still force him into fragmentation. That is because maxCashAmountOut gets computed like this:

Math.mulDivDown(creditPosition.credit, PERCENT - state.getSwapFeePercent(tenor), PERCENT + ratePerTenor)

The malicious lender has control over 1 of the values here and that is ratePerTenor. The ratePerTenor value is the interest the borrower has to pay, in simplest terms. The lender can frontrun and change his yield curve and thus, change the ratePerTenor value which makes maxCashAmountOut a different value than the user expected. There is this check in validateSellCreditMarket():

if (apr > params.maxAPR) {
            revert Errors.APR_GREATER_THAN_MAX_APR(apr, params.maxAPR);
        }

However, this check does not prevent the issue. Even if the borrower specified the maxAPR parameter exactly equal to the expected apr, the lender can actually decrease the APR on his yield curve. This will make the check pass as it would make apr even lower than expected and definitely lower than the maxAPR specified by the user. A user can decrease his APR slightly and that would make ratePerTenor not much smaller than what it would usually be (100000000000000000 vs 99884000000000000 according to my POC below, a difference of slightly higher than 1.001%).

For the POC, I have used event emissions as I couldn't find a way to see the values we need to see to confirm the issue without significantly changing the code.

Create a Log event for uint256 in SellCreditMarket.sol. Then, add the following event emissions in SellCreditMarket::executeSellCreditMarket() (second and third line in the else block in the if statement, the same way as I did in the code below, first event is equal to maxCashAmountOut and the second one is cashAmountOut):

if (params.exactAmountIn) {
            creditAmountIn = params.amount;
            (cashAmountOut, fees) = state.getCashAmountOut({
                creditAmountIn: creditAmountIn,
                maxCredit: params.creditPositionId == RESERVED_ID ? creditAmountIn : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        } else {
            cashAmountOut = params.amount;
            emit Log(Math.mulDivDown(creditPosition.credit, PERCENT - state.getSwapFeePercent(tenor), PERCENT + ratePerTenor));
            emit Log(cashAmountOut);
            (creditAmountIn, fees) = state.getCreditAmountIn({
                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
            });
        }

Then, add these 2 event emissions right at the end of the function (the amount the borrower receives and the amount he has to repay with interest):

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

Then, paste the following POC in SellCreditMarket.t.sol (use -vvvv to see the events, toggle the line where Alice changes the APR by making it a comment and back into code to see the scenario of Alice frontrunning and not frontrunning play out):

function testLenderCanForceBorrowerIntoFragmentation() public {
        _setPrice(1e18);
        _updateConfig("borrowATokenCap", type(uint256).max);
        _deposit(alice, weth, 200000e18);
        _deposit(alice, usdc, 200000e6);
        _deposit(bob, usdc, 200000e6);
        _deposit(candy, weth, 200000e18);

        uint256 num = 1e17;
        _buyCreditLimit(bob, type(uint256).max, [int256(num), int256(num)], [uint256(100 days), uint256(800 days)]);
        _buyCreditLimit(alice, type(uint256).max, [int256(num), int256(num)], [uint256(100 days), uint256(800 days)]);

        uint256 debtPositionId = _sellCreditMarket(candy, bob, RESERVED_ID, 100000e6, 365 days, true);
        uint256 creditPositionId = size.getCreditPositionIdsByDebtPositionId(debtPositionId)[0];

        _buyCreditLimit(alice, type(uint256).max, [int256(9.9884e16), int256(9.9884e16)], [uint256(100 days), uint256(800 days)]); // Alice frontruns and changes the APRs, comment out to see what would happen without Alice frontrunning
        _sellCreditMarket(bob, alice, creditPositionId, 90454545454, 365 days, false);

        /*
            Emitted Events without frontrunning:
                cashAmountOut = 90454545454
                maxCashAmountOut = 90454545454 (Both values are equal, no fragmentation)
                cashAmountOut (after all the computations, amount transferred from lender to borrower) = 90454545454
                creditAmountIn (amount the borrower has to repay with interest) = 100000000000

            Emitted Events with frontrunning:
                cashAmountOut = 90464085303
                maxCashAmountOut = 90454545454 (Values are not equal, fragmentation)
                cashAmountOut (after all the computations, amount transferred from lender to borrower) = 90454545454
                creditAmountIn (amount the borrower has to repay with interest) = 99994981601

                Lender receives 1.00005018651% less credit, negligent amount but fragmented the borrower
        */
    }

Tools Used

Manual Review

Recommended Mitigation Steps

You could add a minAPR value as well or change maxAPR to expectedAPR and use ==, that way the APR can't change and this scenario won't be possible.

Assessed type

Other

[c4-judge]

hansfriese marked the issue as not a duplicate

[hansfriese]

Duplicate of #146.

[c4-judge]

hansfriese marked the issue as duplicate of #146

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid