Improve support for storage pointer

- Add support for storage pointer info through phi (storage parameter)
- Update read/write analysis to be more robust with storage parameter

slither/core/cfg/node.py

@@ -946,42 +946,90 @@ def _convert_ssa(v: Variable) -> Optional[Union[StateVariable, LocalVariable]]:
         non_ssa_var = function.get_local_variable_from_name(v.name)
         return non_ssa_var
 
-    def update_read_write_using_ssa(self) -> None:
-        if not self.expression:
-            return
-        for ir in self.irs_ssa:
-            if isinstance(ir, PhiCallback):
-                continue
-            if not isinstance(ir, (Phi, Index, Member)):
-                self._ssa_vars_read += [
-                    v for v in ir.read if isinstance(v, (StateIRVariable, LocalIRVariable))
-                ]
-                for var in ir.read:
-                    if isinstance(var, ReferenceVariable):
-                        origin = var.points_to_origin
-                        if isinstance(origin, (StateIRVariable, LocalIRVariable)):
-                            self._ssa_vars_read.append(origin)
-
-            elif isinstance(ir, (Member, Index)):
-                variable_right: RVALUE = ir.variable_right
-                if isinstance(variable_right, (StateIRVariable, LocalIRVariable)):
-                    self._ssa_vars_read.append(variable_right)
-                if isinstance(variable_right, ReferenceVariable):
-                    origin = variable_right.points_to_origin
+    def _update_read_using_ssa(self, ir: Operation) -> None:
+        """
+        Update self._ssa_vars_read
+        This look for all operations that read a IRvariable
+        It uses the result of the storage pointer
+        - For "normal" operation, the read are mostly everything in ir.read
+        - For "index", the read is the left part (the right part being a reference variable)
+        - For Phi, nothing is considered read
+
+        """
+
+        # For variable read, phi and index have special treatments
+        # Phi don't lead to values read
+        # Index leads to read the variable right (the left variable is a ref variable, not the actual object)
+        # Not that Member is a normal operation here, given we filter out constant by checking for the IRvaraible
+        if not isinstance(ir, (Phi, Index)):
+            self._ssa_vars_read += [
+                v for v in ir.read if isinstance(v, (StateIRVariable, LocalIRVariable))
+            ]
+            for var in ir.read:
+                if isinstance(var, ReferenceVariable):
+                    origin = var.points_to_origin
                     if isinstance(origin, (StateIRVariable, LocalIRVariable)):
                         self._ssa_vars_read.append(origin)
 
-            if isinstance(ir, OperationWithLValue):
-                if isinstance(ir, (Index, Member, Length)):
-                    continue  # Don't consider Member and Index operations -> ReferenceVariable
-                var = ir.lvalue
-                if isinstance(var, ReferenceVariable):
-                    var = var.points_to_origin
+                # If we read from a storage variable (outside of phi operator)
+                if isinstance(var, LocalIRVariable) and var.is_storage:
+                    for refer_to in var.refers_to:
+                        # the following should always be true
+                        if isinstance(refer_to, (StateIRVariable, LocalIRVariable)):
+                            self._ssa_vars_read.append(refer_to)
+
+        elif isinstance(ir, Index):
+            variable_right: RVALUE = ir.variable_right
+            if isinstance(variable_right, (StateIRVariable, LocalIRVariable)):
+                self._ssa_vars_read.append(variable_right)
+
+            if isinstance(variable_right, ReferenceVariable):
+                origin = variable_right.points_to_origin
+                if isinstance(origin, (StateIRVariable, LocalIRVariable)):
+                    self._ssa_vars_read.append(origin)
+
+    def _update_write_using_ssa(self, ir: Operation) -> None:
+        """
+        Update self._ssa_vars_written
+        This look for all operations that write a IRvariable
+        It uses the result of the storage pointer
+
+        Index/member/Length are not considering writing to anything
+        For index/member it is implictely handled when their associated RefernceVarible are written
+
+        """
+
+        if isinstance(ir, OperationWithLValue) and not isinstance(ir, Phi):
+            if isinstance(ir, (Index, Member, Length)):
+                return  # Don't consider Member and Index operations -> ReferenceVariable
+
+            var = ir.lvalue
+
+            if isinstance(var, ReferenceVariable):
+                var = var.points_to_origin
+
+            vars = [var]
+
+            # If we write to a storage pointer, add everything it points to as target
+            if isinstance(var, LocalIRVariable) and var.is_storage:
+                vars += var.refers_to
+
+            for var in vars:
                 # Only store non-slithIR variables
                 if var and isinstance(var, (StateIRVariable, LocalIRVariable)):
                     if isinstance(ir, PhiCallback):
                         continue
                     self._ssa_vars_written.append(var)
