Merge pull request #340 from crytic/dev-fix-c3

Fix incorrect inheritance order

slither/core/declarations/contract.py

@@ -386,7 +386,7 @@ class Contract(ChildSlither, SourceMapping):
         accessible_elements = {}
         contracts_visited = []
         for father in self.inheritance_reverse:
-            functions = {v.full_name: v for (_, v) in getter_available(father)
+            functions = {v.full_name: v for (v) in getter_available(father)
                          if not v.contract in contracts_visited}
             contracts_visited.append(father)
             inherited_elements.update(functions)

slither/core/declarations/function.py

@@ -126,6 +126,7 @@ class Function(ChildContract, ChildInheritance, SourceMapping):
         self._all_solidity_variables_used_as_args = None
 
         self._is_shadowed = False
+        self._shadows = False
 
         # set(ReacheableNode)
         self._reachable_from_nodes = set()
@@ -316,6 +317,14 @@ class Function(ChildContract, ChildInheritance, SourceMapping):
     def is_shadowed(self, is_shadowed):
         self._is_shadowed = is_shadowed
 
+    @property
+    def shadows(self):
+        return self._shadows
+
+    @shadows.setter
+    def shadows(self, _shadows):
+        self._shadows = _shadows
+
     # endregion
     ###################################################################################
     ###################################################################################

slither/printers/inheritance/inheritance_graph.py

@@ -10,7 +10,6 @@ from slither.core.declarations.contract import Contract
 from slither.core.solidity_types.user_defined_type import UserDefinedType
 from slither.printers.abstract_printer import AbstractPrinter
 from slither.utils.inheritance_analysis import (detect_c3_function_shadowing,
-                                                detect_function_shadowing,
                                                 detect_state_variable_shadowing)
 
 
@@ -26,19 +25,6 @@ class PrinterInheritanceGraph(AbstractPrinter):
         inheritance = [x.inheritance for x in slither.contracts]
         self.inheritance = set([item for sublist in inheritance for item in sublist])
 
-        # Create a lookup of direct shadowing instances.
-        self.direct_overshadowing_functions = {}
-        shadows = detect_function_shadowing(slither.contracts, True, False)
-        for overshadowing_instance in shadows:
-            overshadowing_function = overshadowing_instance[2]
-
-            # Add overshadowing function entry.
-            if overshadowing_function not in self.direct_overshadowing_functions:
-                self.direct_overshadowing_functions[overshadowing_function] = set()
-            self.direct_overshadowing_functions[overshadowing_function].add(overshadowing_instance)
-
-        # Create a lookup of shadowing state variables.
-        # Format: { colliding_variable : set([colliding_variables]) }
         self.overshadowing_state_variables = {}
         shadows = detect_state_variable_shadowing(slither.contracts)
         for overshadowing_instance in shadows:
@@ -55,7 +41,7 @@ class PrinterInheritanceGraph(AbstractPrinter):
         func_name = func.full_name
         pattern = '<TR><TD align="left">    %s</TD></TR>'
         pattern_shadow = '<TR><TD align="left"><font color="#FFA500">    %s</font></TD></TR>'
-        if func in self.direct_overshadowing_functions:
+        if func.shadows:
             return pattern_shadow % func_name
         return pattern % func_name
 
@@ -89,12 +75,10 @@ class PrinterInheritanceGraph(AbstractPrinter):
         # If this variable is an overshadowing variable, we'll want to return information describing it.
         result = []
         indirect_shadows = detect_c3_function_shadowing(contract)
-        if indirect_shadows:
-            for collision_set in sorted(indirect_shadows, key=lambda x: x[0][1].name):
-                winner = collision_set[-1][1].contract_declarer.name
-                collision_steps = [colliding_function.contract_declarer.name for _, colliding_function in collision_set]
-                collision_steps = ', '.join(collision_steps)
-                result.append(f"'{collision_set[0][1].full_name}' collides in inherited contracts {collision_steps} where {winner} is chosen.")
+        for winner, colliding_functions in indirect_shadows.items():
+            collision_steps = ', '.join([f.contract_declarer.name
+                                         for f in colliding_functions] + [winner.contract_declarer.name])
+            result.append(f"'{winner.full_name}' collides in inherited contracts {collision_steps} where {winner.contract_declarer.name} is chosen.")
         return '\n'.join(result)
 
     def _get_port_id(self, var, contract):
@@ -116,10 +100,12 @@ class PrinterInheritanceGraph(AbstractPrinter):
         # Functions
         visibilities = ['public', 'external']
         public_functions = [self._get_pattern_func(f, contract) for f in contract.functions if
-                            not f.is_constructor and f.contract_declarer == contract and f.visibility in visibilities]
+                            not f.is_constructor and not f.is_constructor_variables
+                            and f.contract_declarer == contract and f.visibility in visibilities]
         public_functions = ''.join(public_functions)
         private_functions = [self._get_pattern_func(f, contract) for f in contract.functions if
-                             not f.is_constructor and f.contract_declarer == contract and f.visibility not in visibilities]
+                             not f.is_constructor and not f.is_constructor_variables
+                             and f.contract_declarer == contract and f.visibility not in visibilities]
         private_functions = ''.join(private_functions)
 
         # Modifiers

