Fix external-function detector to avoid false-positives

scripts/tests_generate_expected_json_4.sh

@@ -14,28 +14,29 @@ generate_expected_json(){
 
 }
 
-generate_expected_json tests/uninitialized.sol "uninitialized-state"
-generate_expected_json tests/backdoor.sol "backdoor"
-generate_expected_json tests/backdoor.sol "suicidal"
-generate_expected_json tests/pragma.0.4.24.sol "pragma"
-generate_expected_json tests/old_solc.sol.json "solc-version"
-generate_expected_json tests/reentrancy.sol "reentrancy"
-generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage"
-generate_expected_json tests/tx_origin.sol "tx-origin"
-generate_expected_json tests/unused_state.sol "unused-state"
-generate_expected_json tests/locked_ether.sol "locked-ether"
-generate_expected_json tests/arbitrary_send.sol "arbitrary-send"
-generate_expected_json tests/inline_assembly_contract.sol "assembly"
-generate_expected_json tests/inline_assembly_library.sol "assembly"
-generate_expected_json tests/low_level_calls.sol "low-level-calls"
-generate_expected_json tests/const_state_variables.sol "constable-states"
-generate_expected_json tests/external_function.sol "external-function"
-generate_expected_json tests/naming_convention.sol "naming-convention"
-generate_expected_json tests/uninitialized_local_variable.sol "uninitialized-local"
-generate_expected_json tests/controlled_delegatecall.sol "controlled-delegatecall"
-generate_expected_json tests/constant.sol "constant-function"
-generate_expected_json tests/unused_return.sol "unused-return"
-generate_expected_json tests/shadowing_state_variable.sol "shadowing-state"
-generate_expected_json tests/shadowing_abstract.sol "shadowing-abstract"
-generate_expected_json tests/timestamp.sol "timestamp"
-generate_expected_json tests/multiple_calls_in_loop.sol "calls-loop"
+#generate_expected_json tests/uninitialized.sol "uninitialized-state"
+#generate_expected_json tests/backdoor.sol "backdoor"
+#generate_expected_json tests/backdoor.sol "suicidal"
+#generate_expected_json tests/pragma.0.4.24.sol "pragma"
+#generate_expected_json tests/old_solc.sol.json "solc-version"
+#generate_expected_json tests/reentrancy.sol "reentrancy"
+#generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage"
+#generate_expected_json tests/tx_origin.sol "tx-origin"
+#generate_expected_json tests/unused_state.sol "unused-state"
+#generate_expected_json tests/locked_ether.sol "locked-ether"
+#generate_expected_json tests/arbitrary_send.sol "arbitrary-send"
+#generate_expected_json tests/inline_assembly_contract.sol "assembly"
+#generate_expected_json tests/inline_assembly_library.sol "assembly"
+#generate_expected_json tests/low_level_calls.sol "low-level-calls"
+#generate_expected_json tests/const_state_variables.sol "constable-states"
+#generate_expected_json tests/external_function.sol "external-function"
+generate_expected_json tests/external_function_2.sol "external-function"
+#generate_expected_json tests/naming_convention.sol "naming-convention"
+#generate_expected_json tests/uninitialized_local_variable.sol "uninitialized-local"
+#generate_expected_json tests/controlled_delegatecall.sol "controlled-delegatecall"
+#generate_expected_json tests/constant.sol "constant-function"
+#generate_expected_json tests/unused_return.sol "unused-return"
+#generate_expected_json tests/shadowing_state_variable.sol "shadowing-state"
+#generate_expected_json tests/shadowing_abstract.sol "shadowing-abstract"
+#generate_expected_json tests/timestamp.sol "timestamp"
+#generate_expected_json tests/multiple_calls_in_loop.sol "calls-loop"

scripts/tests_generate_expected_json_5.sh

@@ -14,16 +14,16 @@ generate_expected_json(){
 
 }
 
-generate_expected_json tests/uninitialized-0.5.1.sol "uninitialized-state"
+#generate_expected_json tests/uninitialized-0.5.1.sol "uninitialized-state"
 #generate_expected_json tests/backdoor.sol "backdoor"
 #generate_expected_json tests/backdoor.sol "suicidal"
 #generate_expected_json tests/pragma.0.4.24.sol "pragma"
 #generate_expected_json tests/old_solc.sol.json "solc-version"
-generate_expected_json tests/reentrancy-0.5.1.sol "reentrancy"
+#generate_expected_json tests/reentrancy-0.5.1.sol "reentrancy"
 #generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage"
-generate_expected_json tests/tx_origin-0.5.1.sol "tx-origin"
-generate_expected_json tests/locked_ether-0.5.1.sol "locked-ether"
-generate_expected_json tests/arbitrary_send-0.5.1.sol "arbitrary-send"
-generate_expected_json tests/inline_assembly_contract-0.5.1.sol "assembly"
-generate_expected_json tests/inline_assembly_library-0.5.1.sol "assembly"
-generate_expected_json tests/constant-0.5.1.sol "constant-function"
+#generate_expected_json tests/tx_origin-0.5.1.sol "tx-origin"
+#generate_expected_json tests/locked_ether-0.5.1.sol "locked-ether"
+#generate_expected_json tests/arbitrary_send-0.5.1.sol "arbitrary-send"
+#generate_expected_json tests/inline_assembly_contract-0.5.1.sol "assembly"
+#generate_expected_json tests/inline_assembly_library-0.5.1.sol "assembly"
+#generate_expected_json tests/constant-0.5.1.sol "constant-function"

