Merge pull request #2403 from crytic/fix/scoping

resolve available definitions from import by reference ID

slither/core/declarations/contract.py

@@ -10,7 +10,7 @@
 
 from slither.core.cfg.scope import Scope
 from slither.core.source_mapping.source_mapping import SourceMapping
-from slither.utils.using_for import USING_FOR, _merge_using_for
+from slither.utils.using_for import USING_FOR, merge_using_for
 from slither.core.declarations.function import Function, FunctionType, FunctionLanguage
 from slither.utils.erc import (
     ERC20_signatures,
@@ -342,7 +342,7 @@ def using_for_complete(self) -> USING_FOR:
             result = self.using_for
             top_level_using_for = self.file_scope.using_for_directives
             for uftl in top_level_using_for:
-                result = _merge_using_for(result, uftl.using_for)
+                result = merge_using_for(result, uftl.using_for)
             self._using_for_complete = result
         return self._using_for_complete
 

slither/core/declarations/function.py

@@ -355,8 +355,7 @@ def is_checked(self) -> bool:
     @property
     def id(self) -> Optional[str]:
         """
-        Return the ID of the funciton. For Solidity with compact-AST the ID is the reference ID
-        For other, the ID is None
+        Return the reference ID of the function, if available.
 
         :return:
         :rtype:

slither/core/declarations/function_contract.py

@@ -65,7 +65,10 @@ def is_declared_by(self, contract: "Contract") -> bool:
 
     @property
     def file_scope(self) -> "FileScope":
-        return self.contract.file_scope
+        # This is the contract declarer's file scope because inherited functions have access
+        # to the file scope which their declared in. This scope may contain references not
+        # available in the child contract's scope. See inherited_function_scope.sol for an example.
+        return self.contract_declarer.file_scope
 
     # endregion
     ###################################################################################

slither/core/declarations/function_top_level.py

@@ -5,7 +5,7 @@
 
 from slither.core.declarations import Function
 from slither.core.declarations.top_level import TopLevel
-from slither.utils.using_for import USING_FOR, _merge_using_for
+from slither.utils.using_for import USING_FOR, merge_using_for
 
 if TYPE_CHECKING:
     from slither.core.compilation_unit import SlitherCompilationUnit
@@ -32,7 +32,7 @@ def using_for_complete(self) -> USING_FOR:
         if self._using_for_complete is None:
             result = {}
             for uftl in self.file_scope.using_for_directives:
-                result = _merge_using_for(result, uftl.using_for)
+                result = merge_using_for(result, uftl.using_for)
             self._using_for_complete = result
         return self._using_for_complete
 

slither/core/declarations/import_directive.py

@@ -11,8 +11,8 @@ class Import(SourceMapping):
     def __init__(self, filename: Path, scope: "FileScope") -> None:
         super().__init__()
         self._filename: Path = filename
-        self._alias: Optional[str] = None
         self.scope: "FileScope" = scope
+        self._alias: Optional[str] = None
         # Map local name -> original name
         self.renaming: Dict[str, str] = {}
 

slither/core/declarations/using_for_top_level.py

@@ -1,6 +1,5 @@
-from typing import TYPE_CHECKING, List, Dict, Union
+from typing import TYPE_CHECKING
 
-from slither.core.solidity_types.type import Type
 from slither.core.declarations.top_level import TopLevel
 from slither.utils.using_for import USING_FOR
 
@@ -11,7 +10,7 @@
 class UsingForTopLevel(TopLevel):
     def __init__(self, scope: "FileScope") -> None:
         super().__init__()
-        self._using_for: Dict[Union[str, Type], List[Type]] = {}
+        self._using_for: USING_FOR = {}
         self.file_scope: "FileScope" = scope
 
     @property

slither/core/scope/scope.py

@@ -29,6 +29,7 @@ class FileScope:
     def __init__(self, filename: Filename) -> None:
         self.filename = filename
         self.accessible_scopes: List[FileScope] = []
+        self.exported_symbols: Set[int] = set()
 
         self.contracts: Dict[str, Contract] = {}
         # Custom error are a list instead of a dict
@@ -56,53 +57,39 @@ def __init__(self, filename: Filename) -> None:
         # Name -> type alias
         self.type_aliases: Dict[str, TypeAlias] = {}
 
-    def add_accesible_scopes(self) -> bool:  # pylint: disable=too-many-branches
+    def add_accessible_scopes(self) -> bool:  # pylint: disable=too-many-branches
         """
         Add information from accessible scopes. Return true if new information was obtained
-
         :return:
         :rtype:
         """
 
         learn_something = False
-
         for new_scope in self.accessible_scopes:
-            if not _dict_contain(new_scope.contracts, self.contracts):
-                self.contracts.update(new_scope.contracts)
-                learn_something = True
-            if not new_scope.custom_errors.issubset(self.custom_errors):
-                self.custom_errors |= new_scope.custom_errors
-                learn_something = True
-            if not _dict_contain(new_scope.enums, self.enums):
-                self.enums.update(new_scope.enums)
-                learn_something = True
-            if not new_scope.events.issubset(self.events):
-                self.events |= new_scope.events
-                learn_something = True
-            if not new_scope.functions.issubset(self.functions):
-                self.functions |= new_scope.functions
-                learn_something = True
+            # To support using for directives on user defined types and user defined functions,
+            # we need to propagate the using for directives from the imported file to the importing file
+            # since it is not reflected in the "exportedSymbols" field of the AST.
             if not new_scope.using_for_directives.issubset(self.using_for_directives):
                 self.using_for_directives |= new_scope.using_for_directives
                 learn_something = True
-            if not new_scope.imports.issubset(self.imports):
-                self.imports |= new_scope.imports
-                learn_something = True
-            if not new_scope.pragmas.issubset(self.pragmas):
-                self.pragmas |= new_scope.pragmas
+            if not _dict_contain(new_scope.type_aliases, self.type_aliases):
+                self.type_aliases.update(new_scope.type_aliases)
                 learn_something = True
-            if not _dict_contain(new_scope.structures, self.structures):
-                self.structures.update(new_scope.structures)
+            if not new_scope.functions.issubset(self.functions):
+                self.functions |= new_scope.functions
                 learn_something = True
-            if not _dict_contain(new_scope.variables, self.variables):
-                self.variables.update(new_scope.variables)
+
+            # To get around this bug for aliases https://github.com/ethereum/solidity/pull/11881,
+            # we propagate the exported_symbols from the imported file to the importing file
+            # See tests/e2e/solc_parsing/test_data/top-level-nested-import-0.7.1.sol
+            if not new_scope.exported_symbols.issubset(self.exported_symbols):
+                self.exported_symbols |= new_scope.exported_symbols
                 learn_something = True
+
+            # This is need to support aliasing when we do a late lookup using SolidityImportPlaceholder
             if not _dict_contain(new_scope.renaming, self.renaming):
                 self.renaming.update(new_scope.renaming)
                 learn_something = True
-            if not _dict_contain(new_scope.type_aliases, self.type_aliases):
-                self.type_aliases.update(new_scope.type_aliases)
-                learn_something = True
 
         return learn_something
 

slither/slither.py

@@ -1,11 +1,10 @@
 import logging
-from typing import Union, List, ValuesView, Type, Dict, Optional
+from typing import Union, List, Type, Dict, Optional
 
 from crytic_compile import CryticCompile, InvalidCompilation
 
 # pylint: disable= no-name-in-module
 from slither.core.compilation_unit import SlitherCompilationUnit
-from slither.core.scope.scope import FileScope
 from slither.core.slither_core import SlitherCore
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 from slither.exceptions import SlitherError
@@ -27,32 +26,71 @@ def _check_common_things(
 ) -> None:
 
     if not issubclass(cls, base_cls) or cls is base_cls:
-        raise Exception(
+        raise SlitherError(
             f"You can't register {cls!r} as a {thing_name}. You need to pass a class that inherits from {base_cls.__name__}"
         )
 
     if any(type(obj) == cls for obj in instances_list):  # pylint: disable=unidiomatic-typecheck
-        raise Exception(f"You can't register {cls!r} twice.")
+        raise SlitherError(f"You can't register {cls!r} twice.")
 
 
-def _update_file_scopes(candidates: ValuesView[FileScope]):
+def _update_file_scopes(
+    sol_parser: SlitherCompilationUnitSolc,
+):  # pylint: disable=too-many-branches
     """
-    Because solc's import allows cycle in the import
-    We iterate until we aren't adding new information to the scope
-
+    Since all definitions in a file are exported by default, including definitions from its (transitive) dependencies,
+    we can identify all top level items that could possibly be referenced within the file from its exportedSymbols.
+    It is not as straightforward for user defined types and functions as well as aliasing. See add_accessible_scopes for more details.
     """
+    candidates = sol_parser.compilation_unit.scopes.values()
     learned_something = False
+    # Because solc's import allows cycle in the import graph, iterate until we aren't adding new information to the scope.
     while True:
         for candidate in candidates:
-            learned_something |= candidate.add_accesible_scopes()
+            learned_something |= candidate.add_accessible_scopes()
         if not learned_something:
             break
         learned_something = False
 
+    for scope in candidates:
+        for refId in scope.exported_symbols:
+            if refId in sol_parser.contracts_by_id:
+                contract = sol_parser.contracts_by_id[refId]
+                scope.contracts[contract.name] = contract
+            elif refId in sol_parser.functions_by_id:
+                functions = sol_parser.functions_by_id[refId]
+                assert len(functions) == 1
+                scope.functions.add(functions[0])
+            elif refId in sol_parser.imports_by_id:
+                import_directive = sol_parser.imports_by_id[refId]
+                scope.imports.add(import_directive)
+            elif refId in sol_parser.top_level_variables_by_id:
+                top_level_variable = sol_parser.top_level_variables_by_id[refId]
+                scope.variables[top_level_variable.name] = top_level_variable
+            elif refId in sol_parser.top_level_events_by_id:
+                top_level_event = sol_parser.top_level_events_by_id[refId]
+                scope.events.add(top_level_event)
+            elif refId in sol_parser.top_level_structures_by_id:
+                top_level_struct = sol_parser.top_level_structures_by_id[refId]
+                scope.structures[top_level_struct.name] = top_level_struct
+            elif refId in sol_parser.top_level_type_aliases_by_id:
+                top_level_type_alias = sol_parser.top_level_type_aliases_by_id[refId]
+                scope.type_aliases[top_level_type_alias.name] = top_level_type_alias
+            elif refId in sol_parser.top_level_enums_by_id:
+                top_level_enum = sol_parser.top_level_enums_by_id[refId]
+                scope.enums[top_level_enum.name] = top_level_enum
+            elif refId in sol_parser.top_level_errors_by_id:
+                top_level_custom_error = sol_parser.top_level_errors_by_id[refId]
+                scope.custom_errors.add(top_level_custom_error)
+            else:
+                logger.error(
+                    f"Failed to resolved name for reference id {refId} in {scope.filename.absolute}."
+                )
+
 
 class Slither(
     SlitherCore
-):  # pylint: disable=too-many-instance-attributes,too-many-locals,too-many-statements
+):  # pylint: disable=too-many-instance-attributes,too-many-locals,too-many-statements,too-many-branches
     def __init__(self, target: Union[str, CryticCompile], **kwargs) -> None:
         """
         Args:
@@ -118,7 +156,18 @@ def __init__(self, target: Union[str, CryticCompile], **kwargs) -> None:
                     sol_parser.parse_top_level_items(ast, path)
                     self.add_source_code(path)
 
-                _update_file_scopes(compilation_unit_slither.scopes.values())
+                for contract in sol_parser._underlying_contract_to_parser:
+                    if contract.name.startswith("SlitherInternalTopLevelContract"):
+                        raise SlitherError(
+                            # region multi-line-string
+                            """Your codebase has a contract named 'SlitherInternalTopLevelContract'.
+        Please rename it, this name is reserved for Slither's internals"""
+                            # endregion multi-line
+                        )
+                    sol_parser._contracts_by_id[contract.id] = contract
+                    sol_parser._compilation_unit.contracts.append(contract)
+
+                _update_file_scopes(sol_parser)
 
         if kwargs.get("generate_patches", False):
             self.generate_patches = True

slither/slithir/convert.py

@@ -1502,7 +1502,6 @@ def convert_to_pop(ir: HighLevelCall, node: "Node") -> List[Operation]:
 
 
 def look_for_library_or_top_level(
-    contract: Contract,
     ir: HighLevelCall,
     using_for,
     t: Union[
@@ -1536,8 +1535,9 @@ def look_for_library_or_top_level(
         lib_contract = None
         if isinstance(destination, FunctionContract) and destination.contract.is_library:
             lib_contract = destination.contract
-        elif not isinstance(destination, FunctionTopLevel):
-            lib_contract = contract.file_scope.get_contract_from_name(str(destination))
+        elif isinstance(destination, UserDefinedType) and isinstance(destination.type, Contract):
+            lib_contract = destination.type
+
         if lib_contract:
             lib_call = LibraryCall(
                 lib_contract,
@@ -1561,20 +1561,14 @@ def look_for_library_or_top_level(
 def convert_to_library_or_top_level(
     ir: HighLevelCall, node: "Node", using_for
 ) -> Optional[Union[LibraryCall, InternalCall,]]:
-    # We use contract_declarer, because Solidity resolve the library
-    # before resolving the inheritance.
-    # Though we could use .contract as libraries cannot be shadowed
-    contract = (
-        node.function.contract_declarer if isinstance(node.function, FunctionContract) else None
-    )
     t = ir.destination.type
     if t in using_for:
-        new_ir = look_for_library_or_top_level(contract, ir, using_for, t)
+        new_ir = look_for_library_or_top_level(ir, using_for, t)
         if new_ir:
             return new_ir
 
     if "*" in using_for:
-        new_ir = look_for_library_or_top_level(contract, ir, using_for, "*")
+        new_ir = look_for_library_or_top_level(ir, using_for, "*")
         if new_ir:
             return new_ir
 
@@ -1585,7 +1579,7 @@ def convert_to_library_or_top_level(
         and UserDefinedType(node.function.contract) in using_for
     ):
         new_ir = look_for_library_or_top_level(
-            contract, ir, using_for, UserDefinedType(node.function.contract)
+            ir, using_for, UserDefinedType(node.function.contract)
         )
         if new_ir:
             return new_ir
@@ -1740,7 +1734,10 @@ def convert_type_of_high_and_internal_level_call(
             ]
 
             for import_statement in contract.file_scope.imports:
-                if import_statement.alias and import_statement.alias == ir.contract_name:
+                if (
+                    import_statement.alias is not None
+                    and import_statement.alias == ir.contract_name
+                ):
                     imported_scope = contract.compilation_unit.get_scope(import_statement.filename)
                     candidates += [
                         f
@@ -1771,7 +1768,7 @@ def convert_type_of_high_and_internal_level_call(
         if func is None and candidates:
             func = _find_function_from_parameter(ir.arguments, candidates, False)
 
-    # lowlelvel lookup needs to be done at last step
+    # low level lookup needs to be done as last step
     if not func:
         if can_be_low_level(ir):
             return convert_to_low_level(ir)

slither/solc_parsing/declarations/contract.py

@@ -585,7 +585,9 @@ def _analyze_params_elements(  # pylint: disable=too-many-arguments,too-many-loc
                     element.is_shadowed = True
                     accessible_elements[element.full_name].shadows = True
         except (VariableNotFound, KeyError) as e:
-            self.log_incorrect_parsing(f"Missing params {e}")
+            self.log_incorrect_parsing(
+                f"Missing params {e} {self._contract.source_mapping.to_detailed_str()}"
+            )
         return all_elements
 
     def analyze_constant_state_variables(self) -> None:

slither/solc_parsing/declarations/function.py

@@ -63,10 +63,12 @@ def __init__(
         # Only present if compact AST
         if self.is_compact_ast:
             self._function.name = function_data["name"]
-            if "id" in function_data:
-                self._function.id = function_data["id"]
         else:
             self._function.name = function_data["attributes"][self.get_key()]
+
+        if "id" in function_data:
+            self._function.id = function_data["id"]
+
         self._functionNotParsed = function_data
         self._returnsNotParsed: List[dict] = []
         self._params_was_analyzed = False

slither/solc_parsing/expressions/find_variable.py

@@ -305,6 +305,7 @@ def _find_variable_init(
         else:
             assert isinstance(underlying_function, FunctionContract)
             scope = underlying_function.contract.file_scope
+
     elif isinstance(caller_context, StructureTopLevelSolc):
         direct_contracts = []
         direct_functions_parser = []
@@ -505,4 +506,6 @@ def find_variable(
     if ret:
         return ret, False
 
-    raise VariableNotFound(f"Variable not found: {var_name} (context {contract})")
+    raise VariableNotFound(
+        f"Variable not found: {var_name} (context {contract} {contract.source_mapping.to_detailed_str()})"
+    )