Merge pull request #2584 from crytic/dev-chronicle-price-detector

Add Chronicle unchecked price detector

slither/detectors/all_detectors.py

@@ -97,6 +97,7 @@
 from .statements.tautological_compare import TautologicalCompare
 from .statements.return_bomb import ReturnBomb
 from .functions.out_of_order_retryable import OutOfOrderRetryable
+from .statements.chronicle_unchecked_price import ChronicleUncheckedPrice
 from .statements.pyth_unchecked_confidence import PythUncheckedConfidence
 from .statements.pyth_unchecked_publishtime import PythUncheckedPublishTime
 from .functions.chainlink_feed_registry import ChainlinkFeedRegistry

slither/detectors/statements/chronicle_unchecked_price.py

@@ -0,0 +1,147 @@
+from typing import List
+
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DetectorClassification,
+    DETECTOR_INFO,
+)
+from slither.utils.output import Output
+from slither.slithir.operations import Binary, Assignment, Unpack, SolidityCall
+from slither.core.variables import Variable
+from slither.core.declarations.solidity_variables import SolidityFunction
+from slither.core.cfg.node import Node
+
+
+class ChronicleUncheckedPrice(AbstractDetector):
+    """
+    Documentation: This detector finds calls to Chronicle oracle where the returned price is not checked
+    https://docs.chroniclelabs.org/Resources/FAQ/Oracles#how-do-i-check-if-an-oracle-becomes-inactive-gets-deprecated
+    """
+
+    ARGUMENT = "chronicle-unchecked-price"
+    HELP = "Detect when Chronicle price is not checked."
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#chronicle-unchecked-price"
+
+    WIKI_TITLE = "Chronicle unchecked price"
+    WIKI_DESCRIPTION = "Chronicle oracle is used and the price returned is not checked to be valid. For more information https://docs.chroniclelabs.org/Resources/FAQ/Oracles#how-do-i-check-if-an-oracle-becomes-inactive-gets-deprecated."
+
+    # region wiki_exploit_scenario
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract C {
+    IChronicle chronicle;
+
+    constructor(address a) {
+        chronicle = IChronicle(a);
+    }
+
+    function bad() public {
+        uint256 price = chronicle.read();
+    }
+```
+The `bad` function gets the price from Chronicle by calling the read function however it does not check if the price is valid."""
+    # endregion wiki_exploit_scenario
+
+    WIKI_RECOMMENDATION = "Validate that the price returned by the oracle is valid."
+
+    def _var_is_checked(self, nodes: List[Node], var_to_check: Variable) -> bool:
+        visited = set()
+        checked = False
+
+        while nodes:
+            if checked:
+                break
+            next_node = nodes[0]
+            nodes = nodes[1:]
+
+            for node_ir in next_node.all_slithir_operations():
+                if isinstance(node_ir, Binary) and var_to_check in node_ir.read:
+                    checked = True
+                    break
+                # This case is for tryRead and tryReadWithAge
+                # if the isValid boolean is checked inside a require(isValid)
+                if (
+                    isinstance(node_ir, SolidityCall)
+                    and node_ir.function
+                    in (
+                        SolidityFunction("require(bool)"),
+                        SolidityFunction("require(bool,string)"),
+                        SolidityFunction("require(bool,error)"),
+                    )
+                    and var_to_check 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)
+        return checked
+
+    # pylint: disable=too-many-nested-blocks,too-many-branches
+    def _detect(self) -> List[Output]:
+        results: List[Output] = []
+
+        for contract in self.compilation_unit.contracts_derived:
+            for target_contract, ir in sorted(
+                contract.all_high_level_calls,
+                key=lambda x: (x[1].node.node_id, x[1].node.function.full_name),
+            ):
+                if target_contract.name in ("IScribe", "IChronicle") and ir.function_name in (
+                    "read",
+                    "tryRead",
+                    "readWithAge",
+                    "tryReadWithAge",
+                    "latestAnswer",
+                    "latestRoundData",
+                ):
+                    found = False
+                    if ir.function_name in ("read", "latestAnswer"):
+                        # We need to iterate the IRs as we are not always sure that the following IR is the assignment
+                        # for example in case of type conversion it isn't
+                        for node_ir in ir.node.irs:
+                            if isinstance(node_ir, Assignment):
+                                possible_unchecked_variable_ir = node_ir.lvalue
+                                found = True
+                                break
+                    elif ir.function_name in ("readWithAge", "tryRead", "tryReadWithAge"):
+                        # We are interested in the first item of the tuple
+                        # readWithAge : value
+                        # tryRead/tryReadWithAge : isValid
+                        for node_ir in ir.node.irs:
+                            if isinstance(node_ir, Unpack) and node_ir.index == 0:
+                                possible_unchecked_variable_ir = node_ir.lvalue
+                                found = True
+                                break
+                    elif ir.function_name == "latestRoundData":
+                        found = False
+                        for node_ir in ir.node.irs:
+                            if isinstance(node_ir, Unpack) and node_ir.index == 1:
+                                possible_unchecked_variable_ir = node_ir.lvalue
+                                found = True
+                                break
+
+                    # If we did not find the variable assignment we know it's not checked
+                    checked = (
+                        self._var_is_checked(ir.node.sons, possible_unchecked_variable_ir)
+                        if found
+                        else False
+                    )
+
+                    if not checked:
+                        info: DETECTOR_INFO = [
+                            "Chronicle price is not checked to be valid in ",
+                            ir.node.function,
+                            "\n\t- ",
+                            ir.node,
+                            "\n",
+                        ]
+                        res = self.generate_result(info)
+                        results.append(res)
+
+        return results

tests/e2e/detectors/snapshots/detectors__detector_ChronicleUncheckedPrice_0_8_20_chronicle_unchecked_price_sol__0.txt

@@ -0,0 +1,18 @@
+Chronicle price is not checked to be valid in C.bad2() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#74-76)
+	- (price,None) = chronicle.readWithAge() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#75)
+
+Chronicle price is not checked to be valid in C.bad() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#65-67)
+	- price = chronicle.read() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#66)
+
+Chronicle price is not checked to be valid in C.bad5() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#101-103)
+	- price = scribe.latestAnswer() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#102)
+
+Chronicle price is not checked to be valid in C.bad4() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#92-94)
+	- (isValid,price,None) = chronicle.tryReadWithAge() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#93)
+
+Chronicle price is not checked to be valid in C.bad3() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#83-85)
+	- (isValid,price) = chronicle.tryRead() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#84)
+
+Chronicle price is not checked to be valid in C.bad6() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#110-112)
+	- (None,price,None,None,None) = scribe.latestRoundData() (tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol#111)
+

tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol

@@ -0,0 +1,119 @@
+interface IChronicle {
+    /// @notice Returns the oracle's current value.
+    /// @dev Reverts if no value set.
+    /// @return value The oracle's current value.
+    function read() external view returns (uint value);
+
+    /// @notice Returns the oracle's current value and its age.
+    /// @dev Reverts if no value set.
+    /// @return value The oracle's current value.
+    /// @return age The value's age.
+    function readWithAge() external view returns (uint value, uint age);
+
+    /// @notice Returns the oracle's current value.
+    /// @return isValid True if value exists, false otherwise.
+    /// @return value The oracle's current value if it exists, zero otherwise.
+    function tryRead() external view returns (bool isValid, uint value);
+
+    /// @notice Returns the oracle's current value and its age.
+    /// @return isValid True if value exists, false otherwise.
+    /// @return value The oracle's current value if it exists, zero otherwise.
+    /// @return age The value's age if value exists, zero otherwise.
+    function tryReadWithAge()
+        external
+        view
+        returns (bool isValid, uint value, uint age);
+}
+
+interface IScribe is IChronicle {
+    /// @notice Returns the oracle's latest value.
+    /// @dev Provides partial compatibility with Chainlink's
+    ///      IAggregatorV3Interface.
+    /// @return roundId 1.
+    /// @return answer The oracle's latest value.
+    /// @return startedAt 0.
+    /// @return updatedAt The timestamp of oracle's latest update.
+    /// @return answeredInRound 1.
+    function latestRoundData()
+        external
+        view
+        returns (
+            uint80 roundId,
+            int answer,
+            uint startedAt,
+            uint updatedAt,
+            uint80 answeredInRound
+        );
+
+    /// @notice Returns the oracle's latest value.
+    /// @dev Provides partial compatibility with Chainlink's
+    ///      IAggregatorV3Interface.
+    /// @custom:deprecated See https://docs.chain.link/data-feeds/api-reference/#latestanswer.
+    /// @return answer The oracle's latest value.
+    function latestAnswer() external view returns (int);
+}
+
+contract C {
+    IScribe scribe;
+    IChronicle chronicle;
+
+    constructor(address a) {
+        scribe = IScribe(a);
+        chronicle = IChronicle(a);
+    }
+
+    function bad() public {
+        uint256 price = chronicle.read();
+    }
+
+    function good() public {
+        uint256 price = chronicle.read();
+        require(price != 0);
+    }
+
+    function bad2() public {
+        (uint256 price,) = chronicle.readWithAge();
+    }
+
+    function good2() public {
+        (uint256 price,) = chronicle.readWithAge();
+        require(price != 0);
+    }
+
+    function bad3() public {
+        (bool isValid, uint256 price) = chronicle.tryRead();
+    }
+
+    function good3() public {
+        (bool isValid, uint256 price) = chronicle.tryRead();
+        require(isValid);
+    }
+
+    function bad4() public {
+        (bool isValid, uint256 price,) = chronicle.tryReadWithAge();
+    }
+
+    function good4() public {
+        (bool isValid, uint256 price,) = chronicle.tryReadWithAge();
+        require(isValid);
+    }
+
+    function bad5() public {
+        int256 price = scribe.latestAnswer();
+    }
+
+    function good5() public {
+        int256 price = scribe.latestAnswer();
+        require(price != 0);
+    }
+
+    function bad6() public {
+        (, int256 price,,,) = scribe.latestRoundData();
+    }
+
+    function good6() public {
+        (, int256 price,,,) = scribe.latestRoundData();
+        require(price != 0);
+    }
+
+}

tests/e2e/detectors/test_detectors.py

@@ -1714,6 +1714,11 @@ def id_test(test_item: Test):
         "out_of_order_retryable.sol",
         "0.8.20",
     ),
+    Test(
+        all_detectors.ChronicleUncheckedPrice,
+        "chronicle_unchecked_price.sol",
+        "0.8.20",
+    ),
     Test(
         all_detectors.PythUncheckedConfidence,
         "pyth_unchecked_confidence.sol",