n33k - Market: DoS when stuffed with pending protected positions

[sherlock-admin]

n33k

high

Market: DoS when stuffed with pending protected positions

Summary

Market has no limit on how many protected position updates can be added into pending position update queue. When settling these pending position updates, transations can OOG revert. This fully bricks the protocol and funds will be locked forever.

Vulnerability Detail

In _invariant, there is a limit on the number of pending position updates. But for protected position updates, _invariant returns early and does not trigger this check.

    function _invariant(
        Context memory context,
        address account,
        Order memory newOrder,
        Fixed6 collateral,
        bool protected
    ) private view {
        ....

        if (protected) return; // The following invariants do not apply to protected position updates (liquidations)
        ....
            if (
            context.global.currentId > context.global.latestId + context.marketParameter.maxPendingGlobal ||
            context.local.currentId > context.local.latestId + context.marketParameter.maxPendingLocal
        ) revert MarketExceedsPendingIdLimitError();
        ....
    }

After the _invariant check, the postion updates will be added into pending position queues.

        _invariant(context, account, newOrder, collateral, protected);

        // store
        _pendingPosition[context.global.currentId].store(context.currentPosition.global);
        _pendingPositions[account][context.local.currentId].store(context.currentPosition.local);

When the protocol enters next oracle version, the global pending queue _pendingPosition will be settled in a loop.

    function _settle(Context memory context, address account) private {
        ....
        // settle
        while (
            context.global.currentId != context.global.latestId &&
            (nextPosition = _pendingPosition[context.global.latestId + 1].read()).ready(context.latestVersion)
        ) _processPositionGlobal(context, context.global.latestId + 1, nextPosition);

The OOG revert happens if there are too many pending position updates.

This revert will happend on every update calls because they all need to settle this _pendingPosition before update.

    function update(
        address account,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool protect
    ) external nonReentrant whenNotPaused {
        Context memory context = _loadContext(account);
        _settle(context, account);
        _update(context, account, newMaker, newLong, newShort, collateral, protect);
        _saveContext(context, account);
    }

Impact

The protocol will be fully unfunctional and funds will be locked. There will be no recover to this DoS.

A malicious user can tigger this intentionally at very low cost. Alternatively, this can occur during a volatile market period when there are massive liquidations.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L497

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L505-L508

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L333-L337

Tool used

Manual Review

Recommendation

Either or both,

1. Limit the number of pending protected position updates can be queued in _invariant.
2. Limit the number of global pending protected postions can be settled in _settle.

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

141345 commented:

x

panprog commented:

medium because it's actually very hard to create many pending positions and at the same time liquidate: new pending position is only created once time advances by the granularity. If previous pending positions are not still settled, this means there were no oracle commits, this means that market price didn't change and accounts can be liquidated the whole time.

[syjcnss]

Escalate

This should be valid high.

Because the impact is critical and should never happen even under edge cases.

This DoS does not require an attacker. The protocol could enter such state spontaneously if conditions are met.

[sherlock-admin2]

Escalate

This should be valid high.

Because the impact is critical and should never happen even under edge cases.

This DoS does not require an attacker. The protocol could enter such state spontaneously if conditions are met.

You've created a valid escalation!

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.

[panprog]

Escalate

I agree that this issue is valid. However, it should be medium, not high, because it's actually quite unlikely to enter such state:

1. New pending position (both global and local) is only created when oracle.current() timestamp changes. Since it's in granularities, this happens every granularity seconds.
2. Whenever an oracle version is commited, all pending positions up to that commit are settled. This means that in order to enter such state - there should be a large number of consecutive requested and uncommited oracle versions.
3. Since such state can only happen when there is no oracle commit for a long time, this means that all this time the same price is used for liquidation, so all accounts potentially liquidatable are liquidatable for this entire time, and for such state to happen, at least 1 account should be liquidated in each different granularity timestamp, which is unlikely - liquidations are more likely to happen all in the first 1-2 oracle versions.

Example of what has to happen to cause this bug: -- no accounts liquidatable t=120: oracle commits new price for oracle version = 100 For example, the price change is high and causes mass liquidations t=130: liquidation (new pending position at timestamp 200, 1 pending position) t=140: liquidation (same pending position at timestamp 200, still 1 pending position) t=150: liquidation (same) ... t=210: liquidation (new pending position at timestamp 200, 2 pending positions) t=220: liquidation (same, 2 pending) ... (no oracle commit for timestamp=200) ... t=310: liquidation (new pending position at timestamp 300, 3 pending positions) ... (no oracle commits for neither timestamps 200, nor 300) t=410: liquidation (4 pending) ... (still no oracle commits) t=510: liquidation (5 pending) ... (still no oracle commits) t=610: liquidation (6 pending -> exceeds pending positions limit)

So if pending limit is 5, there should be:

• no oracle commits for at least 4 * granularity seconds
• at least 1 position change request each granularity window
• at least 1 liquidation after 4 granularity seconds have passed - remember that all 4 granularity seconds all positions are liquidatable, so they're much more likely to happen earlier rather than after such long period of time.

This will allow to exceed the pending limit. However, the real impact (protocol funds get stuck) will likely occur much later after even more oracle versions and liquidations, because it's unlikely that pending limit is set at the edge of transaction max gas limit.

So given conditions which are quite rare to occur, but possible, this should be medium.

[sherlock-admin2]

Escalate

I agree that this issue is valid. However, it should be medium, not high, because it's actually quite unlikely to enter such state:

1. New pending position (both global and local) is only created when oracle.current() timestamp changes. Since it's in granularities, this happens every granularity seconds.
2. Whenever an oracle version is commited, all pending positions up to that commit are settled. This means that in order to enter such state - there should be a large number of consecutive requested and uncommited oracle versions.
3. Since such state can only happen when there is no oracle commit for a long time, this means that all this time the same price is used for liquidation, so all accounts potentially liquidatable are liquidatable for this entire time, and for such state to happen, at least 1 account should be liquidated in each different granularity timestamp, which is unlikely - liquidations are more likely to happen all in the first 1-2 oracle versions.

Example of what has to happen to cause this bug: -- no accounts liquidatable t=120: oracle commits new price for oracle version = 100 For example, the price change is high and causes mass liquidations t=130: liquidation (new pending position at timestamp 200, 1 pending position) t=140: liquidation (same pending position at timestamp 200, still 1 pending position) t=150: liquidation (same) ... t=210: liquidation (new pending position at timestamp 200, 2 pending positions) t=220: liquidation (same, 2 pending) ... (no oracle commit for timestamp=200) ... t=310: liquidation (new pending position at timestamp 300, 3 pending positions) ... (no oracle commits for neither timestamps 200, nor 300) t=410: liquidation (4 pending) ... (still no oracle commits) t=510: liquidation (5 pending) ... (still no oracle commits) t=610: liquidation (6 pending -> exceeds pending positions limit)

So if pending limit is 5, there should be:

• no oracle commits for at least 4 * granularity seconds
• at least 1 position change request each granularity window
• at least 1 liquidation after 4 granularity seconds have passed - remember that all 4 granularity seconds all positions are liquidatable, so they're much more likely to happen earlier rather than after such long period of time.

This will allow to exceed the pending limit. However, the real impact (protocol funds get stuck) will likely occur much later after even more oracle versions and liquidations, because it's unlikely that pending limit is set at the edge of transaction max gas limit.

So given conditions which are quite rare to occur, but possible, this should be medium.

You've created a valid escalation!

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.

[syjcnss]

I agree with @panprog on his 1,2,3 explainations on why it's actually quite unlikely to enter such state.

I think it should be high because there is NO attacker/special setup(i.e., pending limit is set at the edge of transaction max gas limit in @panprog's comment) required for this locking of fund to happen.

[panprog]

I agree that this state can happen by itself without special setup or attacker, but the likelihood is very low, that's why it's medium. Refer to sherlock rules on how to identify high and medium issues, high issues do not require limiting external conditions, medium issues require certain external conditions or specific states - that's exactly the case for this issue.

https://docs.sherlock.xyz/audits/judging/judging#how-to-identify-a-high-issue

How to identify a high issue:

• Definite loss of funds without limiting external conditions.
• Breaks core contract functionality, rendering the protocol/contract useless (should not be easily replaced without loss of funds) and definitely causes significant loss of funds.
• Significant loss of funds/large profit for the attacker at a minimal cost.

How to identify a medium issue:

• Causes a loss of funds but requires certain external conditions or specific states.
• Breaks core contract functionality, rendering the contract useless (should not be easily replaced without loss of funds) or leading to unknown potential exploits/loss of funds. Eg: Unable to remove malicious user/collateral from the contract.
• A material loss of funds, no/minimal profit for the attacker at a considerable cost

[syjcnss]

Don't think medium issues require certain external conditions or specific states applies to this one.

The conditions are that untrusted 3rd-party liquidators&keepers behave non-ideally(slow liquidation&price commit) under massive liquidations which should be considered met. And once is all it takes to lock fund.

[arjun-io]

This is a valid state that can occur so in our view this is a valid issue. However, we agree that there is a very low likelihood that this can occur with correct parameters set for the market

[syjcnss]

The likelihood is low but the result is untolerable.

The protocol is relying on untrusted 3rd parties to prevent this from happenning. The probability increases under congested network conditions and low liquidator/keeper performance.

Despite the severity of this issue in the contest I suggest to have a fix.

[141345]

The condition for this to be valid is strict. With long granularity, and large number of pending positions. For a malicious user, there is cost of the position and potential risk. The likelihood for it to happen by user self is even lower. Seems medium severity according to the criteria.

[hrishibhat]

Result: Medium Unique Agree with the above Escalation and the comment on why this issue should be valid medium. https://github.com/sherlock-audit/2023-07-perennial-judging/issues/94#issuecomment-1697453789

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• syjcnss: accepted
• panprog: accepted

[jacksanford1]

From WatchPug:

Acknowledged