panprog - PythOracle commit() function doesn't require (nor stores) pyth price publish timestamp to be after the previous commit's publish timestamp, which makes it possible to manipulate price to unfairly liquidate users and possible stealing protocol funds

[sherlock-admin]

panprog

medium

PythOracle commit() function doesn't require (nor stores) pyth price publish timestamp to be after the previous commit's publish timestamp, which makes it possible to manipulate price to unfairly liquidate users and possible stealing protocol funds

Summary

PythOracle allows any user to commit non-requested oracle version. However, it doesn't verify pyth price publish timestamp to be in order (like commitRequested does). This makes it possible to commit prices out of order, potentially leading to price manipulations allowing to unfairly liquidate users or steal funds from the protocol.

Vulnerability Detail

PythOracle commitRequested() has the following check:
https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L138-L139

However, commit() doesn't have the same check. This allows malicious user to commit prices out of order, which can potentially lead to price manipulation attacks and unfair liquidation of users or loss of protocol funds.

For example, the following scenario is possible:
Timestamp = 100: Alice requests to open 1 ETH long position with $10 collateral
Timestamp = 113: pyth price = $980
Timestamp = 114: pyth price = $990
Timestamp = 115: pyth price = $1000
Timestamp = 116: Keeper commits requested price = $1000 (with publish timestamp = 115)
Timestamp = 117: Malicious user Bob commits oracle version 101 with price = $980 (publish timestamp = 113) and immediately liquidates Alice.

Even though the current price is $1000, Alice is liquidated using the price which is earlier than the price when Alice position is opened, which is unfair liquidation. The other more complex scenarios are also possible for malicious Bob to liquidate itself to steal protocol funds.

Impact

Unfair liquidation as described in the scenario above or possible loss of protocol funds.

Code Snippet

Tool used

Manual Review

Recommendation

