diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index d1f46d8e8..f72a88835 100644 --- a/slither/detectors/all_detectors.py +++ b/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 diff --git a/slither/detectors/statements/chronicle_unchecked_price.py b/slither/detectors/statements/chronicle_unchecked_price.py new file mode 100644 index 000000000..47ad2ddc5 --- /dev/null +++ b/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 diff --git a/tests/e2e/detectors/snapshots/detectors__detector_ChronicleUncheckedPrice_0_8_20_chronicle_unchecked_price_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_ChronicleUncheckedPrice_0_8_20_chronicle_unchecked_price_sol__0.txt new file mode 100644 index 000000000..6ddbfa4e5 --- /dev/null +++ b/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) + diff --git a/tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol b/tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol new file mode 100644 index 000000000..e12560fa7 --- /dev/null +++ b/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); + } + +} \ No newline at end of file diff --git a/tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol-0.8.20.zip b/tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol-0.8.20.zip new file mode 100644 index 000000000..746efabf6 Binary files /dev/null and b/tests/e2e/detectors/test_data/chronicle-unchecked-price/0.8.20/chronicle_unchecked_price.sol-0.8.20.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index ce3b60af9..a8f55707e 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/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",