Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Emmanuel - PythOracle:if price.expo is less than 0, wrong prices will be recorded #56

Open
sherlock-admin opened this issue Aug 15, 2023 · 4 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Aug 15, 2023

Emmanuel

medium

PythOracle:if price.expo is less than 0, wrong prices will be recorded

Summary

In PythOracle#_recordPrice function, prices with negative exponents are not handled correctly, leading to a massive deviation in prices.

Vulnerability Detail

Here is PythOracle#_recordPrice function:

    function _recordPrice(uint256 oracleVersion, PythStructs.Price memory price) private {
        _prices[oracleVersion] = Fixed6Lib.from(price.price).mul(
            Fixed6Lib.from(SafeCast.toInt256(10 ** SafeCast.toUint256(price.expo > 0 ? price.expo : -price.expo)))
        );
        _publishTimes[oracleVersion] = price.publishTime;
    }

If price is 5e-5 for example, it will be recorded as 5e5
If price is 5e-6, it will be recorded as 5e6.

As we can see, there is a massive deviation in recorded price from actual price whenever price's exponent is negative

Impact

Wrong prices will be recorded.
For example,
If priceA is 5e-5, and priceB is 5e-6. But due to the wrong conversion,

  • There is a massive change in price(5e5 against 5e-5)
  • we know that priceA is ten times larger than priceB, but priceA will be recorded as ten times smaller than priceB.
    Unfortunately, current payoff functions may not be able to take care of these discrepancies

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L203

Tool used

Manual Review

Recommendation

In PythOracle.sol, _prices mapping should not be mapping(uint256 => Fixed6) private _prices;
Instead, it should be mapping(uint256 => Price) private _prices;, where Price is a struct that stores the price and expo:

struct Price{
    Fixed6 price,
    int256 expo
}

This way, the price exponents will be preserved, and can be used to scale the prices correctly wherever it is used.

@sherlock-admin
Copy link
Contributor Author

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

141345 commented:

h

@arjun-io arjun-io added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Aug 18, 2023
@sherlock-admin2 sherlock-admin2 changed the title Fresh Aegean Sparrow - PythOracle:if price.expo is less than 0, wrong prices will be recorded Emmanuel - PythOracle:if price.expo is less than 0, wrong prices will be recorded Aug 23, 2023
@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label Aug 23, 2023
@arjun-io
Copy link

Fixed: equilibria-xyz/perennial-v2#53

@jacksanford1
Copy link

From WatchPug:

The updated version may lower the precision from a higher value (e.g. 10) to the fixed precision of 6.

For example, the price of BTT/USD would be rounding down to 0 with the updated version.

https://pyth.network/price-feeds/crypto-btt-usd

@arjun-io
Copy link

From WatchPug:

The updated version may lower the precision from a higher value (e.g. 10) to the fixed precision of 6.

For example, the price of BTT/USD would be rounding down to 0 with the updated version.

https://pyth.network/price-feeds/crypto-btt-usd

Ack, noted that we currently do not support price feeds with very small unit prices. We’ll record this among the limitations / requirements for new markets and add it to the list of possible future improvements, but will not implement a fix at this time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

4 participants