slither/printers/summary/slithir.py

@@ -23,7 +23,7 @@ class PrinterSlithIR(AbstractPrinter):
         for contract in self.contracts:
             print('Contract {}'.format(contract.name))
             for function in contract.functions:
-                print(f'\tFunction {function.canonical_name}')
+                print(f'\tFunction {function.canonical_name} {"" if function.is_shadowed else "(*)"}')
                 for node in function.nodes:
                     if node.expression:
                         print('\t\tExpression: {}'.format(node.expression))

slither/solc_parsing/declarations/contract.py

@@ -290,7 +290,7 @@ class ContractSolc04(Contract):
 
         elements_no_params = self._modifiers_no_params
         getter = lambda f: f.modifiers
-        getter_available = lambda f: f.available_modifiers_as_dict().items()
+        getter_available = lambda f: f.modifiers_declared
         Cls = ModifierSolc
         self._modifiers = self._analyze_params_elements(elements_no_params, getter, getter_available, Cls)
 
@@ -302,7 +302,7 @@ class ContractSolc04(Contract):
 
         elements_no_params = self._functions_no_params
         getter = lambda f: f.functions
-        getter_available = lambda f: f.available_functions_as_dict().items()
+        getter_available = lambda f: f.functions_declared
         Cls = FunctionSolc
         self._functions = self._analyze_params_elements(elements_no_params, getter, getter_available, Cls)
 
@@ -354,6 +354,7 @@ class ContractSolc04(Contract):
         for element in all_elements.values():
             if accessible_elements[element.full_name] != all_elements[element.canonical_name]:
                 element.is_shadowed = True
+                accessible_elements[element.full_name].shadows = True
 
         return all_elements
 

slither/solc_parsing/expressions/expression_parsing.py

