From 405bb8d87ec04f554642e76aaa9299224a213c07 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Thu, 27 Feb 2025 14:56:47 +0100 Subject: [PATCH 1/3] 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 | 104 +++++++++++++----- slither/core/declarations/function.py | 73 +++++++++--- slither/slithir/variables/local_variable.py | 3 + .../local_alias.sol | 20 ++++ .../with_library.sol | 47 ++++++++ .../without_library.sol | 35 ++++++ tests/unit/slithir/test_storage_pointer.py | 45 ++++++++ 7 files changed, 283 insertions(+), 44 deletions(-) create mode 100644 tests/unit/slithir/test_data/variable_read_write_storage_pointer/local_alias.sol create mode 100644 tests/unit/slithir/test_data/variable_read_write_storage_pointer/with_library.sol create mode 100644 tests/unit/slithir/test_data/variable_read_write_storage_pointer/without_library.sol create mode 100644 tests/unit/slithir/test_storage_pointer.py diff --git a/slither/core/cfg/node.py b/slither/core/cfg/node.py index 83c896a42e..d71d8ed1fa 100644 --- a/slither/core/cfg/node.py +++ b/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)] diff --git a/slither/core/declarations/function.py b/slither/core/declarations/function.py index 54aec60ac6..09ccef7cd8 100644 --- a/slither/core/declarations/function.py +++ b/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 --git a/slither/slithir/variables/local_variable.py b/slither/slithir/variables/local_variable.py index 35b624a013..347c9657f8 100644 --- a/slither/slithir/variables/local_variable.py +++ b/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 --git a/tests/unit/slithir/test_data/variable_read_write_storage_pointer/local_alias.sol b/tests/unit/slithir/test_data/variable_read_write_storage_pointer/local_alias.sol new file mode 100644 index 0000000000..e14336ec69 --- /dev/null +++ b/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; + + } +} \ No newline at end of file diff --git a/tests/unit/slithir/test_data/variable_read_write_storage_pointer/with_library.sol b/tests/unit/slithir/test_data/variable_read_write_storage_pointer/with_library.sol new file mode 100644 index 0000000000..15aaf31897 --- /dev/null +++ b/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); + } + +} \ No newline at end of file diff --git a/tests/unit/slithir/test_data/variable_read_write_storage_pointer/without_library.sol b/tests/unit/slithir/test_data/variable_read_write_storage_pointer/without_library.sol new file mode 100644 index 0000000000..82367e3fe4 --- /dev/null +++ b/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); + } + +} \ No newline at end of file diff --git a/tests/unit/slithir/test_storage_pointer.py b/tests/unit/slithir/test_storage_pointer.py new file mode 100644 index 0000000000..d92a79f39c --- /dev/null +++ b/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} From 0eca6465763e95be289b88a4b77ba5c94aa72a1c Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Thu, 27 Feb 2025 15:47:41 +0100 Subject: [PATCH 2/3] Update DAO test Note: I did not fully review the changes, but I assume its due to the increased precision of the analysis --- ...tector_ReentrancyEth_0_4_25_DAO_sol__0.txt | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/e2e/detectors/snapshots/detectors__detector_ReentrancyEth_0_4_25_DAO_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_ReentrancyEth_0_4_25_DAO_sol__0.txt index 2d6c2b8204..cef2055310 100644 --- a/tests/e2e/detectors/snapshots/detectors__detector_ReentrancyEth_0_4_25_DAO_sol__0.txt +++ b/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) + From bbedc2201132ba3d4a9d1ca65acfe4e70d002092 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Thu, 27 Feb 2025 15:57:44 +0100 Subject: [PATCH 3/3] Rename vars to candidates --- slither/core/cfg/node.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/slither/core/cfg/node.py b/slither/core/cfg/node.py index d71d8ed1fa..e6e7e7dec8 100644 --- a/slither/core/cfg/node.py +++ b/slither/core/cfg/node.py @@ -1008,13 +1008,13 @@ def _update_write_using_ssa(self, ir: Operation) -> None: if isinstance(var, ReferenceVariable): var = var.points_to_origin - vars = [var] + candidates = [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 + candidates += var.refers_to - for var in vars: + for var in candidates: # Only store non-slithIR variables if var and isinstance(var, (StateIRVariable, LocalIRVariable)): if isinstance(ir, PhiCallback):