+
+    def update_read_write_using_ssa(self) -> None:
+
+        for ir in self.irs_ssa:
+            if isinstance(ir, PhiCallback):
+                continue
+
+            self._update_read_using_ssa(ir)
+            self._update_write_using_ssa(ir)
+
         self._ssa_vars_read = list(set(self._ssa_vars_read))
         self._ssa_state_vars_read = [v for v in self._ssa_vars_read if isinstance(v, StateVariable)]
         self._ssa_local_vars_read = [v for v in self._ssa_vars_read if isinstance(v, LocalVariable)]

slither/core/declarations/function.py

@@ -1793,30 +1793,52 @@ def _unchange_phi(ir: "Operation") -> bool:
             return True
         return ir.rvalues[0] == ir.lvalue
 
+    def _fix_phi_entry(
+        self,
+        node: "Node",
+        last_state_variables_instances: Dict[str, List["StateVariable"]],
+        initial_state_variables_instances: Dict[str, "StateVariable"],
+    ) -> None:
+        from slither.slithir.variables import Constant, StateIRVariable, LocalIRVariable
+
+        for ir in node.irs_ssa:
+            if isinstance(ir.lvalue, StateIRVariable):
+                additional = [initial_state_variables_instances[ir.lvalue.canonical_name]]
+                additional += last_state_variables_instances[ir.lvalue.canonical_name]
+                ir.rvalues = list(set(additional + ir.rvalues))
+            # function parameter that are storage pointer
+            else:
+                # find index of the parameter
+                idx = self.parameters.index(ir.lvalue.non_ssa_version)
+                # find non ssa version of that index
+                additional = [n.ir.arguments[idx] for n in self.reachable_from_nodes]
+                additional = unroll(additional)
+                additional = [a for a in additional if not isinstance(a, Constant)]
+                ir.rvalues = list(set(additional + ir.rvalues))
+
+                if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage:
+                    # Update the refers_to to point to the phi rvalues
+                    # This basically means that the local variable is a storage that point to any
+                    # state variable that the storage pointer alias analysis found
+                    ir.lvalue.refers_to = [
+                        rvalue for rvalue in ir.rvalues if isinstance(rvalue, StateIRVariable)
+                    ]
+
     def fix_phi(
         self,
         last_state_variables_instances: Dict[str, List["StateVariable"]],
         initial_state_variables_instances: Dict[str, "StateVariable"],
     ) -> None:
-        from slither.slithir.operations import InternalCall, PhiCallback
-        from slither.slithir.variables import Constant, StateIRVariable
+        from slither.slithir.operations import InternalCall, PhiCallback, Phi
+        from slither.slithir.variables import StateIRVariable, LocalIRVariable
 
         for node in self.nodes:
+            if node == self.entry_point:
+                self._fix_phi_entry(
+                    node, last_state_variables_instances, initial_state_variables_instances
+                )
             for ir in node.irs_ssa:
-                if node == self.entry_point:
-                    if isinstance(ir.lvalue, StateIRVariable):
-                        additional = [initial_state_variables_instances[ir.lvalue.canonical_name]]
-                        additional += last_state_variables_instances[ir.lvalue.canonical_name]
-                        ir.rvalues = list(set(additional + ir.rvalues))
-                    # function parameter
-                    else:
-                        # find index of the parameter
-                        idx = self.parameters.index(ir.lvalue.non_ssa_version)
-                        # find non ssa version of that index
-                        additional = [n.ir.arguments[idx] for n in self.reachable_from_nodes]
-                        additional = unroll(additional)
-                        additional = [a for a in additional if not isinstance(a, Constant)]
-                        ir.rvalues = list(set(additional + ir.rvalues))
+
                 if isinstance(ir, PhiCallback):
                     callee_ir = ir.callee_ir
                     if isinstance(callee_ir, InternalCall):
