howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Liquidate.sol#L95-L113

Vulnerability details

Impact

If a loan is overdue, it is subject to liquidation, upon liquidation the liquidator gets debt in collateral + some liquidator profit. The protocol in it's turn claims some protocol profit from that liquidation. The way the protocol's profit is being calculated is wrong by design, as it fully depends on the remaining collateral. Let's take it step by step:

// split the remaining collateral between the protocol and the borrower, capped by the crLiquidation
uint256 collateralRemainder = assignedCollateral - liquidatorProfitCollateralToken;

// cap the collateral remainder to the liquidation collateral ratio
//   otherwise, the split for non-underwater overdue loans could be too much
uint256 collateralRemainderCap = Math.mulDivDown(debtInCollateralToken, state.riskConfig.crLiquidation, PERCENT);

collateralRemainder = Math.min(collateralRemainder, collateralRemainderCap);

protocolProfitCollateralToken = Math.mulDivDown( collateralRemainder, collateralProtocolPercent, PERCENT);

The protocol calculates the remaining loan's assigned collateral after subtracting the debt + liquidator's profit
The remainder is being capped at 130% of the debt
A percent of the remainder is being cut as the protocol's profit

So in theory, if the assignedCollateral changes the protocol's profit changes.

For example: Bob has 5 WETH collateral and 2 loans (3k USDC and 700 USDC). Alice has 5 WETH collateral and 1 3k USDC loan. Assuming both 3k loans are overdue, when liquidating both loans, Bob will end up paying less collateral than Alice, i.e. the protocol earning less rewards for the same loan. This is because the assigned collateral for Bob's 3k loan is < Alice's 3k loan.

On the other hand, the protocol allows users to lend themselves, so users can use this as a way to lose less collateral.

As a result, the protocol gets different liquidation rewards from loans with the same debt, depending on the number of active loans of the borrowers'.

Proof of Concept

The below assumes that another reported issue is fixed, to overcome this, replace the value of liquidatorReward in executeLiquidate, with the following:

uint256 liquidatorReward = Math.min(
    assignedCollateral - debtInCollateralToken,
    Math.mulDivUp(
        debtInCollateralToken,
        state.feeConfig.liquidationRewardPercent,
        PERCENT
    )
);

The below shows 2 tests, with the same collateral, the first having 2 loans, and the second having 1 loan. They show how the protocol's profit is inconsistent.

uint256 nextCreditPositionId = type(uint256).max / 2;
uint256 nextDebtPositionId = 0;

function setUpLiquidationStuff() public returns (uint256, uint256) {
    _deposit(alice, weth, 5e18);
    _deposit(bob, usdc, 20_000e6);

    int256[] memory aprs = new int256[](1);
    uint256[] memory tenors = new uint256[](1);
    uint256[] memory marketRateMultipliers = new uint256[](1);

    aprs[0] = 0.2e18;
    tenors[0] = 365 days;
    marketRateMultipliers[0] = 0;

    // Alice creates a limit order
    vm.prank(alice);
    size.sellCreditLimit(
        SellCreditLimitParams({
            curveRelativeTime: YieldCurve({
                tenors: tenors,
                marketRateMultipliers: marketRateMultipliers,
                aprs: aprs
            })
        })
    );

    // Bob lends to Alice
    vm.prank(bob);
    size.buyCreditMarket(
        BuyCreditMarketParams({
            borrower: alice,
            creditPositionId: type(uint256).max,
            amount: 3_000e6,
            tenor: 365 days,
            deadline: block.timestamp,
            minAPR: 0,
            exactAmountIn: true
        })
    );

    return (nextCreditPositionId++, nextDebtPositionId++);
}

function test_liquidate__fake_loan() public {
    (, uint256 debtPositionId) = setUpLiquidationStuff();

    // Alice lends to herself
    vm.prank(alice);
    size.buyCreditMarket(
        BuyCreditMarketParams({
            borrower: alice,
            creditPositionId: type(uint256).max,
            amount: 700e6,
            tenor: 365 days,
            deadline: block.timestamp,
            minAPR: 0,
            exactAmountIn: true
        })
    );

    // Loan is overdue
    vm.warp(block.timestamp + 365 days + 1);

    uint256 feeRecipientCollaterBalanceBefore = size
        .data()
        .collateralToken
        .balanceOf(feeRecipient);

    // Bob liquidates Alice's loan
    vm.prank(bob);
    size.liquidate(
        LiquidateParams({
            debtPositionId: debtPositionId,
            minimumCollateralProfit: 0
        })
    );

    // Protocol got around 0.01 WETH
    assertEq(
        (size.data().collateralToken.balanceOf(feeRecipient) -
            feeRecipientCollaterBalanceBefore) / 1e16,
        1
    );
}

function test_liquidate__no_fake_loan() public {
    (, uint256 debtPositionId) = setUpLiquidationStuff();

    // Loan is overdue
    vm.warp(block.timestamp + 365 days + 1);

    uint256 feeRecipientCollaterBalanceBefore = size
        .data()
        .collateralToken
        .balanceOf(feeRecipient);

    // Bob liquidates Alice's loan
    vm.prank(bob);
    size.liquidate(
        LiquidateParams({
            debtPositionId: debtPositionId,
            minimumCollateralProfit: 0
        })
    );

    // Protocol got around 0.02 WETH
    assertEq(
        (size.data().collateralToken.balanceOf(feeRecipient) -
            feeRecipientCollaterBalanceBefore) / 1e16,
        2
    );
}

Tools Used

Manual review

Recommended Mitigation Steps

Instead of fully relying on the collateral's remainder when calculating the protocol's liquidation profit, rely on the debt itself, and take a percentage of that debt, while having the remainder as a cap. Something similar to:

protocolProfitCollateralToken = Math.min(
    assignedCollateral - liquidatorProfitCollateralToken,
    Math.mulDivUp(
        debtInCollateralToken,
        state.feeConfig.liquidationRewardPercent,
        PERCENT
    )
);

This way, the protocol's profit will always be consistent for loans having the same debt, regardless of the number of active loans.

Assessed type

Math

aviggiano commented 4 months ago

This is by design

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Invalid

0xAlix2 commented 3 months ago

Hello @hansfriese, thanks for your time.

Assuming crLiquidation is 130%...

I agree this is by design, which could be wrong, but with the current implementation the protocol is taking min(130% of the debt, collateral remainder), the collateral remainder was always less than the other, so a borrower could open some dummy loans (by having himself the lender and the borrower), which reduces the assignedCollateral of that specific loan and force the protocol to earn less profit, the above POC demonstrates this. So this allows any borrower to manipulate the protocol's profit.

As a result, the impact of this has been reduced a lot by the fix of https://github.com/code-423n4/2024-06-size-findings/issues/140, while showing a different higher impact, leading to protocol loss rather than borrowers'. I believe this should be a valid separate high issue (looking into how you're judging issues that affect the protocol).

hansfriese commented 3 months ago

I understand your point but it's an intended design. Please check #74 for details. Also, in your comment, it's impossible to open dummy loans once the borrower is liquidatable due to the health check.

0xAlix2 commented 3 months ago

Hello @hansfriese, thanks for your reply!

Also, in your comment, it's impossible to open dummy loans once the borrower is liquidatable due to the health check.

Right, but a user can minimize the protocol's profit by opening dummy loans if he has an overdue liquidatable loan (without having the user underwater). I agree that this is by design after #140's fix (not while having the min(130% of the debt, collateral remainder)), so what I'm trying to show is an issue that was somewhat "unintentionally" fixed by #140. Even though the user is still able to do so after the fix, the surface of such scenarios is hugely reduced.