sherlock-audit / 2023-07-perennial-judging

2 stars 1 forks source link

minhtrng - Oracle requests are not limited to granularity #170

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

minhtrng

medium

Oracle requests are not limited to granularity

Summary

Requests of oracle prices are not limited to the granularity set by the PythFactory. This will update the price feed much more frequently and cause the protocol to pay more rewards to keepers than intended.

Vulnerability Detail

Oracle.request requests price data from the provider everytime it is called:

//Oracle
function request(address account) external onlyAuthorized {
    (OracleVersion memory latestVersion, uint256 currentTimestamp) = oracles[global.current].provider.status();

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

// PythOracle (provider)
function request(address) external onlyAuthorized {
    if (versionList.length == 0 || versionList[versionList.length - 1] != block.timestamp) {
        versionList.push(block.timestamp); 
    }

The PythFactory defines a _granularity which is meant to limit the price updates to one update per granularity window. This intent has been confirmed by the sponsor, but the implementation has been missing in the request flow.

Impact

Keepers update the price too frequently at cost of the protocol which has to pay keeper rewards

Code Snippet

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

Tool used

Manual Review

Recommendation

Either in Oracle.request or PythOracle.request perform a check if there has been a request already within the current granularity window and return early if so.

Duplicate of #42

sherlock-admin commented 1 year ago

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

141345 commented:

l

sherlock-admin2 commented 12 months ago

Escalate

I have explicitly verified with one of the sponsors that this is in fact an issue. They were already aware of it during their own testing, but since this has not been declared as an officially known issue at the start of the contest, and the impact is a continuous loss of protocol funds (having to pay the keeper fee up to every block), this should match the criteria for a medium

this issue is also partially related to #153, as it would enable the described fee drainage to happen at every block rather than once per granularity window.

You've deleted an escalation for this issue.
panprog commented 12 months ago

Escalate

This is a clear duplicate of #42

sherlock-admin2 commented 12 months ago

Escalate

This is a clear duplicate of #42

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 commented 12 months ago

I concede that its the same root cause, I pulled back my escalation

hrishibhat commented 12 months ago

Result: High Duplicate of #42

sherlock-admin2 commented 12 months ago

Escalations have been resolved successfully!

Escalation status: