spyrosonic10 - Account can not be liquidated when price fall by 99%.

[sherlock-admin]

spyrosonic10

high

Account can not be liquidated when price fall by 99%.

Summary

Liquidation fails when price fall by 99%.

Vulnerability Detail

_calcLiquidation() method has logic related to liquidations. This method calculate total liquidation discount, collateral to liquidate and liquidation surcharge. All these calculations looks okay in normal scenarios but there is an edge case when liquidation fails if price crashes by 99% or more. In such scenario collateralToLiquidateWithoutDiscount will be very large and calculated liquidation surcharge becomes greater than collateralToLiquidate

uint256 collateralToLiquidateWithoutDiscount = (_debtToLiquidate * (10 ** decimals)) / price;
collateralToLiquidate = (collateralToLiquidateWithoutDiscount * totalLiquidationDiscount) / Constants.PRECISION;
if (collateralToLiquidate > _accountCollateral) {
            collateralToLiquidate = _accountCollateral;
}
uint256 liquidationSurcharge = (collateralToLiquidateWithoutDiscount * LIQUIDATION_SURCHARGE) / Constants.PRECISION

Contract revert from below line hence liquidation will fail in this scenario.

uint256 collateralToLiquidator = collateralToLiquidate - liquidationSurcharge;

Impact

Liquidation fails when price crash by 99% or more. Expected behaviour is that liquidation should be successful in all scenarios.

Code Snippet

Block of code that has bug.
https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L396-L422

Below is POC that prove failed liquidation.

it("should revert liquidation if an account is unhealthy and price crashed 99%", async () => {
        // Assume price is crashed 99%
        await glpOracle.updatePrice(PRECISION.mul(1).div(100));
        // check if the account is underwater
        const health = await gmxVault.getAccountHealth(user.address);
        expect(health).eq(false);

        // Check the liquidation amount
        const liqAmt = await gmxVault.getMaxLiquidation(user.address);

        // Mint some TAU to the liquidator and approve vault to spend it
        await mintHelper(liqAmt, liquidator.address);
        await tau.connect(liquidator).approve(gmxVault.address, liqAmt);
        const totalTauSupply = await tau.totalSupply();

        // liquidation will fail
        const tx = gmxVault.connect(liquidator).liquidate(user.address, liqAmt, 0);
        // reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)
        await expect(tx).revertedWithPanic(0x11);
      });

PS: This test goes in 00_GmxYieldAdapter.ts and inside describe("Liquidate", async () => { block defined at line 269

Tool used

Manual Review

Recommendation

Presently liquidation surcharge is calculated on collateralToLiquidateWithoutDiscount. Project team may want to reconsider this logic and calculate surcharge on collateralToLiquidate instead of collateralToLiquidateWithoutDiscount. This will be business decision but easy fix

Another option is you may want to calculate surcharge on Math.min(collateralToLiquidate, collateralToLiquidateWithoutDiscount).

        uint256 collateralToTakeSurchargeOn = Math.min(collateralToLiquidate, collateralToLiquidateWithoutDiscount);
        uint256 liquidationSurcharge = (collateralToTakeSurchargeOn * LIQUIDATION_SURCHARGE) / Constants.PRECISION;
        return (collateralToLiquidate, liquidationSurcharge);

[Sierraescape]

https://github.com/protokol/taurus-contracts/pull/122

[hrishibhat]

Since this is an edge case for the given price fall resulting in reverting liquidations,
Considering this as a valid medium

[IAm0x52]

Escalate for 10 USDC

This is the same root cause as #89 that the liquidation surcharge is calculated based on the uncapped amount. This is another symptom of that same underlying problem, so it should be a dupe of #89

[sherlock-admin]

Escalate for 10 USDC

This is the same root cause as #89 that the liquidation surcharge is calculated based on the uncapped amount. This is another symptom of that same underlying problem, so it should be a dupe of #89

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[spyrosonic10]

Escalate for 10 USDC

I do not agree with escalation raised above. This issue is about failure of liquidation when price fall by x%. This finding is an edge case where it does impact all underwater accounts so it is fair to say that it impact whole protocol. Root cause and impact both are different in this issue compare to #89 so this is definitely not a duplicate of #89.

[sherlock-admin]

Escalate for 10 USDC

I do not agree with escalation raised above. This issue is about failure of liquidation when price fall by x%. This finding is an edge case where it does impact all underwater accounts so it is fair to say that it impact whole protocol. Root cause and impact both are different in this issue compare to #89 so this is definitely not a duplicate of #89.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Accepting the first escalation.
After further internal discussion, both the outcomes originate out of the same root cause of using collateralToLiquidateWithoutDiscount to calculate liquidationSurcharge. While one mentions increase in the fee the other instance increases to cause underflow.
Considering #89 a duplicate of this issue.

[sherlock-admin]

Escalation accepted

Accepting the first escalation.
After further internal discussion, both the outcomes originate out of the same root cause of using collateralToLiquidateWithoutDiscount to calculate liquidationSurcharge. While one mentions increase in the fee the other instance increases to cause underflow.
Considering #89 a duplicate of this issue.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[MLON33]

https://github.com/protokol/taurus-contracts/pull/122

Fixed here

[IAm0x52]

Fix looks good. Surcharge is now taken on actual amount liquidated rather than undercounted amount