@@ -117,18 +117,18 @@ def find_variable(var_name, caller_context, referenced_declaration=None, is_supe
         return conc_variables_ptr[var_name]
 
     if is_super:
-        getter_available = lambda f: f.available_functions_as_dict().items()
+        getter_available = lambda f: f.functions_declared
         d = {f.canonical_name:f for f in contract.functions}
-        functions = {f.full_name:f for f in contract.available_elements_from_inheritances(d, getter_available).values()}
+        functions = {f.full_name:f for f in contract_declarer.available_elements_from_inheritances(d, getter_available).values()}
     else:
         functions = contract.available_functions_as_dict()
     if var_name in functions:
         return functions[var_name]
 
     if is_super:
-        getter_available = lambda m: m.available_modifiers_as_dict().items()
+        getter_available = lambda m: m.modifiers_declared
         d = {m.canonical_name: m for m in contract.modifiers}
-        modifiers = {m.full_name: m for m in contract.available_elements_from_inheritances(d, getter_available).values()}
+        modifiers = {m.full_name: m for m in contract_declarer.available_elements_from_inheritances(d, getter_available).values()}
     else:
         modifiers = contract.available_modifiers_as_dict()
     if var_name in modifiers:

slither/utils/inheritance_analysis.py

@@ -2,6 +2,7 @@
 Detects various properties of inheritance in provided contracts.
 """
 
+from collections import defaultdict
 
 def detect_c3_function_shadowing(contract):
     """
@@ -9,112 +10,21 @@ def detect_c3_function_shadowing(contract):
     properties, despite not directly inheriting from each other.
 
     :param contract: The contract to check for potential C3 linearization shadowing within.
-    :return: A list of list of tuples: (contract, function), where each inner list describes colliding functions.
-    The later elements in the inner list overshadow the earlier ones. The contract-function pair's function does not
-    need to be defined in its paired contract, it may have been inherited within it.
+    :return: A dict (function winner -> [shadowed functions])
     """
 
-    # Loop through all contracts, and all underlying functions.
-    results = {}
-    for i in range(0, len(contract.immediate_inheritance) - 1):
-        inherited_contract1 = contract.immediate_inheritance[i]
+    targets = {f.full_name:f for f in contract.functions_inherited if f.shadows and not f.is_constructor
+               and not f.is_constructor_variables}
 
-        for function1 in inherited_contract1.functions_and_modifiers_declared:
-            # If this function has already be handled or is unimplemented, we skip it
-            if function1.full_name in results or function1.is_constructor or not function1.is_implemented:
-                continue
-
-            # Define our list of function instances which overshadow each other.
-            functions_matching = [(inherited_contract1, function1)]
-            already_processed = set([function1])
-
-            # Loop again through other contracts and functions to compare to.
-            for x in range(i + 1, len(contract.immediate_inheritance)):
-                inherited_contract2 = contract.immediate_inheritance[x]
-
-                # Loop for each function in this contract
-                for function2 in inherited_contract2.functions_and_modifiers:
-                    # Skip this function if it is the last function that was shadowed.
-                    if function2 in already_processed or function2.is_constructor or not function2.is_implemented:
-                        continue
-
-                    # If this function does have the same full name, it is shadowing through C3 linearization.
-                    if function1.full_name == function2.full_name:
-                        functions_matching.append((inherited_contract2, function2))
-                        already_processed.add(function2)
-
-            # If we have more than one definition matching the same signature, we add it to the results.
-            if len(functions_matching) > 1:
-                results[function1.full_name] = functions_matching
-
-    return list(results.values())
+    collisions = defaultdict(set)
 
-
-def detect_direct_function_shadowing(contract):
-    """
-    Detects and obtains functions which are shadowed immediately by the provided ancestor contract.
-    :param contract: The ancestor contract which we check for function shadowing within.
-    :return: A list of tuples (overshadowing_function, overshadowed_immediate_base_contract, overshadowed_function)
-    -overshadowing_function is the function defined within the provided contract that overshadows another
-    definition.
-    -overshadowed_immediate_base_contract is the immediate inherited-from contract that provided the shadowed
-    function (could have provided it through inheritance, does not need to directly define it).
-    -overshadowed_function is the function definition which is overshadowed by the provided contract's definition.
-    """
-    functions_declared = {function.full_name: function for function in contract.functions_and_modifiers_declared}
-    results = {}
-    for base_contract in reversed(contract.immediate_inheritance):
-        for base_function in base_contract.functions_and_modifiers:
-
-            # We already found the most immediate shadowed definition for this function, skip to the next.
-            if base_function.full_name in results:
+    for contract_inherited in contract.immediate_inheritance:
+        for candidate in contract_inherited.functions:
+            if candidate.full_name not in targets or candidate.is_shadowed:
                 continue
-
-            # If this function is implemented and it collides with a definition in our immediate contract, we add
-            # it to our results.
-            if base_function.is_implemented and base_function.full_name in functions_declared:
-                results[base_function.full_name] = (functions_declared[base_function.full_name], base_contract, base_function)
-
-    return list(results.values())
-
-
-def detect_function_shadowing(contracts, direct_shadowing=True, indirect_shadowing=True):
-    """
-    Detects all overshadowing and overshadowed functions in the provided contracts.
-    :param contracts: The contracts to detect shadowing within.
-    :param direct_shadowing: Include results from direct inheritance/inheritance ancestry.
-    :param indirect_shadowing: Include results from indirect inheritance collisions as a result of multiple
-    inheritance/c3 linearization.
-    :return: Returns a set of tuples(contract_scope, overshadowing_contract, overshadowing_function,
-    overshadowed_contract, overshadowed_function), where:
-    -The contract_scope defines where the detection of shadowing is most immediately found.
-    -For each contract-function pair, contract is the first contract where the function is seen, while the function
-    refers to the actual definition. The function does not need to be defined in the contract (could be inherited).
-    """
-    results = set()
-    for contract in contracts:
-
-        # Detect immediate inheritance shadowing.
-        if direct_shadowing:
-            shadows = detect_direct_function_shadowing(contract)
-            for (overshadowing_function, overshadowed_base_contract, overshadowed_function) in shadows:
-                results.add((contract, contract, overshadowing_function, overshadowed_base_contract,
-                             overshadowed_function))
-
-        # Detect c3 linearization shadowing (multi inheritance shadowing).
-        if indirect_shadowing:
-            shadows = detect_c3_function_shadowing(contract)
-            for colliding_functions in shadows:
-                for x in range(0, len(colliding_functions) - 1):
-                    for y in range(x + 1, len(colliding_functions)):
-                        # The same function definition can appear more than once in the inheritance chain,
-                        # overshadowing items between, so it is important to remember to filter it out here.
-                        if colliding_functions[y][1].contract_declarer != colliding_functions[x][1].contract_declarer:
-                            results.add((contract, colliding_functions[y][0], colliding_functions[y][1],
-                                         colliding_functions[x][0], colliding_functions[x][1]))
-
-    return results
-
+            if targets[candidate.full_name].canonical_name != candidate.canonical_name:
+                collisions[targets[candidate.full_name]].add(candidate)
+    return collisions
 
 def detect_state_variable_shadowing(contracts):
     """