feat: for the merge improved checks

Improve checks and detection

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
 

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

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
 
 

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.

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.
 

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.
+

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,

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();
+    }
+}

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"),
 ]