Merge pull request #2677 from crytic/dev-improve-storage-pointer

Improve support for storage pointer analysis

Author: smonicas

Diff for: 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
+
+            candidates = [var]
+
+            # If we write to a storage pointer, add everything it points to as target
+            if isinstance(var, LocalIRVariable) and var.is_storage:
+                candidates += var.refers_to
+
+            for var in candidates:
                 # 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)]

Diff for: 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:

Diff for: 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()

Diff for: tests/e2e/detectors/snapshots/detectors__detector_ReentrancyEth_0_4_25_DAO_sol__0.txt

@@ -1,16 +1,3 @@
-Reentrancy in TokenCreation.refund() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#318-332):
-	External calls:
-	- extraBalance.balance >= extraBalance.accumulatedInput() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#321)
-	- extraBalance.payOut(address(this),extraBalance.accumulatedInput()) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#322)
-	- msg.sender.call.value(weiGiven[msg.sender])() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#325)
-	External calls sending eth:
-	- msg.sender.call.value(weiGiven[msg.sender])() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#325)
-	State variables written after the call(s):
-	- weiGiven[msg.sender] = 0 (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#329)
-	TokenCreationInterface.weiGiven (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#251) can be used in cross function reentrancies:
-	- TokenCreation.createTokenProxy(address) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#299-316)
-	- TokenCreation.refund() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#318-332)
-
 Reentrancy in DAO.executeProposal(uint256,bytes) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#853-937):
 	External calls:
 	- ! isRecipientAllowed(p.recipient) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#881)
@@ -34,6 +21,7 @@ Reentrancy in DAO.executeProposal(uint256,bytes) (tests/e2e/detectors/test_data/
 	- DAO.splitDAO(uint256,address) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#947-1020)
 	- DAO.vote(uint256,bool) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#820-850)
 	- closeProposal(_proposalID) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#933)
+		- p = proposals[_proposalID] (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#941)
 		- p.open = false (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#944)
 	DAOInterface.proposals (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#394) can be used in cross function reentrancies:
 	- DAO.DAO(address,DAO_Creator,uint256,uint256,uint256,address) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#702-726)
@@ -70,3 +58,16 @@ Reentrancy in DAO.executeProposal(uint256,bytes) (tests/e2e/detectors/test_data/
 	- DAO.retrieveDAOReward(bool) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#1037-1057)
 	- DAOInterface.totalRewardToken (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#412)
 
+Reentrancy in TokenCreation.refund() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#318-332):
+	External calls:
+	- extraBalance.balance >= extraBalance.accumulatedInput() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#321)
+	- extraBalance.payOut(address(this),extraBalance.accumulatedInput()) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#322)
+	- msg.sender.call.value(weiGiven[msg.sender])() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#325)
+	External calls sending eth:
+	- msg.sender.call.value(weiGiven[msg.sender])() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#325)
+	State variables written after the call(s):
+	- weiGiven[msg.sender] = 0 (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#329)
+	TokenCreationInterface.weiGiven (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#251) can be used in cross function reentrancies:
+	- TokenCreation.createTokenProxy(address) (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#299-316)
+	- TokenCreation.refund() (tests/e2e/detectors/test_data/reentrancy-eth/0.4.25/DAO.sol#318-332)
+

Diff for: 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;
+
+    }
+}

Diff for: 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);
+    }
+    
+}   

Diff for: 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);
+    }
+
+}