scripts/travis_test_4.sh

@@ -81,6 +81,7 @@ test_slither tests/inline_assembly_library.sol "assembly"
 test_slither tests/low_level_calls.sol "low-level-calls"
 test_slither tests/const_state_variables.sol "constable-states"
 test_slither tests/external_function.sol "external-function"
+test_slither tests/external_function_2.sol "external-function"
 test_slither tests/naming_convention.sol "naming-convention"
 #test_slither tests/complex_func.sol "complex-function"
 test_slither tests/controlled_delegatecall.sol "controlled-delegatecall"

scripts/travis_test_5.sh

@@ -79,8 +79,9 @@ test_slither tests/inline_assembly_library-0.5.1.sol "assembly"
 test_slither tests/low_level_calls.sol "low-level-calls"
 test_slither tests/const_state_variables.sol "constable-states"
 test_slither tests/external_function.sol "external-function"
+test_slither tests/external_function_2.sol "external-function"
 test_slither tests/naming_convention.sol "naming-convention"
-#test_slither tests/complex_func.sol "complex-function"
+##test_slither tests/complex_func.sol "complex-function"
 test_slither tests/controlled_delegatecall.sol "controlled-delegatecall"
 test_slither tests/constant-0.5.1.sol "constant-function"
 test_slither tests/unused_return.sol "unused-return"

slither/core/declarations/contract.py

@@ -68,6 +68,14 @@ def inheritance_reverse(self):
     def setInheritance(self, inheritance):
         self._inheritance = inheritance
 
+    @property
+    def derived_contracts(self):
+        '''
+            list(Contract): Return the list of contracts derived from self
+        '''
+        candidates = self.slither.contracts
+        return [c for c in candidates if self in c.inheritance]
+
     @property
     def structures(self):
         '''
@@ -85,12 +93,6 @@ def enums(self):
     def enums_as_dict(self):
         return self._enums
 
-    @property
-    def modifiers(self):
-        '''
-            list(Modifier): List of the modifiers
-        '''
-        return list(self._modifiers.values())
 
     def modifiers_as_dict(self):
         return self._modifiers
@@ -113,6 +115,75 @@ def functions_inherited(self):
         '''
         return [f for f in self.functions if f.contract != self]
 
+    @property
+    def functions_not_inherited(self):
+        '''
+            list(Function): List of the functions defined within the contract (not inherited)
+        '''
+        return [f for f in self.functions if f.contract == self]
+
+    @property
+    def functions_entry_points(self):
+        '''
+            list(Functions): List of public and external functions
+        '''
+        return [f for f in self.functions if f.visibility in ['public', 'external']]
+
+    @property
+    def modifiers(self):
+        '''
+            list(Modifier): List of the modifiers
+        '''
+        return list(self._modifiers.values())
+
+    @property
+    def modifiers_inherited(self):
+        '''
+            list(Modifier): List of the inherited modifiers
+        '''
+        return [m for m in self.modifiers if m.contract != self]
+
+    @property
+    def modifiers_not_inherited(self):
+        '''
+            list(Modifier): List of the modifiers defined within the contract (not inherited)
+        '''
+        return [m for m in self.modifiers if m.contract == self]
+
+    @property
+    def functions_and_modifiers(self):
+        '''
+            list(Function|Modifier): List of the functions and modifiers
+        '''
+        return self.functions + self.modifiers
+
+    @property
+    def functions_and_modifiers_inherited(self):
+        '''
+            list(Function|Modifier): List of the inherited functions and modifiers
+        '''
+        return self.functions_inherited + self.modifiers_inherited
+
+    @property
+    def functions_and_modifiers_not_inherited(self):
+        '''
+            list(Function|Modifier): List of the functions and modifiers defined within the contract (not inherited)
+        '''
+        return self.functions_not_inherited + self.modifiers_not_inherited
+
+    def get_functions_overridden_by(self, function):
+        '''
+            Return the list of functions overriden by the function
+        Args:
+            (core.Function)
+        Returns:
+            list(core.Function)
+
+        '''
+        candidates = [c.functions_not_inherited for c in self.inheritance]
+        candidates = [candidate for sublist in candidates for candidate in sublist]
+        return [f for f in candidates if f.full_name == function.full_name]
+
     @property
     def all_functions_called(self):
         '''

slither/core/declarations/function.py

@@ -319,6 +319,18 @@ def full_name(self):
         name, parameters, _ = self.signature
         return name+'('+','.join(parameters)+')'
 
+    @property
+    def functions_shadowed(self):
+        '''
+            Return the list of functions shadowed
+        Returns:
+            list(core.Function)
+
+        '''
+        candidates = [c.functions_not_inherited for c in self.contract.inheritance]
+        candidates = [candidate for sublist in candidates for candidate in sublist]
+        return [f for f in candidates if f.full_name == self.full_name]
+
 
     @property
     def slither(self):

slither/detectors/functions/external_function.py

@@ -27,7 +27,10 @@ def detect_functions_called(contract):
             (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,38 +39,139 @@ def detect_functions_called(contract):
 
     @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:
                     if isinstance(ir, (InternalDynamicCall)):
                         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/expected_json/external_function_2.external-function.json

@@ -0,0 +1 @@
+[]