Merge pull request #1575 from crytic/dev-private-variable

Improve lookup for state variables

slither/core/declarations/contract.py

@@ -70,6 +70,8 @@ def __init__(self, compilation_unit: "SlitherCompilationUnit", scope: "FileScope
         self._enums: Dict[str, "EnumContract"] = {}
         self._structures: Dict[str, "StructureContract"] = {}
         self._events: Dict[str, "Event"] = {}
+        # map accessible variable from name -> variable
+        # do not contain private variables inherited from contract
         self._variables: Dict[str, "StateVariable"] = {}
         self._variables_ordered: List["StateVariable"] = []
         self._modifiers: Dict[str, "Modifier"] = {}
@@ -330,7 +332,9 @@ def custom_errors_as_dict(self) -> Dict[str, "CustomErrorContract"]:
     @property
     def variables(self) -> List["StateVariable"]:
         """
-        list(StateVariable): List of the state variables. Alias to self.state_variables
+        Returns all the accessible variables (do not include private variable from inherited contract)
+
+        list(StateVariable): List of the state variables. Alias to self.state_variables.
         """
         return list(self.state_variables)
 
@@ -341,6 +345,9 @@ def variables_as_dict(self) -> Dict[str, "StateVariable"]:
     @property
     def state_variables(self) -> List["StateVariable"]:
         """
+        Returns all the accessible variables (do not include private variable from inherited contract).
+        Use state_variables_ordered for all the variables following the storage order
+
         list(StateVariable): List of the state variables.
         """
         return list(self._variables.values())

slither/slithir/convert.py

@@ -1577,15 +1577,13 @@ def convert_type_of_high_and_internal_level_call(
     else:
         assert isinstance(ir, HighLevelCall)
         assert contract
-
         candidates = [
             f
             for f in contract.functions
             if f.name == ir.function_name
             and not f.is_shadowed
             and len(f.parameters) == len(ir.arguments)
         ]
-
         if len(candidates) == 1:
             func = candidates[0]
         if func is None:

slither/solc_parsing/declarations/contract.py

@@ -326,7 +326,13 @@ def parse_custom_errors(self):
 
     def parse_state_variables(self):
         for father in self._contract.inheritance_reverse:
-            self._contract.variables_as_dict.update(father.variables_as_dict)
+            self._contract.variables_as_dict.update(
+                {
+                    name: v
+                    for name, v in father.variables_as_dict.items()
+                    if v.visibility != "private"
+                }
+            )
             self._contract.add_variables_ordered(
                 [
                     var
@@ -391,11 +397,11 @@ def parse_functions(self):
     ###################################################################################
     ###################################################################################
 
-    def log_incorrect_parsing(self, error):
+    def log_incorrect_parsing(self, error: str) -> None:
         if self._contract.compilation_unit.core.disallow_partial:
             raise ParsingError(error)
         LOGGER.error(error)
-        self._contract.is_incorrectly_parsed = True
+        self._contract.is_incorrectly_constructed = True
 
     def analyze_content_modifiers(self):
         try:

slither/solc_parsing/slither_compilation_unit_solc.py

@@ -743,7 +743,7 @@ def _convert_to_slithir(self):
                     # And the interface is redefined due to contract's name reuse
                     # But the available version misses some functions
                     self._underlying_contract_to_parser[contract].log_incorrect_parsing(
-                        f"Impossible to generate IR for {contract.name}.{func.name}:\n {e}"
+                        f"Impossible to generate IR for {contract.name}.{func.name} ({func.source_mapping}):\n {e}"
                     )
 
             contract.convert_expression_to_slithir_ssa()

tests/lookup/private_variable.sol

@@ -0,0 +1,13 @@
+contract A{
+	uint private v = 10;
+}
+
+contract B{
+	uint v = 20;
+}
+
+contract C is B, A{
+	function f() public view returns(uint) {
+		return v;
+	}
+}

tests/test_features.py

@@ -5,6 +5,7 @@
 from solc_select import solc_select
 
 from slither import Slither
+from slither.core.variables.state_variable import StateVariable
 from slither.detectors import all_detectors
 from slither.detectors.abstract_detector import AbstractDetector
 from slither.slithir.operations import LibraryCall, InternalCall
@@ -139,3 +140,13 @@ def test_using_for_in_library() -> None:
         if isinstance(ir, LibraryCall) and ir.destination == "B" and ir.function_name == "b":
             return
     assert False
+
+
+def test_private_variable() -> None:
+    solc_select.switch_global_version("0.8.15", always_install=True)
+    slither = Slither("./tests/lookup/private_variable.sol")
+    contract_c = slither.get_contract_from_name("C")[0]
+    f = contract_c.functions[0]
+    var_read = f.variables_read[0]
+    assert isinstance(var_read, StateVariable)
+    assert str(var_read.contract) == "B"