minhtrng - Oracle requests dont check if latest provider is still active

[sherlock-admin2]

minhtrng

medium

Oracle requests dont check if latest provider is still active

Summary

Requests in the oracle will always request from the current provider even if the latest provider is still active. This can cause a gap in price, when latest provider will not be updated but its most recent data will still be used.

Vulnerability Detail

Oracle.request always requests from global.current:

oracles[global.current].provider.request(account);

If the global.latest is not stale yet, its latest() data will be used (but not updated, due to the above):

function _handleLatest(
    OracleVersion memory currentOracleLatestVersion
) private view returns (OracleVersion memory latestVersion) {
    if (global.current == global.latest) return currentOracleLatestVersion;

    bool isLatestStale = _latestStale(currentOracleLatestVersion);
    latestVersion = isLatestStale ? currentOracleLatestVersion : oracles[global.latest].provider.latest();

    uint256 latestOracleTimestamp =
        uint256(isLatestStale ? oracles[global.current].timestamp : oracles[global.latest].timestamp);

    if (!isLatestStale && latestVersion.timestamp > latestOracleTimestamp)
        return at(latestOracleTimestamp);
    }

Impact

Stale price being used due to old provider still being active but receiving no udpate for the last granularity window that it is being active.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/c2f6141c231d3952898ea47793872cac69a1d2af/perennial-v2/packages/perennial-oracle/contracts/Oracle.sol#L38

Tool used

Manual Review

Recommendation

Request from oracles[global.latest].provider if block.timestamp is not past its ending time yet.

Duplicate of #42

[sherlock-admin]

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

141345 commented:

m

panprog commented:

invalid because no real explanation of the issue

[nevillehuang]

Escalate

After reviewing other watsons comments, agreed that it is a duplicate of #42 with same root cause but slight different impact described

[sherlock-admin2]

Escalate

After reviewing other watsons comments, agreed that it is a duplicate of #42 with same root cause but slight different impact described

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 want to add more details. This issue description is somewhat vague and hard to actually understand what it means. But from what I understand, after the new oracle provider is set:

• oracle.request() will go to new provider
• oracle.latest() will still return previous provider data
• when previous provider's current() is still active (>=block.timestamp), this can cause some issues (stale price?)

This is actually expected behavior and at most I see that it will request from both oracles, but will only use data from 1 oracle. For example: t=50: oracle.request (request to provider1 for timestamp = 100) t=55: oracle.update(provider2) t=60: oracle.request (request to provider2 for timestamp = 100)

once provider1 request is commited and provider2 request is commited, provider1.latest() will still be used and provider2.latest() will be ignored until provider2.latest().timestamp > last provider1 request timestamp (100).

This is the only issue I see: new provider will be requested, but the commited price for that request will be ignored (previous provider price used instead).

There is no impact in this and there is no funds loss in keeper fees - both providers will receive the fee for the keeper. So the issue should be invalid or low.

The report might also imply that in such case provider1 will keep returning latest() forever, but it's not true: if provider1 last request is already commited, then it will switch to provider2 immediately after provider2 latest().timestamp is higher than provider1 last request timestamp. If provider1 last request is not commited yet, then it just has to wait until keepers commit this last request and it will then automatically switch to provider2.

Either way, the report is either low or invalid.

[sherlock-admin2]

Escalate

I want to add more details. This issue description is somewhat vague and hard to actually understand what it means. But from what I understand, after the new oracle provider is set:

• oracle.request() will go to new provider
• oracle.latest() will still return previous provider data
• when previous provider's current() is still active (>=block.timestamp), this can cause some issues (stale price?)

This is actually expected behavior and at most I see that it will request from both oracles, but will only use data from 1 oracle. For example: t=50: oracle.request (request to provider1 for timestamp = 100) t=55: oracle.update(provider2) t=60: oracle.request (request to provider2 for timestamp = 100)

once provider1 request is commited and provider2 request is commited, provider1.latest() will still be used and provider2.latest() will be ignored until provider2.latest().timestamp > last provider1 request timestamp (100).

This is the only issue I see: new provider will be requested, but the commited price for that request will be ignored (previous provider price used instead).

There is no impact in this and there is no funds loss in keeper fees - both providers will receive the fee for the keeper. So the issue should be invalid or low.

The report might also imply that in such case provider1 will keep returning latest() forever, but it's not true: if provider1 last request is already commited, then it will switch to provider2 immediately after provider2 latest().timestamp is higher than provider1 last request timestamp. If provider1 last request is not commited yet, then it just has to wait until keepers commit this last request and it will then automatically switch to provider2.

Either way, the report is either low or invalid.

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.

[Minh-Trng]

The issue assumes that the granularity is not in effect and price updates are meant to happen multiple times within an epoch (which reflects the state of the code in scope)

So the issue describes the following: Epoch goes from time 100 to 200:

• provider1 gets updated at t=100
• owner updates shortly after
• provider1 will show the value that the keeper has commited for t=100 for the rest of the epoch

However, I admit that with the fix of #42 this is a non-issue. Due to this same root-cause enabling this issue, I would like to ask this issue to be marked as duplicate of #42. this shouldnt change anything regarding payouts, since I already have submitted a duplicate of it, this only affects my valid-invalid-ratio

[panprog]

The issue assumes that the granularity is not in effect and price updates are meant to happen multiple times within an epoch (which reflects the state of the code in scope)

Oh! Now I understand. So is it correct that what you mean is:

t=50: oracle.request (request to provider1, requested timestamp is 50, but oracles[1].timestamp = 100) t=55: oracle.update(provider2)

After t=100, provider1 is commited to requested timestamp=50, so provider1.latest().timestamp = 50

Now oracle.latest() will always return provider1.latest() because _latestStale returns false due to this line:

        if (uint256(oracles[global.latest].timestamp) > latestTimestamp) return false;

oracles[1].timestamp = 100 (last requested timestamp) latestTimestamp = provider1.latest().timestamp = 50 (last commited timestamp due to #42 )

So at this time oracle will keep returning stale price until provider1.commit is called by anyone (commit unrequested). This will allow the _latestStale to pass this line above and return true.

So while this issue is fixable by anyone (just commit unrequested once), it's unexpected and requires manual intervention to correctly switch the provider.

Hm.. I think this issue core reason is indeed #42, so it's basically #42 with a different (medium) impact, should be duplicate of it.

[141345]

suggestion: duplicate of https://github.com/sherlock-audit/2023-07-perennial-judging/issues/42

Since with the fix of https://github.com/sherlock-audit/2023-07-perennial-judging/issues/42 this one will be no problem. Based on Sherlock's duplication-rules, this issue can be grouped.

[hrishibhat]

Result: High Duplicate of #42 Considering this issue a duplicate of #42 based on the comments above.

[arjun-io]

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

[kbrizzle]

Extra context: this may be a duplicate of https://github.com/sherlock-audit/2023-07-perennial-judging/issues/42, as our fix for that one does cover most of this issue as well.

We've posted a separate fix here for the specific issue of double-requesting on both the latest and current oracle during the small period of an oracle update. This is a minor issue, but since there is an economic cost to requesting, it's not technically correct to request both in the old and new sub-oracles for the same timestamp.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted
• panprog: accepted