AkshaySrivastav - Liquidators can prevent users from making their positions healthy during an unpause

[sherlock-admin]

AkshaySrivastav

medium

Liquidators can prevent users from making their positions healthy during an unpause

Summary

The Perennial protocol has a paused state in which all operations are paused. The protocol can be unpaused by privileged accounts. But when this unpause happens the liquidators can frontrun and liquidate user positions before those users get a chance to make their positions healthy.

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/collateral/Collateral.sol#L108-L135

Vulnerability Detail

The pauser address can pause the perennial protocol using the Controller.updatePaused function. Once the protocol is paused all these operations cannot be done by users:

• Open/close/modify current make or take positions on a product.
• Deposit or withdraw collateral.
• Liquidate under-collateralized positions.

Though, real prices from oracles will surely move up or down during this paused period. If the oracle prices go down, the users won't be allowed to add more collateral to their positions or close their positions. Hence their positions will get under-collateralized (based upon real prices).

Once the protocol is unpaused the liquidators can front-run most users and liquidate their positions. Most users will not get a chance to make their position healthy.

This results in loss of funds for the users.

Perennial also has the concept of settlement delay. Any position opened/closed at oracle version n is settled at oracle version n + 1. This also alleviates the frontrunning liquidation issue. While validating an account's health before liquidation, the protocol only considers the current maintenance requirement for the account (not the next). This makes users more prone to getting front-runned and liquidated.

Ref: audit finding

Impact

By front-running any collateral deposit or position closure of a legitimate user which became under-collateralized during the paused state, the liquidator can unfairly liquidate user positions and collect liquidation profit as soon as the protocol is unpaused. This causes loss of funds to the user.

Code Snippet

Collateral

    function depositTo(address account, IProduct product, UFixed18 amount)
    external
    nonReentrant
    notPaused
    notZeroAddress(account)
    isProduct(product)
    collateralInvariant(account, product)
    {
        _products[product].creditAccount(account, amount);
        token.pull(msg.sender, amount);

        emit Deposit(account, product, amount);
    }

Product

    function closeTakeFor(address account, UFixed18 amount)
        public
        nonReentrant
        notPaused
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account)
        closeInvariant(account)
        liquidationInvariant(account)
    {
        _closeTake(account, amount);
    }

    function closeMakeFor(address account, UFixed18 amount)
        public
        nonReentrant
        notPaused
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account)
        takerInvariant
        closeInvariant(account)
        liquidationInvariant(account)
    {
        _closeMake(account, amount);
    }

Tool used

Manual Review

Recommendation

Consider adding a grace period after unpausing during which liquidation remains blocked to allow users to avoid unfair liquidation by closing their positions or supplying additional collateral.

[akshaysrivastav]

Escalate for 10 USDC

I think this issue should be considered as valid. As per the comment here, the protocol team can disagree with the mitigation suggested in #168 but the validity of issue should be accepted. The report clearly shows how a protocol owner actions (pause) will result in unfair liquidations causing loss of funds to users.

Also it is unlikely that on unpause human users will be able to secure their positions before MEV/liquidation bots capture the available profit. Hence the loss is certain.

For reference, a similar issue was consider valid in the recent Notional V3 contest. Maintaining a consistent valid/invalid classification standard will be ideal here.

[sherlock-admin]

Escalate for 10 USDC

I think this issue should be considered as valid. As per the comment here, the protocol team can disagree with the mitigation suggested in #168 but the validity of issue should be accepted. The report clearly shows how a protocol owner actions (pause) will result in unfair liquidations causing loss of funds to users.

Also it is unlikely that on unpause human users will be able to secure their positions before MEV/liquidation bots capture the available profit. Hence the loss is certain.

For reference, a similar issue was consider valid in the recent Notional V3 contest. Maintaining a consistent valid/invalid classification standard will be ideal here.

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.

[KenzoAgada]

Sherlock:

• At the main issue, SolidityATL - Vulnerable unpause logic flow lead to unfair forced liquidations #168, I wrote that "This can be considered as an un-mandatory "idea for improvement", but I think that the risk is there so this is a reasonable submission, and if I recall correctly, was accepted by Sherlock judges in the past."
• The sponsor disputed the issue and replied that "By introducing a time delay to allow traders to optionally increase their collateral to meet maintenance requirements the Product runs the risk of going into further shortfall if the traders opt to not increase their margin. The fairest option for both liquidators and other traders in the protocol is to require that traders are paying attention and self-liquidate (to retain the fee) if they so chose.".
• I accepted the sponsor's POV and closed the issue as a design choice.
• However here the escalator says that even if the sponsor would not like to implement the mitigation, the validity of the issue should be accepted.
• This issue has been accepted recently in Notional and Blueberry. But unlike here, there the sponsor confirmed the issues.

I think the watson raises a valid point. Even if the sponsor chose not to allow this as a design choice, the issue itself is there, was accepted by Sherlock in the past, and therefore should probably be accepted here as well.
I think restoring it to medium is fair.

[jacksanford1]

Seems like a valid issue if true. The recommended fix's acceptance/rejection is not relevant to the issue itself.

This is a temporary freezing (pause) induced by the protocol team, but it can cause the protocol to enter a state where on unpause many users will lose their funds to MEV (due to being liquidatable) before the user can salvage the position potentially.

Borderline Low issue but we can make it a Medium since it can result in a significant loss of funds for many users at the same time.

[jacksanford1]

Result:
Medium
Has Duplicates
Reasoning can be found in the previous message.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• akshaysrivastav: accepted