roguereddwarf - BaseVault: liquidationSurcharge amount is too high if collateralToLiquidate gets capped

[sherlock-admin]

roguereddwarf

medium

BaseVault: liquidationSurcharge amount is too high if collateralToLiquidate gets capped

Summary

According to the comment explaining the LIQUIDATION_SURCHARGE percentage, it should be a percentage of the liquidated amount:
Link

// 2% of the liquidated amount is sent to the FeeSplitter to fund protocol stability
// and disincentivize self-liquidation

So if say $100 of collateral is liquidated, the surcharge should be $2. I will show how this is not true in some cases. I.e. the surcharge may be higher (even a lot higher).

I had a discussion about this with the sponsor and it was assessed that this is unintended behavior. The liquidation surcharge percentage breaks in the case that I will show.

Vulnerability Detail

Let's have a look at how the liquidation surcharge is calculated:
Link

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


        // Revert if requested liquidation amount is greater than allowed
        if (
            _debtToLiquidate >
            _getMaxLiquidation(_accountCollateral, _accountDebt, price, decimals, totalLiquidationDiscount)
        ) revert wrongLiquidationAmount();


        return (
            collateralToLiquidate,
            (collateralToLiquidateWithoutDiscount * LIQUIDATION_SURCHARGE) / Constants.PRECISION
        );

The collateralToLiquidate is calculated by multiplying collateralToLiquidateWithoutDiscount by totalLiquidationDiscount.
And then it is capped at _accountCollateral. The cap is necessary because it is not possible to remove more collateral from the loan than there is left.

As part of the return statement, the liquidationSurcharge return value is calculated as (collateralToLiquidateWithoutDiscount * LIQUIDATION_SURCHARGE) / Constants.PRECISION.

It becomes clear that this can result in a higher liquidation surcharge than expected:
Let collateralToLiquidateWithoutDiscount=$100, totalLiquidationDiscount=120% (20% percent discount on the collateral).
Then collateralToLiquidate=$100*120%=$120 and liquidationSurcharge=$100*2%=$2.

If however the collateralToLiquidate gets capped at say $50, then liquidationSurcharge is still calculated in the same way.
But the surcharge percentage would actually be $2/$50=4% instead of the expected 2%.

Impact

The liquidationSurcharge that is charged to the liquidator by subtracting it from the collateral he receives may not be the 2% that it is supposed to. This means that the liquidator needs to pay a higher liquidation fee in some cases and ends up with less profit.

Code Snippet

https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L396-L422

https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L432-L445

Tool used

Manual Review

Recommendation

It is not obvious how this issue can be fixed because the surcharge percentage is not well defined for when collateralToLiquidate > _accountCollateral and the collateralToLiquidate is capped.

After all if this is the case then what should collateralToLiquidateWithoutDiscount be?
Should the amount by which collateralToLiquidate is reduced due to the cap be taken from the discount or from collateralToLiquidate AND the discount.

In other words it is not clear what value to use to calculate the liquidation surcharge.

This has been discussed with the sponsor and they should look into different ways to modify the calculation by first firmly establishing how the liquidation surcharge is intended to behave.

Duplicate of #61

[Sierraescape]

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

[hrishibhat]

Considering this issue a valid duplicate of #61 as the underlying issue is the same.