sherlock-admin commented 1 year ago

panprog

high

Oracle request timestamp and pending position timestamp mismatch can make most position updates invalid

Summary

When a new pending position is added, its timestamp is set to currentTimestamp returned by oracle's status function, which is a timestamp at certain granularities rounding up into the future, which means that most of the time it's greater than block.timestamp. However, when request is called for the oracle, the request timestamp is set to block.timestamp. Due to this mismatch, when the oracle price is commited, it is commited with request's timestamp, but when the position is settled, it tries to read the price at position's timestamp, which is a different time. As such, if the oracle price is commited for each request, it's still easily possible that all pending positions will have invalid oracle versions, completely breaking the protocol's functionality.

Vulnerability Detail

An example of what happens exactly:

PythOracle granularity is set to 100.
User opens position at timestamp = 101. Pending position is stored with timestamp = 200 (because PythOracle returns currentTimestamp = 200)
At the same time oracle.request() is called, which stores 101 (current timestamp) into versionList
User calls oracle.commitRequested(), which stores current price into _prices[101]
Later when that pending position is settled, it requests oracle.at(200) which doesn't have a price set (is invalid).

The same will happen to all pending positions - so most of them will easily be invalid, which will completely break the protocol and cause all kinds of problems due to pending positions being invalid and not updating profit and loss properly.

Impact

The most straightforward impact is unexpectedly long position commit times and possible funds loss due to this, if the oracle commit flow is the normal expected flow (only commit requested versions). For example: T=1: User A requests to open position long = 1. Position timestamp = 100. Oracle request timestamp = 1 T=15: Oracle commits requested version at timestamp = 1, price = $100. T=10010: User B requests to open position. Position timestamp = 10100. Oracle request timestamp = 10010 T=10025: Oracle commits requested version at timestamp = 10010, price = $110.

User A expects to be filled at price close to $100. However, he's only filled when the next user trades after him, which happens much later than expected with a very different price ($110), so User A has lost $10 unexpectedly. Basically, each user will only be settled when the next user trades. In quiet markets this can lead to very long settlement times and very bad prices for users.

User A, however, can notice these long waiting times and can fix it by voluntary commiting non-requested versions. For example, he can commit at T=120 and be filled with the correct price. However, this will mean that all commits must be made non-requested, thus they will not be rewarded with the keeper fees. So the user will pay keeper fees when trading, but will also be forced to lose gas fees for oracle commits, so either broken and long waiting times, or broken oracle non-rewarded updates: both are high impacts.

Another impact is completely broken internal accounting due to a lot of invalid oracle versions. There is a different bug reported by me about desync of global and local positions during invalid oracles. This bug, when coupled with the desync of global and local positions, will lead to catastrophic consequences and complete breakage of accounting of collateral, bank run and loss of funds for users. Scenario of what can (and will) happen: User B has active open position maker=2 with collateral = 100 T=99: User A opens long=1 with collateral=100: update(0,1,0,100) (pending position timestamp = 100) T=101: User A decides to close: update(0,0,0,0) (pending position timestamp = 200) T=130: Oracle commited for timestamp=110, price = $100 (user A position at timestamp = 100 is invalid) T=150: User B settles: update(2,0,0,0) T=220: Oracle commited for timestamp=205, price = $90 after settlement of user A and user B: user A will have collateral = $100 (local pending position long = 1 at timestamp = 100 will be invalidated and ignored) user B will have collateral = $110 (global pending position long = 1 will be current at timestamp 110 and accumulate pnl from timestamp 110 to timestamp=205)

So total deposit of both users is $100 + $100 = $200 Total collateral in the end: $100 + $110 = $210 But protocol only has $200 in funds, so users will be unable to withdraw everything, which can cause bank run and loss of funds for the last user.

Such situations will happen all the time by themselves due to lots of invalid oracle versions, so this will mess up accounting completely.

For the details of this bug, you can refer to my other report.

Code Snippet

Oracle status() returns timestamp which is in the future.

Oracle status() returns timestamp directly from current provider's status(): https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/Oracle.sol#L47

PythOracle status() timestamp is taken from current(), which in turn returns current() from PythFactory: https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L101

PythFactory current() returns timestamp which is granulated into the future using ceilDiv, which rounds up: https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythFactory.sol#L76

Pending position's timestamp is taken from oracle status().

context.currentTimestamp is set to timestamp from oracle.status(): https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L312 https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L575

New pending positions (global and local) timestamp is set to context.currentTimestamp: https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L267-L269

And request() from oracle is done at the same time: https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L284

PythOracle request() stores block.timestamp in the request list (called versionList) (not current() timestamp): https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L77-L81
PythOracle commitRequested() sets price at versionList timestamp (i.e. block.timestamp at the time request() was made)

versionToCommit is stored request's timestamp: https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L135

The commit price is stored at the versionToCommit timestamp: https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L154 https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L202-L203

Tool used

Manual Review

Recommendation

Make timestamp of pending positions and timestamp of oracle request match. Record current() as a timestamp for the request():

 function request(address) external onlyAuthorized { 
     uint nextTimestamp = current();
     if (versionList.length == 0 || versionList[versionList.length - 1] < nextTimestamp) { 
         versionList.push(nextTimestamp);
     } 
 }

sherlock-admin commented 1 year ago

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

141345 commented:

h

arjun-io commented 1 year ago

Fixed via: https://github.com/equilibria-xyz/perennial-v2/pull/57

jacksanford1 commented 1 year ago

From WatchPug:

Fixed.

With the updated code, when there are multiple new orders within the same hour, while the keeper only gets 1 unit of the keeper fee, each order will pay for 1 unit of keeper fee.

A more fair approach would be either:

Only the first request pays the settlement/keeper fee. Splitting the settlement fee among the orders.

arjun-io commented 1 year ago

From WatchPug:

Fixed.

With the updated code, when there are multiple new orders within the same hour, while the keeper only gets 1 unit of the keeper fee, each order will pay for 1 unit of keeper fee.

A more fair approach would be either:

Only the first request pays the settlement/keeper fee. Splitting the settlement fee among the orders.

While true, we choose specifically not to go the route of the suggestion due to complexity versus benefit.

Oracle versions (granularity) in V2 are on the order of 5-30 seconds, so it’s less likely that many orders will happen in the same market at the same version. To add this, we’d either have to only charge the first request of a version, or pro-rate the amount charged retroactively – either adding significant code complexity, or weird economic situations. Finally, we have plans for future updates which will incur per-request cost (settling a user’s account for them when the keeper posts the new price) so we’d like to keep this open.

sherlock-audit / 2023-07-perennial-judging

panprog - Oracle request timestamp and pending position timestamp mismatch can make most position updates invalid #42