sherlock-audit / 2023-03-taurus-judging

4 stars 0 forks source link

Ruhum - User can prevent liquidations by frontrunning the tx and slightly increasing their collateral #12

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

Ruhum

high

User can prevent liquidations by frontrunning the tx and slightly increasing their collateral

Summary

User can prevent liquidations by frontrunning the tx and decreasing their debt so that the liquidation transaction reverts.

Vulnerability Detail

In the liquidation transaction, the caller has to specify the amount of debt they want to liquidate, _debtAmount. The maximum value for that parameter is the total amount of debt the user holds:

    function liquidate(
        address _account,
        uint256 _debtAmount,
        uint256 _minExchangeRate
    ) external onlyLiquidator whenNotPaused updateReward(_account) returns (bool) {
        if (_debtAmount == 0) revert wrongLiquidationAmount();

        UserDetails memory accDetails = userDetails[_account];

        // Since Taurus accounts' debt continuously decreases, liquidators may pass in an arbitrarily large number in order to
        // request to liquidate the entire account.
        if (_debtAmount > accDetails.debt) {
            _debtAmount = accDetails.debt;
        }

        // Get total fee charged to the user for this liquidation. Collateral equal to (liquidated taurus debt value * feeMultiplier) will be deducted from the user's account.
        // This call reverts if the account is healthy or if the liquidation amount is too large.
        (uint256 collateralToLiquidate, uint256 liquidationSurcharge) = _calcLiquidation(
            accDetails.collateral,
            accDetails.debt,
            _debtAmount
        );

In _calcLiquidation(), the contract determines how much collateral to liquidate when _debtAmount is paid by the caller. In that function, there's a check that reverts if the caller tries to liquidate more than they are allowed to depending on the position's health.

    function _calcLiquidation(
        uint256 _accountCollateral,
        uint256 _accountDebt,
        uint256 _debtToLiquidate
    ) internal view returns (uint256 collateralToLiquidate, uint256 liquidationSurcharge) {
        // ... 

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

The goal is to get that if-clause to evaluate to true so that the transaction reverts. To modify your position's health you have two possibilities: either you increase your collateral or decrease your debt. So instead of preventing the liquidation by pushing your position to a healthy state, you only modify it slightly so that the caller's liquidation transaction reverts.

Given that Alice has:

The liquidator will probably use the maximum amount they can liquidate and call liquidate() with 23.07e18. Alice frontruns the liquidator's transaction and increases the collateral by 1. That will change the max liquidation amount to: $(1.3e18 100e18 - 101e18 1e18) / 1.3e18 = 22.3e18$.

That will cause _calcLiquidation() to revert because 23.07e18 > 22.3e18.

The actual amount of collateral to add or debt to decrease depends on the liquidation transaction. But, generally, you would expect the liquidator to liquidate as much as possible. Thus, you only have to slightly move the position to cause their transaction to revert

Impact

User can prevent liquidations by slightly modifying their position without putting it at a healthy state.

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

In _calcLiquidation() the function shouldn't revert if _debtToLiqudiate > _getMaxLiquidation(). Instead, just continue with the value _getMaxLiquidation() returns.

Sierraescape commented 1 year ago

Great write-up and recommendation.

Sierraescape commented 1 year ago

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

IAm0x52 commented 1 year ago

Escalate for 10 USDC

Should be medium since it can only prevent the liquidation temporarily. Similar issues have always been given medium severity such as Cooler:

https://github.com/sherlock-audit/2023-01-cooler-judging/issues/218

sherlock-admin commented 1 year ago

Escalate for 10 USDC

Should be medium since it can only prevent the liquidation temporarily. Similar issues have always been given medium severity such as Cooler:

https://github.com/sherlock-audit/2023-01-cooler-judging/issues/218

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 commented 1 year ago

Escalate for 10 USDC

Double down the escalation raise point the it only prevent the liquidation temporarily and hence it should be medium.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

Double down the escalation raise point the it only prevent the liquidation temporarily and hence it should be medium.

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 commented 1 year ago

Escalation accepted

Considering the impact of the attack is just preventing liquidations temporarily, considering this a valid medium

sherlock-admin commented 1 year ago

Escalation accepted

Considering the impact of the attack is just preventing liquidations temporarily, considering this a valid medium

This issue's escalations have been accepted!

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

MLON33 commented 1 year ago

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

Fixed here

IAm0x52 commented 1 year ago

Fix looks good. _calcLiquidation now returns a lower debt amount if user is liquidating more than max collateral