diff --git a/slither/core/cfg/node.py b/slither/core/cfg/node.py index fc178db4a..83c896a42 100644 --- a/slither/core/cfg/node.py +++ b/slither/core/cfg/node.py @@ -11,7 +11,6 @@ SolidityFunction, ) from slither.core.expressions.expression import Expression -from slither.core.expressions import CallExpression, Identifier, AssignmentOperation from slither.core.solidity_types import ElementaryType from slither.core.source_mapping.source_mapping import SourceMapping from slither.core.variables.local_variable import LocalVariable @@ -889,21 +888,6 @@ def _find_read_write_call(self) -> None: # pylint: disable=too-many-statements # TODO: consider removing dependancy of solidity_call to internal_call self._solidity_calls.append(ir) self._internal_calls.append(ir) - if ( - isinstance(ir, SolidityCall) - and ir.function == SolidityFunction("sstore(uint256,uint256)") - and isinstance(ir.node.expression, CallExpression) - and isinstance(ir.node.expression.arguments[0], Identifier) - ): - self._vars_written.append(ir.arguments[0]) - if ( - isinstance(ir, SolidityCall) - and ir.function == SolidityFunction("sload(uint256)") - and isinstance(ir.node.expression, AssignmentOperation) - and isinstance(ir.node.expression.expression_right, CallExpression) - and isinstance(ir.node.expression.expression_right.arguments[0], Identifier) - ): - self._vars_read.append(ir.arguments[0]) if isinstance(ir, LowLevelCall): assert isinstance(ir.destination, (Variable, SolidityVariable)) self._low_level_calls.append(ir) diff --git a/slither/visitors/slithir/expression_to_slithir.py b/slither/visitors/slithir/expression_to_slithir.py index 1d68336bd..f4346f81d 100644 --- a/slither/visitors/slithir/expression_to_slithir.py +++ b/slither/visitors/slithir/expression_to_slithir.py @@ -406,6 +406,29 @@ def _post_call_expression(self, expression: CallExpression) -> None: self._result.append(var) set_val(expression, val) + elif ( + called.name in ["sload(uint256)", "sstore(uint256,uint256)"] + and len(args) > 0 + and isinstance(args[0], StateVariable) + ): + # parse_yul._parse_yul_magic_suffixes does a best effort tentative to retrieve + # the right state variable on .slot access + # + # Solidity does not allow state variable to be directly used through sstore/sload + # As you need to specify the slot number (ex you can't do " sload(some_state_variable)") + # + # So we can make the assumption that if a state variable appear on the first argument + # of sstore/sload, we can convert the call to sstore to a normal assignment / read + + if called.name == "sload(uint256)": + val = TemporaryVariable(self._node) + var = Assignment(val, args[0], ElementaryType("uint256")) + self._result.append(var) + set_val(expression, val) + else: + var = Assignment(args[0], args[1], ElementaryType("uint256")) + self._result.append(var) + set_val(expression, args[0]) else: # If tuple if expression.type_call.startswith("tuple(") and expression.type_call != "tuple()": diff --git a/tests/e2e/detectors/snapshots/detectors__detector_CouldBeConstant_0_8_0_unused_yul_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_CouldBeConstant_0_8_0_unused_yul_sol__0.txt new file mode 100644 index 000000000..e69de29bb diff --git a/tests/e2e/detectors/test_data/constable-states/0.8.0/unused_yul.sol b/tests/e2e/detectors/test_data/constable-states/0.8.0/unused_yul.sol new file mode 100644 index 000000000..6eb4ca98c --- /dev/null +++ b/tests/e2e/detectors/test_data/constable-states/0.8.0/unused_yul.sol @@ -0,0 +1,60 @@ +// adapted from https://github.com/crytic/slither/issues/2325 + +contract L1Block { + + address internal constant cDEPOSITOR_ACCOUNT = 0xDeaDDEaDDeAdDeAdDEAdDEaddeAddEAdDEAd0001; + + /// @notice Address of the special depositor account. + function DEPOSITOR_ACCOUNT() public pure returns (address addr_) { + addr_ = cDEPOSITOR_ACCOUNT; + } + + /// @notice The latest L1 block number known by the L2 system. + uint64 public number; + + /// @notice The latest L1 base fee. + uint256 public basefee; + + /// @notice The latest L1 blockhash. + bytes32 public hash; + + /// @notice The number of L2 blocks in the same epoch. + uint64 public sequenceNumber; + + /// @notice The versioned hash to authenticate the batcher by. + bytes32 public batcherHash; + + /// @notice The latest L1 blob base fee. + uint256 public blobBaseFee; + + /// @notice Updates the L1 block values for an Ecotone upgraded chain. + /// Params are packed and passed in as raw msg.data instead of ABI to reduce calldata size. + /// Params are expected to be in the following order: + /// 1. _baseFeeScalar L1 base fee scalar + /// 2. _blobBaseFeeScalar L1 blob base fee scalar + /// 3. _sequenceNumber Number of L2 blocks since epoch start. + /// 4. _timestamp L1 timestamp. + /// 5. _number L1 blocknumber. + /// 6. _basefee L1 base fee. + /// 7. _blobBaseFee L1 blob base fee. + /// 8. _hash L1 blockhash. + /// 9. _batcherHash Versioned hash to authenticate batcher by. + function _setL1BlockValuesEcotone() internal { + address depositor = DEPOSITOR_ACCOUNT(); + assembly { + // Revert if the caller is not the depositor account. + if xor(caller(), depositor) { + mstore(0x00, 0x3cc50b45) // 0x3cc50b45 is the 4-byte selector of "NotDepositor()" + revert(0x1C, 0x04) // returns the stored 4-byte selector from above + } + // sequencenum (uint64), blobBaseFeeScalar (uint32), baseFeeScalar (uint32) + sstore(sequenceNumber.slot, shr(128, calldataload(4))) + // number (uint64) and timestamp (uint64) + sstore(number.slot, shr(128, calldataload(20))) + sstore(basefee.slot, calldataload(36)) // uint256 + sstore(blobBaseFee.slot, calldataload(68)) // uint256 + sstore(hash.slot, calldataload(100)) // bytes32 + sstore(batcherHash.slot, calldataload(132)) // bytes32 + } + } +} \ No newline at end of file diff --git a/tests/e2e/detectors/test_data/constable-states/0.8.0/unused_yul.sol-0.8.0.zip b/tests/e2e/detectors/test_data/constable-states/0.8.0/unused_yul.sol-0.8.0.zip new file mode 100644 index 000000000..c3d44f0c1 Binary files /dev/null and b/tests/e2e/detectors/test_data/constable-states/0.8.0/unused_yul.sol-0.8.0.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index d2f191a4d..38ae278d3 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -510,6 +510,11 @@ def id_test(test_item: Test): "const_state_variables.sol", "0.8.0", ), + Test( + all_detectors.CouldBeConstant, + "unused_yul.sol", + "0.8.0", + ), Test( all_detectors.CouldBeImmutable, "immut_state_variables.sol", diff --git a/tests/e2e/solc_parsing/test_ast_parsing.py b/tests/e2e/solc_parsing/test_ast_parsing.py index 6ec7b6fbd..089c678f7 100644 --- a/tests/e2e/solc_parsing/test_ast_parsing.py +++ b/tests/e2e/solc_parsing/test_ast_parsing.py @@ -476,6 +476,7 @@ def make_version(minor: int, patch_min: int, patch_max: int) -> List[str]: Test("scope/inherited_function_scope.sol", ["0.8.24"]), Test("using_for_global_user_defined_operator_1.sol", ["0.8.24"]), Test("require-error.sol", ["0.8.27"]), + Test("yul-solady.sol", ["0.8.27"]), ] # create the output folder if needed try: diff --git a/tests/e2e/solc_parsing/test_data/compile/yul-solady.sol-0.8.27-compact.zip b/tests/e2e/solc_parsing/test_data/compile/yul-solady.sol-0.8.27-compact.zip new file mode 100644 index 000000000..849144170 Binary files /dev/null and b/tests/e2e/solc_parsing/test_data/compile/yul-solady.sol-0.8.27-compact.zip differ diff --git a/tests/e2e/solc_parsing/test_data/expected/yul-solady.sol-0.8.27-compact.json b/tests/e2e/solc_parsing/test_data/expected/yul-solady.sol-0.8.27-compact.json new file mode 100644 index 000000000..c3fac6ab7 --- /dev/null +++ b/tests/e2e/solc_parsing/test_data/expected/yul-solady.sol-0.8.27-compact.json @@ -0,0 +1,9 @@ +{ + "C": { + "_checkpointPushDiff(uint256,uint256,uint256,bool)": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: INLINE ASM 1\n\"];\n1->2;\n2[label=\"Node Type: NEW VARIABLE 2\n\"];\n2->3;\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: BEGIN_LOOP 4\n\"];\n4->6;\n5[label=\"Node Type: END_LOOP 5\n\"];\n5->25;\n6[label=\"Node Type: NEW VARIABLE 6\n\"];\n6->7;\n7[label=\"Node Type: EXPRESSION 7\n\"];\n7->8;\n8[label=\"Node Type: IF_LOOP 8\n\"];\n8->5[label=\"True\"];\n8->9[label=\"False\"];\n9[label=\"Node Type: IF 9\n\"];\n9->11[label=\"True\"];\n9->10[label=\"False\"];\n10[label=\"Node Type: END_IF 10\n\"];\n10->23;\n11[label=\"Node Type: IF 11\n\"];\n11->13[label=\"True\"];\n11->12[label=\"False\"];\n12[label=\"Node Type: END_IF 12\n\"];\n12->15;\n13[label=\"Node Type: EXPRESSION 13\n\"];\n13->14;\n14[label=\"Node Type: EXPRESSION 14\n\"];\n14->12;\n15[label=\"Node Type: EXPRESSION 15\n\"];\n15->16;\n16[label=\"Node Type: IF 16\n\"];\n16->18[label=\"True\"];\n16->17[label=\"False\"];\n17[label=\"Node Type: END_IF 17\n\"];\n17->20;\n18[label=\"Node Type: EXPRESSION 18\n\"];\n18->19;\n19[label=\"Node Type: BREAK 19\n\"];\n19->17;\n20[label=\"Node Type: EXPRESSION 20\n\"];\n20->21;\n21[label=\"Node Type: EXPRESSION 21\n\"];\n21->22;\n22[label=\"Node Type: BREAK 22\n\"];\n22->10;\n23[label=\"Node Type: NEW VARIABLE 23\n\"];\n23->24;\n24[label=\"Node Type: EXPRESSION 24\n\"];\n24->8;\n25[label=\"Node Type: END INLINE ASM 25\n\"];\n25->26;\n26[label=\"Node Type: RETURN 26\n\"];\n}\n" + }, + "Initializable": { + "_initializableSlot()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n", + "initializer()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: INLINE ASM 2\n\"];\n2->3;\n3[label=\"Node Type: NEW VARIABLE 3\n\"];\n3->4;\n4[label=\"Node Type: EXPRESSION 4\n\"];\n4->5;\n5[label=\"Node Type: EXPRESSION 5\n\"];\n5->6;\n6[label=\"Node Type: IF 6\n\"];\n6->8[label=\"True\"];\n6->7[label=\"False\"];\n7[label=\"Node Type: END_IF 7\n\"];\n7->13;\n8[label=\"Node Type: IF 8\n\"];\n8->10[label=\"True\"];\n8->9[label=\"False\"];\n9[label=\"Node Type: END_IF 9\n\"];\n9->12;\n10[label=\"Node Type: EXPRESSION 10\n\"];\n10->11;\n11[label=\"Node Type: EXPRESSION 11\n\"];\n11->9;\n12[label=\"Node Type: EXPRESSION 12\n\"];\n12->7;\n13[label=\"Node Type: END INLINE ASM 13\n\"];\n13->14;\n14[label=\"Node Type: _ 14\n\"];\n14->15;\n15[label=\"Node Type: INLINE ASM 15\n\"];\n15->16;\n16[label=\"Node Type: IF 16\n\"];\n16->18[label=\"True\"];\n16->17[label=\"False\"];\n17[label=\"Node Type: END_IF 17\n\"];\n17->21;\n18[label=\"Node Type: EXPRESSION 18\n\"];\n18->19;\n19[label=\"Node Type: EXPRESSION 19\n\"];\n19->20;\n20[label=\"Node Type: EXPRESSION 20\n\"];\n20->17;\n21[label=\"Node Type: END INLINE ASM 21\n\"];\n}\n" + } +} \ No newline at end of file diff --git a/tests/e2e/solc_parsing/test_data/yul-solady.sol b/tests/e2e/solc_parsing/test_data/yul-solady.sol new file mode 100644 index 000000000..b82dddcf4 --- /dev/null +++ b/tests/e2e/solc_parsing/test_data/yul-solady.sol @@ -0,0 +1,75 @@ +contract C { + // Snippet of the _checkpointPushDiff function that was making slither crashes + // https://github.com/Vectorized/solady/blob/9298d096feb87de9a8873a704ff98f6892064c65/src/tokens/ERC20Votes.sol#L339-L361 + function _checkpointPushDiff(uint256 lengthSlot, uint256 key, uint256 amount, bool isAdd) + private + returns(uint256 newValue) + { + /// @solidity memory-safe-assembly + assembly { + let lengthSlotPacked := sload(lengthSlot) + for { let n := shr(208, shl(160, lengthSlotPacked)) } 1 {} { + if iszero(n) { + if iszero(or(isAdd, iszero(amount))) { + mstore(0x00, 0x5915f686) // `ERC5805CheckpointValueUnderflow()`. + revert(0x1c, 0x04) + } + newValue := amount + if iszero(or(eq(newValue, address()), shr(160, newValue))) { + sstore(lengthSlot, or(or(key, shl(48, 1)), shl(96, newValue))) + break + } + sstore(lengthSlot, or(or(key, shl(48, 1)), shl(96, address()))) + sstore(not(lengthSlot), newValue) + break + } + let checkpointSlot := add(sub(n, 1), lengthSlot) + } + } + } +} + + +// Snippet of the Initializable contract that was making slither crashes +// https://github.com/Vectorized/solady/blob/9298d096feb87de9a8873a704ff98f6892064c65/src/utils/Initializable.sol#L7 +contract Initializable { + bytes32 private constant _INTIALIZED_EVENT_SIGNATURE = + 0xc7f505b2f371ae2175ee4913f4499e1f2633a7b5936321eed1cdaeb6115181d2; + bytes32 private constant _INITIALIZABLE_SLOT = + 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffbf601132; + + + function _initializableSlot() internal pure virtual returns (bytes32) { + return _INITIALIZABLE_SLOT; + } + + modifier initializer() virtual { + bytes32 s = _initializableSlot(); + /// @solidity memory-safe-assembly + assembly { + let i := sload(s) + // Set `initializing` to 1, `initializedVersion` to 1. + sstore(s, 3) + // If `!(initializing == 0 && initializedVersion == 0)`. + if i { + // If `!(address(this).code.length == 0 && initializedVersion == 1)`. + if iszero(lt(extcodesize(address()), eq(shr(1, i), 1))) { + mstore(0x00, 0xf92ee8a9) // `InvalidInitialization()`. + revert(0x1c, 0x04) + } + s := shl(shl(255, i), s) // Skip initializing if `initializing == 1`. + } + } + _; + /// @solidity memory-safe-assembly + assembly { + if s { + // Set `initializing` to 0, `initializedVersion` to 1. + sstore(s, 2) + // Emit the {Initialized} event. + mstore(0x20, 1) + log1(0x20, 0x20, _INTIALIZED_EVENT_SIGNATURE) + } + } + } +} \ No newline at end of file diff --git a/tests/unit/slithir/test_data/assembly_sstore_sload.sol b/tests/unit/slithir/test_data/assembly_sstore_sload.sol new file mode 100644 index 000000000..bf32d1f9d --- /dev/null +++ b/tests/unit/slithir/test_data/assembly_sstore_sload.sol @@ -0,0 +1,29 @@ +contract Test{ + + uint variable; + + function read() internal { + assembly { + let read_value := sload(variable.slot) + } + } + + function read_parameter(uint slot) internal { + assembly { + let read_value := sload(slot) + } + } + + function write() internal { + assembly { + sstore(variable.slot, 1) + } + } + + function write_parameter(uint slot) internal { + assembly { + sstore(slot, 1) + } + } + +} \ No newline at end of file diff --git a/tests/unit/slithir/test_yul_parser_assembly_slot.py b/tests/unit/slithir/test_yul_parser_assembly_slot.py index da800b55e..593b65a5d 100644 --- a/tests/unit/slithir/test_yul_parser_assembly_slot.py +++ b/tests/unit/slithir/test_yul_parser_assembly_slot.py @@ -38,3 +38,27 @@ def test_yul_parser_assembly_slot(solc_binary_path) -> None: assert isinstance(value.value, StateVariable) elif value.value.name == "bucketId": assert isinstance(value.value, LocalVariable) + + +def test_yul_parser_sstore_sload(solc_binary_path) -> None: + + solc_path = solc_binary_path("0.8.18") + slither = Slither(Path(TEST_DATA_DIR, "assembly_sstore_sload.sol").as_posix(), solc=solc_path) + + contract = slither.get_contract_from_name("Test")[0] + + read = contract.get_function_from_full_name("read()") + # Sload is converted to an assignement + assert len(read.all_solidity_calls()) == 0 + + read_parameter = contract.get_function_from_full_name("read_parameter(uint256)") + # Sload is kept because the slot is dynamic + assert len(read_parameter.all_solidity_calls()) == 1 + + write = contract.get_function_from_full_name("write()") + # Sstore is converted to an assignement + assert len(write.all_solidity_calls()) == 0 + + write_parameter = contract.get_function_from_full_name("write_parameter(uint256)") + # Sstore is kept because the slot is dynamic + assert len(write_parameter.all_solidity_calls()) == 1