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

Inconsistent Price Data Due to Sequencer Downtime and Expired Oracle Prices #94

Closed
howlbot-integration bot opened this issue Jun 17, 2024 · 8 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-69 🤖_178_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

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

Vulnerability details

Impact

Sequencer Downtime: If the sequencer is down, the price data may be stale or incorrect, leading to inaccurate calculations and potential financial losses.

Oracle Price Expiry: Using expired price data can result in incorrect price feeds, causing potential mispricing and financial discrepancies.

Heartbeat Consistency: Inconsistent heartbeats can lead to varying data freshness, causing unreliable price feeds across different tokens.

Proof of Concept

Sequencer Downtime: Assume the sequencer is down for 10 minutes.
getSqrtPrice() fetches data without checking sequencer status.
The price data is stale, leading to incorrect price calculations

Expired Oracle Data: Using expired oracle data can lead to incorrect price calculations, which can result in financial losses and unreliable contract behavior.

Price feed A has a heartbeat of 1 minute, and price feed B has a heartbeat of 5 minutes.
getSqrtPrice() fetches data from both feeds.
The data freshness is inconsistent, leading to unreliable price calculations.

Read here

Tools Used

Manual Review

Recommended Mitigation Steps

wWhile creating a PriceFeed, ensure we have a heartbeat interval for each price feed.

--function createPriceFeed(address quotePrice, bytes32 priceId, uint256 decimalsDiff , uint256 hearbeatInterval) external returns (address)
++    function createPriceFeed(address quotePrice, bytes32 priceId, uint256 decimalsDiff , uint16 hearbeatInterval) external returns (address) {
        // deploy new pair for the every pair
--   PriceFeed priceFeed = new PriceFeed(quotePrice, _pyth, priceId, decimalsDiff);
++   PriceFeed priceFeed = new PriceFeed(quotePrice, _pyth, priceId, decimalsDiff, hearbeatInterval);

        emit PriceFeedCreated(quotePrice, priceId, decimalsDiff, address(priceFeed));

        return address(priceFeed);
    }

Now implement a check against the heartbeat interval.

 function getSqrtPrice() external view returns (uint256 sqrtPrice) {
--        (, int256 quoteAnswer,,,) = AggregatorV3Interface(_quotePriceFeed).latestRoundData();
++ (, int256 quoteAnswer,,updatedAt,) = AggregatorV3Interface(_quotePriceFeed).latestRoundData();

++  require(  block.timestamp - updatedAt <= heartbeatInterval,
++  "ORACLE_HEARTBEAT_FAILED");

    IPyth.Price memory basePrice = IPyth(_pyth).getPriceNoOlderThan(_priceId, VALID_TIME_PERIOD);

    require(basePrice.expo == -8, "INVALID_EXP");

    require(quoteAnswer > 0 && basePrice.price > 0);

        uint256 price = uint256(int256(basePrice.price)) * Constants.Q96 / uint256(quoteAnswer);
        price = price * Constants.Q96 / _decimalsDiff;

        sqrtPrice = FixedPointMathLib.sqrt(price);
    }

Now, to check if the sequencer is down, please follow the example in the Chainlink documentation on how to check the sequencer status: Chainlink Sequencer Status Check.

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 🤖_178_group AI based duplicate group recommendation bug Something isn't working duplicate-56 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
Copy link
Contributor

alex-ppg changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 28, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Jun 28, 2024
@0xAbhay
Copy link

0xAbhay commented Jun 29, 2024

Hey @alex-ppg ,

This finding is a duplicate of #69 and also additionally discusses the heartbeat inconsistency and sequencer down problem in this finding.
thank you.

@alex-ppg
Copy link

alex-ppg commented Jul 4, 2024

Hey @0xAbhay, that's a good catch! I will re-group this submission under #69 as a duplicate.

@c4-judge c4-judge reopened this Jul 4, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

This previously downgraded issue has been upgraded by alex-ppg

@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

alex-ppg marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

alex-ppg marked the issue as duplicate of #69

@c4-judge c4-judge added duplicate-69 satisfactory satisfies C4 submission criteria; eligible for awards and removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Jul 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

alex-ppg marked the issue as satisfactory

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-69 🤖_178_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants