From 0a5b9fdec1cd408ec9c7baa67862cafe3d945171 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Fri, 8 Sep 2023 00:28:05 -0500 Subject: [PATCH 1/3] prioritize reference id in name resolution to avoid shadowing related issues --- slither/core/declarations/contract.py | 8 +++ slither/solc_parsing/declarations/contract.py | 2 + .../solc_parsing/expressions/find_variable.py | 57 ++++++++++++------- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index 9b1488db31..46a548cee0 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -77,6 +77,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] = [] @@ -370,6 +372,12 @@ def custom_errors_as_dict(self) -> Dict[str, "CustomErrorContract"]: # 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"]: diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index 74a1cbcf0d..b7fc846d02 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/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]) diff --git a/slither/solc_parsing/expressions/find_variable.py b/slither/solc_parsing/expressions/find_variable.py index 32f5afc584..f0adef1d51 100644 --- a/slither/solc_parsing/expressions/find_variable.py +++ b/slither/solc_parsing/expressions/find_variable.py @@ -54,10 +54,26 @@ 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: + func_variables_renamed = function_parser.variables_renamed + if referenced_declaration in func_variables_renamed: + return func_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: @@ -72,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: @@ -365,20 +376,6 @@ def find_variable( if var_name in current_scope.user_defined_types: return current_scope.user_defined_types[var_name], False - # 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): @@ -392,6 +389,24 @@ def find_variable( else: assert isinstance(underlying_func, FunctionTopLevel) + 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 @@ -442,6 +457,8 @@ def find_variable( referenced_declaration, list(current_scope.contracts.values()), list(current_scope.functions), + None, + None, ) if ret: return ret, False From 25d1c6e61e8fb6b320dca2bcec36e98fbf59503e Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Fri, 8 Sep 2023 17:55:04 -0500 Subject: [PATCH 2/3] also support reference id for legacy AST --- slither/solc_parsing/expressions/expression_parsing.py | 9 +++++---- slither/solc_parsing/expressions/find_variable.py | 6 ++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/slither/solc_parsing/expressions/expression_parsing.py b/slither/solc_parsing/expressions/expression_parsing.py index a0bce044c2..c192b4efee 100644 --- a/slither/solc_parsing/expressions/expression_parsing.py +++ b/slither/solc_parsing/expressions/expression_parsing.py @@ -486,13 +486,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) @@ -501,10 +506,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) diff --git a/slither/solc_parsing/expressions/find_variable.py b/slither/solc_parsing/expressions/find_variable.py index 6255ee51ff..b7e28a8912 100644 --- a/slither/solc_parsing/expressions/find_variable.py +++ b/slither/solc_parsing/expressions/find_variable.py @@ -63,10 +63,8 @@ def _find_variable_from_ref_declaration( if referenced_declaration is None: return None # We look for variable declared with the referencedDeclaration attribute - if function_parser is not None: - func_variables_renamed = function_parser.variables_renamed - if referenced_declaration in func_variables_renamed: - return func_variables_renamed[referenced_declaration].underlying_variable + 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 From cd000a4eea72f2d5ff33dd919faab8ba59508d5f Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Wed, 13 Sep 2023 09:10:31 -0500 Subject: [PATCH 3/3] add tests --- .../name_resolution/shadowing_compact.sol | 8 ++++ .../shadowing_legacy_post_0_5_0.sol | 8 ++++ .../shadowing_legacy_pre_0_5_0.sol | 8 ++++ tests/unit/core/test_name_resolution.py | 45 +++++++++++++++++++ 4 files changed, 69 insertions(+) create mode 100644 tests/unit/core/test_data/name_resolution/shadowing_compact.sol create mode 100644 tests/unit/core/test_data/name_resolution/shadowing_legacy_post_0_5_0.sol create mode 100644 tests/unit/core/test_data/name_resolution/shadowing_legacy_pre_0_5_0.sol create mode 100644 tests/unit/core/test_name_resolution.py diff --git a/tests/unit/core/test_data/name_resolution/shadowing_compact.sol b/tests/unit/core/test_data/name_resolution/shadowing_compact.sol new file mode 100644 index 0000000000..c96130ae92 --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/tests/unit/core/test_data/name_resolution/shadowing_legacy_post_0_5_0.sol b/tests/unit/core/test_data/name_resolution/shadowing_legacy_post_0_5_0.sol new file mode 100644 index 0000000000..4a7e3a47f2 --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/tests/unit/core/test_data/name_resolution/shadowing_legacy_pre_0_5_0.sol b/tests/unit/core/test_data/name_resolution/shadowing_legacy_pre_0_5_0.sol new file mode 100644 index 0000000000..e523fd2fdd --- /dev/null +++ b/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; + } +} diff --git a/tests/unit/core/test_name_resolution.py b/tests/unit/core/test_name_resolution.py new file mode 100644 index 0000000000..8756baaba1 --- /dev/null +++ b/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]