diff --git a/slither/detectors/oracles/oracle_detector.py b/slither/detectors/oracles/oracle_detector.py index 9e23ff87d..951f2c045 100644 --- a/slither/detectors/oracles/oracle_detector.py +++ b/slither/detectors/oracles/oracle_detector.py @@ -157,24 +157,27 @@ def vars_in_conditions(self, oracle: Oracle): for node in self.nodes_with_var: for ir in node.irs: if isinstance(ir, InternalCall): - self.investigate_internal_call(ir.function, var) + self.investigate_internal_call(ir.function, var, None) if len(self.nodes_with_var) > 0: # vars_in_condition.append(VarInCondition(var, self.nodes_with_var)) oracle_vars.append(VarInCondition(var, self.nodes_with_var)) else: - if self.investigate_internal_call(oracle.function, var): + if self.investigate_internal_call(oracle.function, var, None): # vars_in_condition.append(VarInCondition(var, self.nodes_with_var)) oracle_vars.append(VarInCondition(var, self.nodes_with_var)) elif nodes := self.investigate_on_return(oracle, var): oracle_vars.append(VarInCondition(var, nodes)) - oracle.out_of_function_checks = True + oracle.out_of_function_checks.append((var, nodes)) + # except RecursionError: + # vars_not_in_condition.append(var) + # oracle_vars.append(var) else: vars_not_in_condition.append(var) oracle_vars.append(var) # oracle.vars_in_condition = vars_in_condition - oracle.vars_not_in_condition.extend(vars_not_in_condition) + oracle.vars_not_in_condition = vars_not_in_condition oracle.oracle_vars = oracle_vars return True @@ -205,7 +208,7 @@ def checks_performed_out_of_original_function(self, oracle, returned_var): oracle.oracle_vars = [var] break self.vars_in_conditions(oracle) - if type(oracle.oracle_vars[0]) == VarInCondition: + if isinstance(oracle.oracle_vars[0], VarInCondition): nodes.extend(oracle.oracle_vars[0].nodes_with_var) i += 1 @@ -224,18 +227,23 @@ def find_if_original_function_called(oracle, function): if ir.function == oracle.function: nodes_of_call.append(node) functions_of_call.append(function) + if ir.function == function: + return None, None return nodes_of_call, functions_of_call def investigate_on_return(self, oracle, var) -> bool: for value in oracle.function.return_values: if is_dependent(value, var, oracle.node): return self.checks_performed_out_of_original_function(oracle, value) + return False # This function interates through all internal calls in function and checks if the var is used in condition any of them - def investigate_internal_call(self, function: FunctionContract, var) -> bool: + def investigate_internal_call(self, function: FunctionContract, var, original_function) -> bool: if function is None: return False + if function == original_function: + return False original_var_as_param = self.map_param_to_var(var, function) if original_var_as_param is None: @@ -260,7 +268,7 @@ def investigate_internal_call(self, function: FunctionContract, var) -> bool: for node in function.nodes: for ir in node.irs: if isinstance(ir, InternalCall): - if self.investigate_internal_call(ir.function, original_var_as_param): + if self.investigate_internal_call(ir.function, original_var_as_param, function): return True return False diff --git a/slither/detectors/oracles/supported_oracles/chainlink_oracle.py b/slither/detectors/oracles/supported_oracles/chainlink_oracle.py index 07444c6cf..dad9f626a 100644 --- a/slither/detectors/oracles/supported_oracles/chainlink_oracle.py +++ b/slither/detectors/oracles/supported_oracles/chainlink_oracle.py @@ -83,7 +83,7 @@ def is_sequencer_check(self, answer, startedAt): answer_checked = False startedAt_checked = False - for node in answer.nodes: + for node in answer.nodes_with_var: for ir in node.irs: if isinstance(ir, Binary): if self.price_check_for_liveness(ir): @@ -120,8 +120,8 @@ def naive_data_validation(self): if self.is_sequencer_check(vars_order[ChainlinkVars.ANSWER.value], var): problems = [] break - if self.out_of_function_checks: + for tup in self.out_of_function_checks: problems.append( - "One or all of the variables are not checked within the function where the call to the oracle was performed.\n" + f"The variation of {tup[0]} is checked on the lines {[str(node.source_mapping) for node in tup[1][::5]]}. Not in the original function where the Oracle call is performed.\n" ) return problems diff --git a/slither/detectors/oracles/supported_oracles/help_functions.py b/slither/detectors/oracles/supported_oracles/help_functions.py index ece271836..a4c265b24 100644 --- a/slither/detectors/oracles/supported_oracles/help_functions.py +++ b/slither/detectors/oracles/supported_oracles/help_functions.py @@ -2,6 +2,7 @@ from slither.slithir.operations.return_operation import Return from slither.slithir.operations import InternalCall from slither.slithir.operations.solidity_call import SolidityCall +from slither.slithir.variables.constant import Constant # Helpfull functions @@ -16,13 +17,21 @@ def check_revert(node: Node) -> bool: return False +def is_boolean(ir) -> bool: + for val in ir.values: + if isinstance(val, Constant): + if isinstance(val.value, bool): + return True + return False + + # Check if the node's sons contain a return statement def return_boolean(node: Node) -> bool: for n in node.sons: if n.type == NodeType.RETURN: for ir in n.irs: if isinstance(ir, Return): - return True + return is_boolean(ir) return False diff --git a/slither/detectors/oracles/supported_oracles/oracle.py b/slither/detectors/oracles/supported_oracles/oracle.py index 880abe5fc..6c1320471 100644 --- a/slither/detectors/oracles/supported_oracles/oracle.py +++ b/slither/detectors/oracles/supported_oracles/oracle.py @@ -20,7 +20,7 @@ def __init__(self, _calls): self.contract = None self.function = None self.node = None - self.out_of_function_checks = False + self.out_of_function_checks = [] self.oracle_vars = [] self.vars_not_in_condition = [] self.returned_vars_indexes = None @@ -32,7 +32,9 @@ def get_calls(self): def is_instance_of(self, ir: Operation) -> bool: return isinstance(ir, HighLevelCall) and ( - isinstance(ir.function, Function) and self.compare_call(ir.function.name) + isinstance(ir.function, Function) + and self.compare_call(ir.function.name) + # add interface ) def set_node(self, _node): @@ -72,8 +74,12 @@ def check_greater_zero(ir: Operation) -> bool: @staticmethod def timestamp_in_node(node) -> bool: - if "block.timestamp" in str(node): - return True + all_nodes = [node] + if node.fathers: + all_nodes.extend(node.fathers) + for var in all_nodes: + if "block.timestamp" in str(var): + return True return False # This function checks if the timestamp value is validated. diff --git a/tests/e2e/detectors/snapshots/detectors__detector_OracleDataCheck_0_8_20_oracle_check_out_of_function_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_OracleDataCheck_0_8_20_oracle_check_out_of_function_sol__0.txt index 227a78104..fe94decd8 100644 --- a/tests/e2e/detectors/snapshots/detectors__detector_OracleDataCheck_0_8_20_oracle_check_out_of_function_sol__0.txt +++ b/tests/e2e/detectors/snapshots/detectors__detector_OracleDataCheck_0_8_20_oracle_check_out_of_function_sol__0.txt @@ -1,3 +1,3 @@ The price can be stale due to incorrect validation of updatedAt value. This value is returned by Chainlink oracle call ChainlinkETHUSDPriceConsumer.priceFeed.latestRoundData (tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_check_out_of_function.sol#58). -One or all of the variables are not checked within the function where the call to the oracle was performed. +The variation of price is checked on the lines ['tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_check_out_of_function.sol#50']. Not in the original function where the Oracle call is performed. diff --git a/tests/e2e/detectors/snapshots/detectors__detector_OracleDataCheck_0_8_20_oracle_timestamp_in_var_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_OracleDataCheck_0_8_20_oracle_timestamp_in_var_sol__0.txt new file mode 100644 index 000000000..dd42ee085 --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_OracleDataCheck_0_8_20_oracle_timestamp_in_var_sol__0.txt @@ -0,0 +1,2 @@ +The oracle ChainlinkETHUSDPriceConsumer.priceFeed (tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_timestamp_in_var.sol#51) returns the variables ['price'] which are not validated. It can potentially lead to unexpected behaviour. + diff --git a/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_data_check_price_in_internal_fc.sol b/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_data_check_price_in_internal_fc.sol index d7899146c..f4697213d 100644 --- a/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_data_check_price_in_internal_fc.sol +++ b/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_data_check_price_in_internal_fc.sol @@ -49,9 +49,6 @@ contract StableOracleDAI { function getPriceUSD() external view returns (uint256) { uint256 wethPriceUSD = 1; uint256 DAIWethPrice = 1; - - // chainlink price data is 8 decimals for WETH/USD, so multiply by 10 decimals to get 18 decimal fractional - //(uint80 roundID, int256 price, uint256 startedAt, uint256 timeStamp, uint80 answeredInRound) = priceFeedDAIETH.latestRoundData(); ( uint80 roundID, int256 price, diff --git a/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_timestamp_in_var.sol b/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_timestamp_in_var.sol new file mode 100644 index 000000000..13f5e7691 --- /dev/null +++ b/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_timestamp_in_var.sol @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.20; + +interface AggregatorV3Interface { + function decimals() external view returns (uint8); + + function description() external view returns (string memory); + + function version() external view returns (uint256); + + // getRoundData and latestRoundData should both raise "No data present" + // if they do not have data to report, instead of returning unset values + // which could be misinterpreted as actual reported values. + function getRoundData( + uint80 _roundId + ) + external + view + returns ( + uint80 roundId, + int256 answer, + uint256 startedAt, + uint256 updatedAt, + uint80 answeredInRound + ); + + function latestRoundData() + external + view + returns ( + uint80 roundId, + int256 answer, + uint256 startedAt, + uint256 updatedAt, + uint80 answeredInRound + ); +} + +contract ChainlinkETHUSDPriceConsumer { + AggregatorV3Interface internal priceFeed; + + constructor() public { + priceFeed = AggregatorV3Interface( + 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419 + ); + } + /** + * Returns the latest price + */ + function getLatestPrice() public view returns (int) { + (, int price, ,uint256 updateAt , ) = priceFeed.latestRoundData(); + uint current_timestamp = block.timestamp; + require(current_timestamp - updateAt < 1 minutes, "Price is outdated"); + return price; + } + + function getDecimals() public view returns (uint8) { + return priceFeed.decimals(); + } +} diff --git a/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_timestamp_in_var.sol-0.8.20.zip b/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_timestamp_in_var.sol-0.8.20.zip new file mode 100644 index 000000000..854f0e5d4 Binary files /dev/null and b/tests/e2e/detectors/test_data/oracle-data-validation/0.8.20/oracle_timestamp_in_var.sol-0.8.20.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 011ad4fc1..acf6ef2a0 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1687,6 +1687,7 @@ def id_test(test_item: Test): Test(all_detectors.OracleDataCheck, "oracle_data_check_price_in_internal_fc.sol", "0.8.20"), Test(all_detectors.OracleDataCheck, "oracle_non_revert.sol", "0.8.20"), Test(all_detectors.OracleDataCheck, "oracle_check_out_of_function.sol", "0.8.20"), + Test(all_detectors.OracleDataCheck,"oracle_timestamp_in_var.sol", "0.8.20"), Test(all_detectors.DeprecatedChainlinkCall, "oracle_deprecated_call.sol", "0.8.20"), ]