Improve the support for sstore/sload with simple slot access

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)

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()":

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
+        }
+    }
+}

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",

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:

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"
+  }
+}

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)
+            }
+        }
+    }
+}

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)
+        }
+    }
+
+}

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