Merge pull request #2581 from crytic/dev-pyth-unchecked-publishtime-c…

…onf-detector

Add Pyth unchecked publishTime and confidence detectors

slither/detectors/all_detectors.py

@@ -97,6 +97,8 @@
 from .statements.tautological_compare import TautologicalCompare
 from .statements.return_bomb import ReturnBomb
 from .functions.out_of_order_retryable import OutOfOrderRetryable
+from .statements.pyth_unchecked_confidence import PythUncheckedConfidence
+from .statements.pyth_unchecked_publishtime import PythUncheckedPublishTime
 from .functions.chainlink_feed_registry import ChainlinkFeedRegistry
 from .functions.pyth_deprecated_functions import PythDeprecatedFunctions
 from .functions.optimism_deprecation import OptimismDeprecation

slither/detectors/statements/pyth_unchecked.py

@@ -0,0 +1,79 @@
+from typing import List
+
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DETECTOR_INFO,
+)
+from slither.utils.output import Output
+from slither.slithir.operations import Member, Binary, Assignment
+
+
+class PythUnchecked(AbstractDetector):
+    """
+    Documentation: This detector finds deprecated Pyth function calls
+    """
+
+    # To be overriden in the derived class
+    PYTH_FUNCTIONS = []
+    PYTH_FIELD = ""
+
+    # pylint: disable=too-many-nested-blocks
+    def _detect(self) -> List[Output]:
+        results: List[Output] = []
+
+        for contract in self.compilation_unit.contracts_derived:
+            for target_contract, ir in contract.all_high_level_calls:
+                if target_contract.name == "IPyth" and ir.function_name in self.PYTH_FUNCTIONS:
+                    # We know for sure the second IR in the node is an Assignment operation of the TMP variable. Example:
+                    # 	Expression: price = pyth.getEmaPriceNoOlderThan(id,age)
+                    #   IRs:
+                    #      TMP_0(PythStructs.Price) = HIGH_LEVEL_CALL, dest:pyth(IPyth), function:getEmaPriceNoOlderThan, arguments:['id', 'age']
+                    #      price(PythStructs.Price) := TMP_0(PythStructs.Price)
+                    assert isinstance(ir.node.irs[1], Assignment)
+                    return_variable = ir.node.irs[1].lvalue
+                    checked = False
+
+                    possible_unchecked_variable_ir = None
+                    nodes = ir.node.sons
+                    visited = set()
+                    while nodes:
+                        if checked:
+                            break
+                        next_node = nodes[0]
+                        nodes = nodes[1:]
+
+                        for node_ir in next_node.all_slithir_operations():
+                            # We are accessing the unchecked_var field of the returned Price struct
+                            if (
+                                isinstance(node_ir, Member)
+                                and node_ir.variable_left == return_variable
+                                and node_ir.variable_right.name == self.PYTH_FIELD
+                            ):
+                                possible_unchecked_variable_ir = node_ir.lvalue
+                            # We assume that if unchecked_var happens to be inside a binary operation is checked
+                            if (
+                                isinstance(node_ir, Binary)
+                                and possible_unchecked_variable_ir is not None
+                                and possible_unchecked_variable_ir in node_ir.read
+                            ):
+                                checked = True
+                                break
+
+                        if next_node not in visited:
+                            visited.add(next_node)
+                            for son in next_node.sons:
+                                if son not in visited:
+                                    nodes.append(son)
+
+                    if not checked:
+                        info: DETECTOR_INFO = [
+                            f"Pyth price {self.PYTH_FIELD} field is not checked in ",
+                            ir.node.function,
+                            "\n\t- ",
+                            ir.node,
+                            "\n",
+                        ]
+                        res = self.generate_result(info)
+                        results.append(res)
+
+        return results

slither/detectors/statements/pyth_unchecked_confidence.py

