Merge pull request #2121 from crytic/fix/prioritize-reference-id

prioritize reference id in name resolution to avoid shadowing issues

slither/core/declarations/contract.py

@@ -78,6 +78,8 @@ def __init__(self, compilation_unit: "SlitherCompilationUnit", scope: "FileScope
         # do not contain private variables inherited from contract
         self._variables: Dict[str, "StateVariable"] = {}
         self._variables_ordered: List["StateVariable"] = []
+        # Reference id -> variable declaration (only available for compact AST)
+        self._state_variables_by_ref_id: Dict[int, "StateVariable"] = {}
         self._modifiers: Dict[str, "Modifier"] = {}
         self._functions: Dict[str, "FunctionContract"] = {}
         self._linearizedBaseContracts: List[int] = []
@@ -404,6 +406,12 @@ def type_aliases_as_dict(self) -> Dict[str, "TypeAliasContract"]:
     # region Variables
     ###################################################################################
     ###################################################################################
+    @property
+    def state_variables_by_ref_id(self) -> Dict[int, "StateVariable"]:
+        """
+        Returns the state variables by reference id (only available for compact AST).
+        """
+        return self._state_variables_by_ref_id
 
     @property
     def variables(self) -> List["StateVariable"]:

slither/solc_parsing/declarations/contract.py

@@ -357,6 +357,8 @@ def parse_state_variables(self) -> None:
             self._variables_parser.append(var_parser)
 
             assert var.name
+            if var_parser.reference_id is not None:
+                self._contract.state_variables_by_ref_id[var_parser.reference_id] = var
             self._contract.variables_as_dict[var.name] = var
             self._contract.add_variables_ordered([var])
 

slither/solc_parsing/expressions/expression_parsing.py

@@ -487,13 +487,18 @@ def parse_expression(expression: Dict, caller_context: CallerContextExpression)
 
         t = None
 
+        referenced_declaration = None
         if caller_context.is_compact_ast:
             value = expression["name"]
             t = expression["typeDescriptions"]["typeString"]
+            if "referencedDeclaration" in expression:
+                referenced_declaration = expression["referencedDeclaration"]
         else:
             value = expression["attributes"]["value"]
             if "type" in expression["attributes"]:
                 t = expression["attributes"]["type"]
+            if "referencedDeclaration" in expression["attributes"]:
+                referenced_declaration = expression["attributes"]["referencedDeclaration"]
 
         if t:
             found = re.findall(r"[struct|enum|function|modifier] \(([\[\] ()a-zA-Z0-9\.,_]*)\)", t)
@@ -502,10 +507,6 @@ def parse_expression(expression: Dict, caller_context: CallerContextExpression)
                 value = value + "(" + found[0] + ")"
                 value = filter_name(value)
 
-        if "referencedDeclaration" in expression:
-            referenced_declaration = expression["referencedDeclaration"]
-        else:
-            referenced_declaration = None
         var, was_created = find_variable(value, caller_context, referenced_declaration)
         if was_created:
             var.set_offset(src, caller_context.compilation_unit)

slither/solc_parsing/expressions/find_variable.py

@@ -56,10 +56,24 @@ def _find_variable_from_ref_declaration(
     referenced_declaration: Optional[int],
     all_contracts: List["Contract"],
     all_functions: List["Function"],
+    function_parser: Optional["FunctionSolc"],
+    contract_declarer: Optional["Contract"],
 ) -> Optional[Union[Contract, Function]]:
+    """
+    Reference declarations take the highest priority, but they are not available for legacy AST.
+    """
     if referenced_declaration is None:
         return None
-    # id of the contracts is the referenced declaration
+    # We look for variable declared with the referencedDeclaration attribute
+    if function_parser is not None and referenced_declaration in function_parser.variables_renamed:
+        return function_parser.variables_renamed[referenced_declaration].underlying_variable
+
+    if (
+        contract_declarer is not None
+        and referenced_declaration in contract_declarer.state_variables_by_ref_id
+    ):
+        return contract_declarer.state_variables_by_ref_id[referenced_declaration]
+    # Ccontracts  ids are the referenced declaration
     # This is not true for the functions, as we dont always have the referenced_declaration
     # But maybe we could? (TODO)
     for contract_candidate in all_contracts:
@@ -74,14 +88,9 @@ def _find_variable_from_ref_declaration(
 def _find_variable_in_function_parser(
     var_name: str,
     function_parser: Optional["FunctionSolc"],
-    referenced_declaration: Optional[int] = None,
 ) -> Optional[Variable]:
     if function_parser is None:
         return None
-    # We look for variable declared with the referencedDeclaration attr
-    func_variables_renamed = function_parser.variables_renamed
-    if referenced_declaration and referenced_declaration in func_variables_renamed:
-        return func_variables_renamed[referenced_declaration].underlying_variable
     # If not found, check for name
     func_variables = function_parser.underlying_function.variables_as_dict
     if var_name in func_variables:
@@ -391,20 +400,6 @@ def find_variable(
     if var_name in current_scope.renaming:
         var_name = current_scope.renaming[var_name]
 
-    # Use ret0/ret1 to help mypy
-    ret0 = _find_variable_from_ref_declaration(
-        referenced_declaration, direct_contracts, direct_functions
-    )
-    if ret0:
-        return ret0, False
-
-    function_parser: Optional[FunctionSolc] = (
-        caller_context if isinstance(caller_context, FunctionSolc) else None
-    )
-    ret1 = _find_variable_in_function_parser(var_name, function_parser, referenced_declaration)
-    if ret1:
-        return ret1, False
-
     contract: Optional[Contract] = None
     contract_declarer: Optional[Contract] = None
     if isinstance(caller_context, ContractSolc):
@@ -427,6 +422,24 @@ def find_variable(
                 if var_name == var.name:
                     return var, False
 
+    function_parser: Optional[FunctionSolc] = (
+        caller_context if isinstance(caller_context, FunctionSolc) else None
+    )
+    # Use ret0/ret1 to help mypy
+    ret0 = _find_variable_from_ref_declaration(
+        referenced_declaration,
+        direct_contracts,
+        direct_functions,
+        function_parser,
+        contract_declarer,
+    )
+    if ret0:
+        return ret0, False
+
+    ret1 = _find_variable_in_function_parser(var_name, function_parser)
+    if ret1:
+        return ret1, False
+
     ret = _find_in_contract(var_name, contract, contract_declarer, is_super, is_identifier_path)
     if ret:
         return ret, False
@@ -477,6 +490,8 @@ def find_variable(
         referenced_declaration,
         list(current_scope.contracts.values()),
         list(current_scope.functions),
+        None,
+        None,
     )
     if ret:
         return ret, False

tests/unit/core/test_data/name_resolution/shadowing_compact.sol

@@ -0,0 +1,8 @@
+pragma solidity 0.8.0;
+contract B {
+    uint public x = 21;
+    function a()  public {
+        uint u = 2 * x;
+        uint x;
+    }
+}

tests/unit/core/test_data/name_resolution/shadowing_legacy_post_0_5_0.sol

@@ -0,0 +1,8 @@
+pragma solidity 0.5.0;
+contract B {
+    uint public x = 21;
+    function a()  public {
+        uint u = 2 * x;
+        uint x;
+    }
+}

tests/unit/core/test_data/name_resolution/shadowing_legacy_pre_0_5_0.sol

@@ -0,0 +1,8 @@
+pragma solidity 0.4.12;
+contract B {
+    uint public x = 21;
+    function a()  public {
+        uint u = 2 * x;
+        uint x;
+    }
+}

tests/unit/core/test_name_resolution.py

@@ -0,0 +1,45 @@
+from pathlib import Path
+
+from slither import Slither
+
+
+TEST_DATA_DIR = Path(__file__).resolve().parent / "test_data"
+NAME_RESOLUTION_TEST_ROOT = Path(TEST_DATA_DIR, "name_resolution")
+
+
+def _sort_references_lines(refs: list) -> list:
+    return sorted([ref.lines[0] for ref in refs])
+
+
+def test_name_resolution_compact(solc_binary_path) -> None:
+    solc_path = solc_binary_path("0.8.0")
+    slither = Slither(
+        Path(NAME_RESOLUTION_TEST_ROOT, "shadowing_compact.sol").as_posix(), solc=solc_path
+    )
+    contract = slither.get_contract_from_name("B")[0]
+    x = contract.get_state_variable_from_name("x")
+    assert _sort_references_lines(x.references) == [5]
+
+
+def test_name_resolution_legacy_post_0_5_0(solc_binary_path) -> None:
+    solc_path = solc_binary_path("0.5.0")
+    slither = Slither(
+        Path(NAME_RESOLUTION_TEST_ROOT, "shadowing_legacy_post_0_5_0.sol").as_posix(),
+        solc=solc_path,
+    )
+    contract = slither.get_contract_from_name("B")[0]
+    x = contract.get_state_variable_from_name("x")
+    assert _sort_references_lines(x.references) == [5]
+
+
+def test_name_resolution_legacy_pre_0_5_0(solc_binary_path) -> None:
+    solc_path = solc_binary_path("0.4.12")
+    slither = Slither(
+        Path(NAME_RESOLUTION_TEST_ROOT, "shadowing_legacy_pre_0_5_0.sol").as_posix(),
+        solc=solc_path,
+        force_legacy=True,
+    )
+    contract = slither.get_contract_from_name("B")[0]
+    function = contract.get_function_from_signature("a()")
+    x = function.get_local_variable_from_name("x")
+    assert _sort_references_lines(x.references) == [5]