Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reversion of getSqrtPrice Function Due to getPriceNoOlderThan Oracle Call #97

Closed
howlbot-integration bot opened this issue Jun 17, 2024 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-31 edited-by-warden 🤖_09_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PriceFeed.sol#L48

Vulnerability details

Impact

The issue with the getSqrtPrice function is that it reverts when calling getPriceNoOlderThan from the Pyth oracle for various price feeds on multiple chains, including Optimism and Base ,Arbitrum.

Proof of Concept

Consider a scenario where a user tries to execute a trade involving the WBTC/USD pair on the Optimism chain , Base chain. The getSqrtPrice function is called to fetch the price data, but it reverts due to the inability to retrieve data from the Pyth oracle.

  1. User Action: A user submits a transaction to trade WBTC for USD.

  2. Function Call: The getSqrtPrice function is called to fetch the price data.

  3. Oracle Query: The function calls getPriceNoOlderThan on the Pyth oracle with the WBTC/USD price ID and an age of 300 seconds.

  4. Error: The Pyth oracle reverts, indicating that it cannot provide the requested data.

  5. Transaction Revert: The entire transaction reverts, leading to wasted gas fees and a failed trade.

Here is the corrected text with proper grammar:

Please test the following by yourself. You can find the Price ID of WBTC/USD, which is 0xc9d8b075a5c69303365ae23633d4e085199bf5c520a3b90fed1322a0342ffc33.

  1. Go to the price feed contract address on Optimism and Base.

  2. Go to the contract section, select "Read as Proxy," and then call the getPriceNoOlderThan function with the price ID and 300.

You will observe that it always reverts.

Using getPriceNoOlderThan to retrieve prices is not a good practice. Some of the price IDs revert on certain chains, like USDC/USD, which reverts on Optimism and Base, and DAI/USDC, which reverts on all supported chains. ETH/USD also reverts on Optimism and Base.

Tools Used

Manual Review

Recommended Mitigation Steps

Use getPriceUnsafe instead of getPriceNoOlderThan.

Assessed type

Oracle

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_09_group AI based duplicate group recommendation bug Something isn't working duplicate-31 edited-by-warden sufficient quality report This report is of sufficient quality labels Jun 17, 2024
howlbot-integration bot added a commit that referenced this issue Jun 17, 2024
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Jun 28, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as unsatisfactory:
Invalid

@0xAbhay
Copy link

0xAbhay commented Jun 29, 2024

Hey @alex-ppg thanks for judging

The uint256 private constant VALID_TIME_PERIOD = 5 * 60; period is the same for everyone, and it does not work consistently for every pair. For example, USDC/USD reverts on Optimism and Base, DAI/USDC reverts on all supported chains, and ETH/USD reverts on Optimism and Base.

Additionally, the WBTC/USD pair on the Optimism chain always reverts when calling getPriceNoOlderThan. These are the pairs where the issue can arise, even if you change the time for specific pairs.

Technical Analysis

  1. Function Behavior Overview:

    • Describe the purpose of getSqrtPrice and its reliance on getPriceNoOlderThan from the Pyth oracle to fetch price data.
    • Emphasize that the function reverts when attempting to retrieve certain price IDs, such as WBTC/USD (0xc9d8b075a5c69303365ae23633d4e085199bf5c520a3b90fed1322a0342ffc33), due to oracle failures.
  2. Constant VALID_TIME_PERIOD:

    • Highlight the presence of uint256 private constant VALID_TIME_PERIOD = 5 * 60;, which sets a fixed time period of 5 minutes for price validity across all price feeds.
    • Explain that while this period is consistent, the ability of the Pyth oracle to provide timely data within this period varies significantly based on specific price pairs and across different chains.
  3. Specific Problematic Pairs:

    • Detail specific pairs that consistently cause reverts:
      -WBTC/USD. pair on the Optimism chain
      • USDC/USD: Reverts on Optimism and Base chains.
      • DAI/USDC: Reverts on all supported chains.
      • ETH/USD: Reverts on Optimism and Base chains.

Impact

  1. Transactional Failure Consequences:
    • Describe the impact of getSqrtPrice reverting during a transaction attempting to trade WBTC for USD:
      • User Action: User initiates a transaction to trade WBTC for USD.
      • Function Call: getSqrtPrice is called to fetch the WBTC/USD price data.
      • Oracle Query: getPriceNoOlderThan is invoked with the WBTC/USD price ID and an age of 300 seconds.
      • Error: Pyth oracle reverts, indicating inability to provide the required data.
      • Transaction Revert: Entire transaction fails, resulting in wasted gas fees and a failed trade.

Proof of Concept

  1. Demonstration Steps:
    • Provide step-by-step instructions to replicate the issue:
      • Navigate to the price feed contract on Optimism and Base chains.
      • Access the contract section and select "Read as Proxy."
      • Call getPriceNoOlderThan with the WBTC/USD price ID (0xc9d8b075a5c69303365ae23633d4e085199bf5c520a3b90fed1322a0342ffc33) and an age of 300 seconds.
    • Emphasize that the function call consistently reverts due to oracle issues with specific price IDs.

Conclusion

The compelling case is that the behavior of getSqrtPrice in PriceFeed.sol constitutes an issue.

#23 finding described the same issue please review thank you

@alex-ppg
Copy link

alex-ppg commented Jul 4, 2024

Updating the price of an external oracle is not the responsibility of the protocol integrating with it, and adopted practices involve off-chain as well as router-level workarounds. I do not believe that we can consider failure to update an external provider's price measurement an HM vulnerability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-31 edited-by-warden 🤖_09_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants