Rewrote external-function detector to handle false-positives as a result of inheritance.

slither/detectors/functions/external_function.py

@@ -27,7 +27,10 @@ class ExternalFunction(AbstractDetector):
             (list): List of all InternallCall, SolidityCall
         """
         result = []
+
+        # Obtain all functions reachable by this contract.
         for func in contract.all_functions_called:
+            # Loop through all nodes in the function, add all calls to a list.
             for node in func.nodes:
                 for ir in node.irs:
                     if isinstance(ir, (InternalCall, SolidityCall)):
@@ -36,6 +39,12 @@ class ExternalFunction(AbstractDetector):
 
     @staticmethod
     def _contains_internal_dynamic_call(contract):
+        """
+        Checks if a contract contains a dynamic call either in a direct definition, or through inheritance.
+
+        Returns:
+            (boolean): True if this contract contains a dynamic call (including through inheritance).
+        """
         for func in contract.all_functions_called:
             for node in func.nodes:
                 for ir in node.irs:
@@ -43,31 +52,126 @@ class ExternalFunction(AbstractDetector):
                         return True
         return False
 
+    @staticmethod
+    def get_base_most_function(function):
+        """
+        Obtains the base function definition for the provided function. This could be used to obtain the original
+        definition of a function, if the provided function is an override.
+
+        Returns:
+            (function): Returns the base-most function of a provided function. (The original definition).
+        """
+        # Loop through the list of inherited contracts and this contract, to find the first function instance which
+        # matches this function's signature. Note here that `inheritance` is in order from most basic to most extended.
+        for contract in function.contract.inheritance + [function.contract]:
+
+            # Loop through the functions not inherited (explicitly defined in this contract).
+            for f in contract.functions_not_inherited:
+
+                # If it matches names, this is the base most function.
+                if f.full_name == function.full_name:
+                    return f
+
+        # Somehow we couldn't resolve it, which shouldn't happen, as the provided function should be found if we could
+        # not find some any more basic.
+        raise Exception("Could not resolve the base-most function for the provided function.")
+
+    @staticmethod
+    def get_all_function_definitions(base_most_function):
+        """
+        Obtains all function definitions given a base-most function. This includes the provided function, plus any
+        overrides of that function.
+
+        Returns:
+            (list): Returns any the provided function and any overriding functions defined for it.
+        """
+        # We assume the provided function is the base-most function, so we check all derived contracts
+        # for a redefinition
+        return [base_most_function] + [function for derived_contract in base_most_function.contract.derived_contracts
+                                       for function in derived_contract.functions
+                                       if function.full_name == base_most_function.full_name]
+
     def detect(self):
         results = []
 
-        public_function_calls = []
         all_info = ''
 
-        for contract in self.slither.contracts_derived:
+        # Create a set to track contracts with dynamic calls. All contracts with dynamic calls could potentially be
+        # calling functions internally, and thus we can't assume any function in such contracts isn't called by them.
+        dynamic_call_contracts = set()
+
+        # Create a completed functions set to skip over functions already processed (any functions which are the base
+        # of, or override hierarchically are processed together).
+        completed_functions = set()
+
+        # First we build our set of all contracts with dynamic calls
+        for contract in self.contracts:
             if self._contains_internal_dynamic_call(contract):
+                dynamic_call_contracts.add(contract)
+
+        # Loop through all not-inherited contracts.
+        for contract in self.slither.contracts_derived:
+
+            # Filter false-positives: Immediately filter this contract if it's in blacklist
+            if contract in dynamic_call_contracts:
                 continue
 
-            func_list = self.detect_functions_called(contract)
-            public_function_calls.extend(func_list)
-
-            for func in [f for f in contract.functions if f.visibility == 'public' and\
-                                                           not f in public_function_calls and\
-                                                           not f.is_constructor]:
-                txt = "{}.{} ({}) should be declared external\n"
-                info = txt.format(func.contract.name,
-                                  func.name,
-                                  func.source_mapping_str)
-                all_info += info
-
-                json = self.generate_json_result(info)
-                self.add_function_to_json(func, json)
-                results.append(json)
+            # Next we'll want to loop through all functions defined directly in this contract.
+            for function in contract.functions_not_inherited:
+
+                # If the function is a constructor, or is public, we skip it.
+                if function.is_constructor or function.visibility != "public":
+                    continue
+
+                # Optimization: If this function has already been processed, we stop.
+                if function in completed_functions:
+                    continue
+
+                # Get the base-most function to know our origin of this function.
+                base_most_function = self.get_base_most_function(function)
+
+                # Get all possible contracts which can call this function (or an override).
+                all_possible_sources = [base_most_function.contract] + base_most_function.contract.derived_contracts
+
+                # Get all function signatures (overloaded and not), mark as completed and we process them now.
+                # Note: We mark all function definitions as the same, as they must all share visibility to override.
+                all_function_definitions = set(self.get_all_function_definitions(base_most_function))
+                completed_functions = completed_functions.union(all_function_definitions)
+
+                # Filter false-positives: Determine if any of these sources have dynamic calls, if so, flag all of these
+                # function definitions, and then flag all functions in all contracts that make dynamic calls.
+                sources_with_dynamic_calls = set(all_possible_sources) & dynamic_call_contracts
+                if sources_with_dynamic_calls:
+                    functions_in_dynamic_call_sources = set([f for dyn_contract in sources_with_dynamic_calls
+                                                             for f in dyn_contract if not f.is_constructor])
+                    completed_functions = completed_functions.union(functions_in_dynamic_call_sources)
+                    continue
+
+                # Detect all functions called in each source, if any match our current signature, we skip
+                # otherwise, this is a candidate (in all sources) to be changed visibility for.
+                is_called = False
+                for possible_source in all_possible_sources:
+                    functions_called = self.detect_functions_called(possible_source)
+                    if set(functions_called) & all_function_definitions:
+                        is_called = True
+                        break
+
+                # If any of this function's definitions are called, we skip it.
+                if is_called:
+                    continue
+
+                # Loop for each function definition, and recommend it be declared external.
+                for function_definition in all_function_definitions:
+                    txt = "{}.{} ({}) should be declared external\n"
+                    info = txt.format(function_definition.contract.name,
+                                      function_definition.name,
+                                      function_definition.source_mapping_str)
+                    all_info += info
+
+                    json = self.generate_json_result(info)
+                    self.add_function_to_json(function_definition, json)
+                    results.append(json)
+
         if all_info != '':
             self.log(all_info)
         return results

tests/external_function_test_3.sol

@@ -0,0 +1,55 @@
+// This tests against false-positives. This test should output no recommendations from the external-function detector.
+
+
+contract ContractWithBaseFunctionCalled {
+    function getsCalledByBase() public;
+    function callsOverrideMe() external {
+        getsCalledByBase();
+    }
+}
+
+
+contract DerivingContractWithBaseCalled is ContractWithBaseFunctionCalled {
+    function getsCalledByBase() public {
+        // This should not be recommended to be marked external because it is called by the base class.
+    }
+}
+
+
+// All the contracts below should not recommend changing to external since inherited contracts have dynamic calls.
+contract ContractWithDynamicCall {
+    function() returns(uint) ptr;
+
+    function test1() public returns(uint){
+        return 1;
+    }
+
+    function test2() public returns(uint){
+        return 2;
+    }
+
+    function set_test1() external{
+        ptr = test1;
+    }
+
+    function set_test2() external{
+        ptr = test2;
+    }
+
+    function exec() external returns(uint){
+        return ptr();
+    }
+}
+
+contract DerivesFromDynamicCall is ContractWithDynamicCall{
+    function getsCalledDynamically() public returns (uint){
+        // This should not be recommended because it is called dynamically.
+        return 3;
+    }
+    function set_test3() public {
+        // This should not be recommended because we inherit from a contract that calls dynamically, and we cannot be
+        // sure it did not somehow call this function.
+
+        ptr = getsCalledDynamically;
+    }
+}