Add publish time check and store publish time in PythOracle.commit():

    function commit(uint256 oracleVersion, bytes calldata updateData) external payable {
        // Must be before the next requested version to commit, if it exists
        // Otherwise, try to commit it as the next request version to commit
        if (versionList.length > nextVersionIndexToCommit && oracleVersion >= versionList[nextVersionIndexToCommit]) {
            commitRequested(nextVersionIndexToCommit, updateData);
            return;
        }

+        if (pythPrice.publishTime <= _lastCommittedPublishTime) revert PythOracleNonIncreasingPublishTimes();
+        _lastCommittedPublishTime = pythPrice.publishTime;

        PythStructs.Price memory pythPrice = _validateAndGetPrice(oracleVersion, updateData);

[sherlock-admin]

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

141345 commented:

m

[Emedudu]

Escalate

This is an invalid issue.

Malicious user Bob commits oracle version 101 with price = $980 (publish timestamp = 113) and immediately liquidates Alice.

How is Bob malicious? He committed the more recent price.

Timestamp = 116: Keeper commits requested price = $1000 (with publish timestamp = 115)

If this price is used(which was requested at timestamp 100, when current timestamp is 115), it means that protocol is using a 15 seconds stale price.
It is better(and an expected behaviour) when Bob commits a more recent price(price at timestamp 101) because now, Protocol is using a less stale price(14 seconds stale).
So it is fair to liquidate Alice because even though the most recent price does not favour her, that's the rules.

[sherlock-admin2]

Escalate

This is an invalid issue.

Malicious user Bob commits oracle version 101 with price = $980 (publish timestamp = 113) and immediately liquidates Alice.

How is Bob malicious? He committed the more recent price.

Timestamp = 116: Keeper commits requested price = $1000 (with publish timestamp = 115)

If this price is used(which was requested at timestamp 100, when current timestamp is 115), it means that protocol is using a 15 seconds stale price.
It is better(and an expected behaviour) when Bob commits a more recent price(price at timestamp 101) because now, Protocol is using a less stale price(14 seconds stale).
So it is fair to liquidate Alice because even though the most recent price does not favour her, that's the rules.

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

This is a valid issue. Let me go into example in more details to explain. A few terms to better understand process:

• oracle version - the timestamp keeper commits the price to. It doesn't mean the price at this timestamp. It means the price used to settle positions at this timestamp.
• pyth publish timestamp - the timestamp of a price published and signed in the pyth network (actual time the price is observed). publish timestamp must come 12-15 seconds after oracle version (these are example settings in perennial tests).

Timestamp = 100: Alice requests to open 1 ETH long position with $10 collateral. Oracle request is created (with oracle version = 100, meaning only publish prices in 112..115 range can be commited)
Timestamp = 113: pyth publish price = $980
Timestamp = 114: pyth publish price = $990
Timestamp = 115: pyth publish price = $1000
Timestamp = 116: Keeper commits requested price = $1000 (oracle version = 100, publish timestamp = 115)
Timestamp = 117: Malicious user Bob commits (unrequested) oracle version = 101 with price = $980, publish timestamp = 113 and immediately liquidates Alice.

What happens is:
oracle version = 100 has publish timestamp = 115
oracle version = 101 has publish timestamp = 113

This breaks invariant that publish timestamps must increase when oracle version increases.

So first position is opened using price $1000 (with publish timestamp = 115), then it is liquidated using the publish timestamp = 113, which is an earlier price than the price of when position was opened, which is unfair liquidation.

[sherlock-admin2]

Escalate

This is a valid issue. Let me go into example in more details to explain. A few terms to better understand process:

• oracle version - the timestamp keeper commits the price to. It doesn't mean the price at this timestamp. It means the price used to settle positions at this timestamp.
• pyth publish timestamp - the timestamp of a price published and signed in the pyth network (actual time the price is observed). publish timestamp must come 12-15 seconds after oracle version (these are example settings in perennial tests).

Timestamp = 100: Alice requests to open 1 ETH long position with $10 collateral. Oracle request is created (with oracle version = 100, meaning only publish prices in 112..115 range can be commited)
Timestamp = 113: pyth publish price = $980
Timestamp = 114: pyth publish price = $990
Timestamp = 115: pyth publish price = $1000
Timestamp = 116: Keeper commits requested price = $1000 (oracle version = 100, publish timestamp = 115)
Timestamp = 117: Malicious user Bob commits (unrequested) oracle version = 101 with price = $980, publish timestamp = 113 and immediately liquidates Alice.

What happens is:
oracle version = 100 has publish timestamp = 115
oracle version = 101 has publish timestamp = 113

This breaks invariant that publish timestamps must increase when oracle version increases.

So first position is opened using price $1000 (with publish timestamp = 115), then it is liquidated using the publish timestamp = 113, which is an earlier price than the price of when position was opened, which is unfair liquidation.

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.

[Emedudu]

This assumption is predicated on the idea that there can be a rapid and significant change in the ETH/USD price, such as a $10 change as demonstrated in the example. Can we substantiate this assumption with historical data, demonstrating that the price of ETH can indeed fluctuate by $10 within a second, or even within 3 seconds?

Besides, it's crucial for the protocol to utilize the price from the most recent oracle version. This underscores the importance for users to maintain healthy positions. Referring to the example Watson provided, Alice should not let her position teeter on the brink of liquidation.

So again, Bob is not acting maliciously. He is diligently fulfilling his role as a keeper, which involves updating oracle versions. He happened upon a careless trader, Alice, with an unhealthy position and executed a liquidation. This action benefits the protocol by reducing the number of unhealthy positions.

[Minh-Trng]

While it is true that commitRequested enforces publish time to increase monotonically, while commit doesnt, the example shows how Alice has at one point in those 3 seconds been be below her liquidation point, so its absolutely fine (and beneficial for the protocol) for her to be liquidated.

Bob has done a better job at playing the keeper than the other keeper

[panprog]

While it is true that commitRequested enforces publish time to increase monotonically, while commit doesnt, the example shows how Alice has at one point in those 3 seconds been be below her liquidation point, so its absolutely fine (and beneficial for the protocol) for her to be liquidated.

No, this is not true. If the price of $980 was commited first, then Alice position would be opened at a price of $980 and will not be liquidatable. If Alice was opened at a price of $990, then she also won't be liquidatable.

This assumption is predicated on the idea that there can be a rapid and significant change in the ETH/USD price, such as a $10 change as demonstrated in the example. Can we substantiate this assumption with historical data, demonstrating that the price of ETH can indeed fluctuate by $10 within a second, or even within 3 seconds?

1. Even 0.01% of a price change is enough to liquidate the account. Yes, the user may be careless with leverage, however this doesn't make this liquidation fair. I chose more extreme numbers just to better demonstrate the scenario.
2. 3 seconds is just an example, it's a protocol setting and can be higher
3. Here is an extreme example to better understand why such liquidation is unfair. If the publish timestamp window is, say, 200 seconds long and the price starts dropping sharply from $1000 to $980. User expects the price to keep dropping, so he opens a short position at close to max leverage (so that a price move of $20 will get it liquidated). The price keeps falling sharply to $960. The position is opened at a price of $960 (end of 200 seconds interval) and immediately liquidated at a price of $980 (start of 200 seconds interval). I don't think any reasonable user expects his position to be liquidated using prices which were well before the opening of the position.

So even with 3-seconds time interval and much smaller price change magnitude, the same scenario is still possible and is still unfair. The published (observed) prices must come in the same order they're observed from pyth, they can not go in a random order.

[Emedudu]

Even 0.01% of a price change is enough to liquidate the account. Yes, the user may be careless with leverage, however this doesn't make this liquidation fair. I chose more extreme numbers just to better demonstrate the scenario.

Why should a user leave his position to be liquidatable when there is a price change of 0.01%? This shows that the position is indeed unhealthy. One thing about oracles is that they return approximate values, and have little deviation from other oracles' values. So no reasonable trader will make her position liquidatable on a 0.01% price change.

3 seconds is just an example, it's a protocol setting and can be higher

From what I can see, they are CONSTANTS: MIN_VALID_TIME_AFTER_VERSION and MAX_VALID_TIME_AFTER_VERSION

Here is an extreme example to better understand why such liquidation is unfair. If the publish timestamp window is, say, 200 seconds long and the price starts dropping sharply from $1000 to $980. User expects the price to keep dropping, so he opens a short position at close to max leverage (so that a price move of $20 will get it liquidated). The price keeps falling sharply to $960. The position is opened at a price of $960 (end of 200 seconds interval) and immediately liquidated at a price of $980 (start of 200 seconds interval). I don't think any reasonable user expects his position to be liquidated using prices which were well before the opening of the position

This seems unrealistic

[Minh-Trng]

A core assumption of this issue is that a user opens a position close to the liquidation price and/or in a highly volatile period without enough buffer till liquidation, so that even a price deviation within 2 seconds would liquidate them. this is clearly a user mistake rather than a protocol error. And thats why a medium severity is imo not justified.

A user does not know the oracle prices 12-15 seconds into the future, so they would not choose which price to be filled at and be mindful of having enough margin. The example might as well go like this:

Timestamp = 113: pyth publish price = $980
Timestamp = 114: pyth publish price = $990
Timestamp = 115: pyth publish price = $1000
Timestamp = 116: pyth publish price = $990
Timestamp = 117: pyth publish price = $980

In this case Alice might get filled at t=115 and get liquidated at t=117 and there is no one to blame but her.

To sum up, the issue is hypothetically possible but requires a careless user AND enough volatility within 2 seconds time span

[panprog]

To sum up, the issue is hypothetically possible but requires a careless user AND enough volatility within 2 seconds time span

Yes, it's unlikely but possible, thus should be a valid medium.

[Emedudu]

Severity should be LOW because likelihood is low and impact is low

[panprog]

Severity should be LOW because likelihood is low and impact is low

Impact is unfair liquidation, so it's high. For example, if the price starts dropping quickly -> every second the price will be less than or equal to previous second's price, in such situation opening short position at max leverage can be valid strategy for the user expecting continuaton of the price fall, and being liquidated at earlier price is also extremely unfair.

[arjun-io]

Fixed: equilibria-xyz/perennial-v2#80

[141345]

Medium seems appropriate.

As per the discussion, it is plausible to commit prices out of order and cause loss, but with several conditions:

• limited time span
• high volatility
• the position is already on the border of liquidation

[hrishibhat]

Result:
Medium
Unique
Considering this issue a valid medium based on the above discussion and the comment from Lead Judge

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• panprog: accepted
• Emedudu: rejected

[jacksanford1]

From WatchPug:

Fixed