@@ -1829,6 +1851,25 @@ def fix_phi(
                         additional = last_state_variables_instances[ir.lvalue.canonical_name]
                         ir.rvalues = list(set(additional + ir.rvalues))
 
+                # Propage storage ref information if it does not exist
+                # This can happen if the refers_to variable was discovered through the phi operator on function parameter
+                # aka you have storage pointer as function parameter
+                # instead of having a storage pointer for which the aliases belong to the function body
+                if (
+                    isinstance(ir, Phi)
+                    and isinstance(ir.lvalue, LocalIRVariable)
+                    and ir.lvalue.is_storage
+                    and not ir.lvalue.refers_to
+                ):
+                    refers_to = []
+                    for candidate in ir.rvalues:
+                        if isinstance(candidate, StateIRVariable):
+                            refers_to.append(candidate)
+                        if isinstance(candidate, LocalIRVariable) and candidate.is_storage:
+                            refers_to += candidate.refers_to
+
+                    ir.lvalue.refers_to = refers_to
+
             node.irs_ssa = [ir for ir in node.irs_ssa if not self._unchange_phi(ir)]
 
     def generate_slithir_and_analyze(self) -> None:

slither/slithir/variables/local_variable.py

@@ -50,6 +50,9 @@ def index(self, idx: int) -> None:
 
     @property
     def refers_to(self):
+        """
+        Return the alias for local variable that are storage pointers
+        """
         if self.is_storage:
             return self._refers_to
         return set()

tests/unit/slithir/test_data/variable_read_write_storage_pointer/local_alias.sol

@@ -0,0 +1,20 @@
+contract Test{
+
+    struct S{
+        uint a;
+    }
+
+    S s0;
+    S s1;
+
+    function test() public{
+        S storage s_local = s0;
+
+        if(true){
+            s_local = s1;
+        }
+
+        s_local.a = 10;
+
+    }
+}

tests/unit/slithir/test_data/variable_read_write_storage_pointer/with_library.sol

@@ -0,0 +1,47 @@
+// Simplified from https://github.com/crytic/slither/issues/2598 
+
+pragma solidity ^0.8.0;
+
+library Roles {
+    struct Role {
+        mapping (address => bool) bearer;
+    }
+
+    /**
+     * @dev Give an account access to this role.
+     */
+    function add(Role storage role, address account) internal {
+        require(!has(role, account), "Roles: account already has role");
+        role.bearer[account] = true;
+    }
+
+    function has(Role storage role, address account) internal view returns (bool) {
+        require(account != address(0), "Roles: account is the zero address");
+        return role.bearer[account];
+    }
+
+}
+
+contract Context {
+
+    function _msgSender() internal view returns (address payable) {
+        return payable(msg.sender);
+    }
+
+}
+
+
+contract MinterRole is Context {
+    using Roles for Roles.Role;
+
+    Roles.Role private _minters;
+
+    function addMinter(address account) public {
+        _addMinter(account);
+    }
+
+    function _addMinter(address account) internal {
+        _minters.add(account);
+    }
+
+}   

tests/unit/slithir/test_data/variable_read_write_storage_pointer/without_library.sol

@@ -0,0 +1,35 @@
+// Simplified from https://github.com/crytic/slither/issues/2598 
+
+pragma solidity ^0.8.0;
+
+contract MinterRole {
+
+    struct Role {
+        mapping (address => bool) bearer;
+    }
+
+
+    /**
+     * @dev Give an account access to this role.
+     */
+    function add(Role storage role, address account) internal {
+        require(!has(role, account), "Roles: account already has role");
+        role.bearer[account] = true;
+    }
+
+    function has(Role storage role, address account) internal view returns (bool) {
+        require(account != address(0), "Roles: account is the zero address");
+        return role.bearer[account];
+    }
+
+    Role private _minters;
+
+    function addMinter(address account) public {
+        _addMinter(account);
+    }
+
+    function _addMinter(address account) internal {
+        add(_minters, account);
+    }
+
+}   

tests/unit/slithir/test_storage_pointer.py

@@ -0,0 +1,45 @@
+from pathlib import Path
+from slither import Slither
+
+
+TEST_DATA_DIR = Path(__file__).resolve().parent / "test_data"
+
+
+def test_local_alias(solc_binary_path) -> None:
+
+    solc_path = solc_binary_path("0.8.18")
+    slither = Slither(
+        Path(TEST_DATA_DIR, "variable_read_write_storage_pointer/local_alias.sol").as_posix(),
+        solc=solc_path,
+    )
+
+    contract = slither.get_contract_from_name("Test")[0]
+
+    test = contract.get_function_from_full_name("test()")
+
+    s0 = contract.get_state_variable_from_name("s0")
+    s1 = contract.get_state_variable_from_name("s1")
+
+    assert set(test.state_variables_written) == {s0, s1}
+
+
+def test_parameter_no_library(solc_binary_path) -> None:
+
+    solc_path = solc_binary_path("0.8.18")
+    slither = Slither(
+        Path(TEST_DATA_DIR, "variable_read_write_storage_pointer/without_library.sol").as_posix(),
+        solc=solc_path,
+    )
+
+    contract = slither.get_contract_from_name("MinterRole")[0]
+
+    print(contract.available_functions_as_dict())
+    add = contract.get_function_from_full_name("add(MinterRole.Role,address)")
+    has = contract.get_function_from_full_name("has(MinterRole.Role,address)")
+
+    minter = contract.get_state_variable_from_name("_minters")
+
+    assert set(add.state_variables_written) == {minter}
+    assert set(add.state_variables_read) == {minter}
+    assert set(has.state_variables_written) == set()
+    assert set(has.state_variables_read) == {minter}