@@ -0,0 +1,50 @@
+from slither.detectors.abstract_detector import DetectorClassification
+from slither.detectors.statements.pyth_unchecked import PythUnchecked
+
+
+class PythUncheckedConfidence(PythUnchecked):
+    """
+    Documentation: This detector finds when the confidence level of a Pyth price is not checked
+    """
+
+    ARGUMENT = "pyth-unchecked-confidence"
+    HELP = "Detect when the confidence level of a Pyth price is not checked"
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#pyth-unchecked-confidence"
+    WIKI_TITLE = "Pyth unchecked confidence level"
+    WIKI_DESCRIPTION = "Detect when the confidence level of a Pyth price is not checked"
+    WIKI_RECOMMENDATION = "Check the confidence level of a Pyth price. Visit https://docs.pyth.network/price-feeds/best-practices#confidence-intervals for more information."
+
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+import "@pythnetwork/pyth-sdk-solidity/IPyth.sol";
+import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol";
+
+contract C {
+    IPyth pyth;
+
+    constructor(IPyth _pyth) {
+        pyth = _pyth;
+    }
+
+    function bad(bytes32 id, uint256 age) public {
+        PythStructs.Price memory price = pyth.getEmaPriceNoOlderThan(id, age);
+        // Use price
+    }
+}    
+```
+The function `A` uses the price without checking its confidence level. 
+"""
+
+    PYTH_FUNCTIONS = [
+        "getEmaPrice",
+        "getEmaPriceNoOlderThan",
+        "getEmaPriceUnsafe",
+        "getPrice",
+        "getPriceNoOlderThan",
+        "getPriceUnsafe",
+    ]
+
+    PYTH_FIELD = "conf"

slither/detectors/statements/pyth_unchecked_publishtime.py

@@ -0,0 +1,52 @@
+from slither.detectors.abstract_detector import DetectorClassification
+from slither.detectors.statements.pyth_unchecked import PythUnchecked
+
+
+class PythUncheckedPublishTime(PythUnchecked):
+    """
+    Documentation: This detector finds when the publishTime of a Pyth price is not checked
+    """
+
+    ARGUMENT = "pyth-unchecked-publishtime"
+    HELP = "Detect when the publishTime of a Pyth price is not checked"
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = (
+        "https://github.com/crytic/slither/wiki/Detector-Documentation#pyth-unchecked-publishtime"
+    )
+    WIKI_TITLE = "Pyth unchecked publishTime"
+    WIKI_DESCRIPTION = "Detect when the publishTime of a Pyth price is not checked"
+    WIKI_RECOMMENDATION = "Check the publishTime of a Pyth price."
+
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+import "@pythnetwork/pyth-sdk-solidity/IPyth.sol";
+import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol";
+
+contract C {
+    IPyth pyth;
+
+    constructor(IPyth _pyth) {
+        pyth = _pyth;
+    }
+
+    function bad(bytes32 id) public {
+        PythStructs.Price memory price = pyth.getEmaPriceUnsafe(id);
+        // Use price
+    }
+}    
+```
+The function `A` uses the price without checking its `publishTime` coming from the `getEmaPriceUnsafe` function. 
+"""
+
+    PYTH_FUNCTIONS = [
+        "getEmaPrice",
+        # "getEmaPriceNoOlderThan",
+        "getEmaPriceUnsafe",
+        "getPrice",
+        # "getPriceNoOlderThan",
+        "getPriceUnsafe",
+    ]
+
+    PYTH_FIELD = "publishTime"

tests/e2e/detectors/snapshots/detectors__detector_PythUncheckedConfidence_0_8_20_pyth_unchecked_confidence_sol__0.txt

@@ -0,0 +1,3 @@
+Pyth price conf field is not checked in C.bad(bytes32,uint256) (tests/e2e/detectors/test_data/pyth-unchecked-confidence/0.8.20/pyth_unchecked_confidence.sol#171-175)
+	- price = pyth.getEmaPriceNoOlderThan(id,age) (tests/e2e/detectors/test_data/pyth-unchecked-confidence/0.8.20/pyth_unchecked_confidence.sol#172)
+

tests/e2e/detectors/snapshots/detectors__detector_PythUncheckedPublishTime_0_8_20_pyth_unchecked_publishtime_sol__0.txt

@@ -0,0 +1,3 @@
+Pyth price publishTime field is not checked in C.bad(bytes32) (tests/e2e/detectors/test_data/pyth-unchecked-publishtime/0.8.20/pyth_unchecked_publishtime.sol#171-175)
+	- price = pyth.getEmaPriceUnsafe(id) (tests/e2e/detectors/test_data/pyth-unchecked-publishtime/0.8.20/pyth_unchecked_publishtime.sol#172)
+

tests/e2e/detectors/test_data/pyth-unchecked-confidence/0.8.20/pyth_unchecked_confidence.sol

@@ -0,0 +1,193 @@
+contract PythStructs {
+    // A price with a degree of uncertainty, represented as a price +- a confidence interval.
+    //
+    // The confidence interval roughly corresponds to the standard error of a normal distribution.
+    // Both the price and confidence are stored in a fixed-point numeric representation,
+    // `x * (10^expo)`, where `expo` is the exponent.
+    //
+    // Please refer to the documentation at https://docs.pyth.network/consumers/best-practices for how
+    // to how this price safely.
+    struct Price {
+        // Price
+        int64 price;
+        // Confidence interval around the price
+        uint64 conf;
+        // Price exponent
+        int32 expo;
+        // Unix timestamp describing when the price was published
+        uint publishTime;
+    }
+
+    // PriceFeed represents a current aggregate price from pyth publisher feeds.
+    struct PriceFeed {
+        // The price ID.
+        bytes32 id;
+        // Latest available price
+        Price price;
+        // Latest available exponentially-weighted moving average price
+        Price emaPrice;
+    }
+}
+
+interface IPyth {
+    /// @notice Returns the period (in seconds) that a price feed is considered valid since its publish time
+    function getValidTimePeriod() external view returns (uint validTimePeriod);
+
+    /// @notice Returns the price and confidence interval.
+    /// @dev Reverts if the price has not been updated within the last `getValidTimePeriod()` seconds.
+    /// @param id The Pyth Price Feed ID of which to fetch the price and confidence interval.
+    /// @return price - please read the documentation of PythStructs.Price to understand how to use this safely.
+    function getPrice(
+        bytes32 id
+    ) external view returns (PythStructs.Price memory price);
+
+    /// @notice Returns the exponentially-weighted moving average price and confidence interval.
+    /// @dev Reverts if the EMA price is not available.
+    /// @param id The Pyth Price Feed ID of which to fetch the EMA price and confidence interval.
+    /// @return price - please read the documentation of PythStructs.Price to understand how to use this safely.
+    function getEmaPrice(
+        bytes32 id
+    ) external view returns (PythStructs.Price memory price);
+
+    /// @notice Returns the price of a price feed without any sanity checks.
+    /// @dev This function returns the most recent price update in this contract without any recency checks.
+    /// This function is unsafe as the returned price update may be arbitrarily far in the past.
+    ///
+    /// Users of this function should check the `publishTime` in the price to ensure that the returned price is
+    /// sufficiently recent for their application. If you are considering using this function, it may be
+    /// safer / easier to use either `getPrice` or `getPriceNoOlderThan`.
+    /// @return price - please read the documentation of PythStructs.Price to understand how to use this safely.
+    function getPriceUnsafe(
+        bytes32 id
+    ) external view returns (PythStructs.Price memory price);
+
+    /// @notice Returns the price that is no older than `age` seconds of the current time.
+    /// @dev This function is a sanity-checked version of `getPriceUnsafe` which is useful in
+    /// applications that require a sufficiently-recent price. Reverts if the price wasn't updated sufficiently
+    /// recently.
+    /// @return price - please read the documentation of PythStructs.Price to understand how to use this safely.
+    function getPriceNoOlderThan(
+        bytes32 id,
+        uint age
+    ) external view returns (PythStructs.Price memory price);
+
+    /// @notice Returns the exponentially-weighted moving average price of a price feed without any sanity checks.
+    /// @dev This function returns the same price as `getEmaPrice` in the case where the price is available.
+    /// However, if the price is not recent this function returns the latest available price.
+    ///
+    /// The returned price can be from arbitrarily far in the past; this function makes no guarantees that
+    /// the returned price is recent or useful for any particular application.
+    ///
+    /// Users of this function should check the `publishTime` in the price to ensure that the returned price is
+    /// sufficiently recent for their application. If you are considering using this function, it may be
+    /// safer / easier to use either `getEmaPrice` or `getEmaPriceNoOlderThan`.
+    /// @return price - please read the documentation of PythStructs.Price to understand how to use this safely.
+    function getEmaPriceUnsafe(
+        bytes32 id
+    ) external view returns (PythStructs.Price memory price);
+
+    /// @notice Returns the exponentially-weighted moving average price that is no older than `age` seconds
+    /// of the current time.
+    /// @dev This function is a sanity-checked version of `getEmaPriceUnsafe` which is useful in
+    /// applications that require a sufficiently-recent price. Reverts if the price wasn't updated sufficiently
+    /// recently.
+    /// @return price - please read the documentation of PythStructs.Price to understand how to use this safely.
+    function getEmaPriceNoOlderThan(
+        bytes32 id,
+        uint age
+    ) external view returns (PythStructs.Price memory price);
+
+    /// @notice Update price feeds with given update messages.
+    /// This method requires the caller to pay a fee in wei; the required fee can be computed by calling
+    /// `getUpdateFee` with the length of the `updateData` array.
+    /// Prices will be updated if they are more recent than the current stored prices.
+    /// The call will succeed even if the update is not the most recent.
+    /// @dev Reverts if the transferred fee is not sufficient or the updateData is invalid.
+    /// @param updateData Array of price update data.
+    function updatePriceFeeds(bytes[] calldata updateData) external payable;
+
+    /// @notice Wrapper around updatePriceFeeds that rejects fast if a price update is not necessary. A price update is
+    /// necessary if the current on-chain publishTime is older than the given publishTime. It relies solely on the
+    /// given `publishTimes` for the price feeds and does not read the actual price update publish time within `updateData`.
+    ///
+    /// This method requires the caller to pay a fee in wei; the required fee can be computed by calling
+    /// `getUpdateFee` with the length of the `updateData` array.
+    ///
+    /// `priceIds` and `publishTimes` are two arrays with the same size that correspond to senders known publishTime
+    /// of each priceId when calling this method. If all of price feeds within `priceIds` have updated and have
+    /// a newer or equal publish time than the given publish time, it will reject the transaction to save gas.
+    /// Otherwise, it calls updatePriceFeeds method to update the prices.
+    ///
+    /// @dev Reverts if update is not needed or the transferred fee is not sufficient or the updateData is invalid.
+    /// @param updateData Array of price update data.
+    /// @param priceIds Array of price ids.
+    /// @param publishTimes Array of publishTimes. `publishTimes[i]` corresponds to known `publishTime` of `priceIds[i]`
+    function updatePriceFeedsIfNecessary(
+        bytes[] calldata updateData,
+        bytes32[] calldata priceIds,
+        uint64[] calldata publishTimes
+    ) external payable;
+
+    /// @notice Returns the required fee to update an array of price updates.
+    /// @param updateData Array of price update data.
+    /// @return feeAmount The required fee in Wei.
+    function getUpdateFee(
+        bytes[] calldata updateData
+    ) external view returns (uint feeAmount);
+
+    /// @notice Parse `updateData` and return price feeds of the given `priceIds` if they are all published
+    /// within `minPublishTime` and `maxPublishTime`.
+    ///
+    /// You can use this method if you want to use a Pyth price at a fixed time and not the most recent price;
+    /// otherwise, please consider using `updatePriceFeeds`. This method does not store the price updates on-chain.
+    ///
+    /// This method requires the caller to pay a fee in wei; the required fee can be computed by calling
+    /// `getUpdateFee` with the length of the `updateData` array.
+    ///
+    ///
+    /// @dev Reverts if the transferred fee is not sufficient or the updateData is invalid or there is
+    /// no update for any of the given `priceIds` within the given time range.
+    /// @param updateData Array of price update data.
+    /// @param priceIds Array of price ids.
+    /// @param minPublishTime minimum acceptable publishTime for the given `priceIds`.
+    /// @param maxPublishTime maximum acceptable publishTime for the given `priceIds`.
+    /// @return priceFeeds Array of the price feeds corresponding to the given `priceIds` (with the same order).
+    function parsePriceFeedUpdates(
+        bytes[] calldata updateData,
+        bytes32[] calldata priceIds,
+        uint64 minPublishTime,
+        uint64 maxPublishTime
+    ) external payable returns (PythStructs.PriceFeed[] memory priceFeeds);
+}
+
+
+contract C {
+    IPyth pyth;
+
+    constructor(IPyth _pyth) {
+        pyth = _pyth;
+    }
+
+    function bad(bytes32 id, uint256 age) public {
+        PythStructs.Price memory price = pyth.getEmaPriceNoOlderThan(id, age);
+        require(price.publishTime > block.timestamp - 120);
+        // Use price
+    }
+
+    function good(bytes32 id, uint256 age) public {
+        PythStructs.Price memory price = pyth.getEmaPriceNoOlderThan(id, age);
+        require(price.conf < 10000);
+        require(price.publishTime > block.timestamp - 120);
+        // Use price
+    }
+
+    function good2(bytes32 id, uint256 age) public {
+        PythStructs.Price memory price = pyth.getEmaPriceNoOlderThan(id, age);
+        require(price.publishTime > block.timestamp - 120);
+        if (price.conf >= 10000) {
+            revert();
+        }
+        // Use price
+    }
+
+}