Only discover mappings under storage nodes (#78)

Our mapping discovery used to happen at every single potential mapping
structure in the tree, but this meant that we would sometimes discover
manual structures that look the same as being access to mapping storage
slots even when they were not. We are now more restrictive.

Cargo.toml

@@ -2,7 +2,7 @@
 [package]
 # Basic metadata for the package.
 name = "storage-layout-analyzer"
-version = "0.1.0"
+version = "0.5.0"
 homepage = "https://github.com/smlxlio/storage-layout-analyzer"
 repository = "https://github.com/smlxlio/storage-layout-analyzer"
 

asset/BatchEnsLookups.sol

@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.0;
+
+contract BatchEnsLookups {
+    function resolveAddresses() public pure  {
+        keccak256(
+            abi.encodePacked(
+                sha3HexAddress()
+            )
+        );
+    }
+
+    function sha3HexAddress() private pure returns (bytes32 ret) {
+        assembly {
+            ret := keccak256(0, 40)
+        }
+    }
+}

asset/MerkleTree.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.0;
+
+contract MerkleTree {
+    function purchaseMerkleAmount() external {
+        bytes32[] memory proof = new bytes32[](1);
+        bytes32 computedHash = bytes32(0);
+        for (uint256 i = 0; i < proof.length; i++) {
+            computedHash = _efficientHash(proof[i], computedHash);
+        }
+    }
+
+    function _efficientHash(bytes32 a, bytes32 b) private pure returns (bytes32 value) {
+        /// @solidity memory-safe-assembly
+        assembly {
+            mstore(0x00, a)
+            mstore(0x20, b)
+            value := keccak256(0x00, 0x40)
+        }
+    }
+}

src/inference/abi.rs

@@ -20,8 +20,8 @@ use crate::utility::U256Wrapper;
 /// Solidity supports a `fixed` and `ufixed` type in the ABI, but the language
 /// support for them is lacking. For that reason we do not include them here for
 /// now.
-#[derive(Clone, Debug, Derivative, Deserialize, Eq, Hash, Serialize)]
-#[derivative(PartialEq)]
+#[derive(Clone, Debug, Derivative, Deserialize, Eq, Serialize)]
+#[derivative(PartialEq, Hash)]
 #[serde(rename_all = "snake_case")]
 pub enum AbiType {
     /// An unknown type, useful for the container types where we know something
@@ -98,9 +98,9 @@ pub enum AbiType {
     /// Conflicted types compare equal regardless of the `conflicts` and
     /// `reasons` that they contain.
     ConflictedType {
-        #[derivative(PartialEq = "ignore")]
+        #[derivative(Hash = "ignore", PartialEq = "ignore")]
         conflicts: Vec<String>,
-        #[derivative(PartialEq = "ignore")]
+        #[derivative(Hash = "ignore", PartialEq = "ignore")]
         reasons:   Vec<String>,
     },
 }

src/inference/lift/mapping_access.rs

@@ -17,6 +17,10 @@ use crate::{
 ///
 /// mapping_ix<slot_ix>[key]
 /// ```
+///
+/// We only perform this resolution underneath [`RSVD::StorageWrite`],
+/// [`RSVD::SLoad`] and [`RSVD::UnwrittenStorageValue`] to ensure that we do not
+/// inadvertently capture non-mapping access patterns as mappings.
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub struct MappingAccess;
 
@@ -34,6 +38,23 @@ impl Lift for MappingAccess {
         value: RuntimeBoxedVal,
         _state: &InferenceState,
     ) -> crate::error::unification::Result<RuntimeBoxedVal> {
+        fn guard_mapping_accesses(data: &RSVD) -> Option<RSVD> {
+            match data {
+                RSVD::StorageWrite { key, value } => Some(RSVD::StorageWrite {
+                    key:   key.clone().transform_data(insert_mapping_accesses),
+                    value: value.clone().transform_data(insert_mapping_accesses),
+                }),
+                RSVD::SLoad { key, value } => Some(RSVD::SLoad {
+                    key:   key.clone().transform_data(insert_mapping_accesses),
+                    value: value.clone().transform_data(insert_mapping_accesses),
+                }),
+                RSVD::UnwrittenStorageValue { key } => Some(RSVD::UnwrittenStorageValue {
+                    key: key.clone().transform_data(insert_mapping_accesses),
+                }),
+                _ => None,
+            }
+        }
+
         fn insert_mapping_accesses(data: &RSVD) -> Option<RSVD> {
             let RSVD::Sha3 { data } = data else {
                 return None;
@@ -52,7 +73,7 @@ impl Lift for MappingAccess {
             })
         }
 
-        Ok(value.transform_data(insert_mapping_accesses))
+        Ok(value.transform_data(guard_mapping_accesses))
     }
 }
 
@@ -79,19 +100,34 @@ mod test {
             None,
         );
         let hash = RSV::new(3, RSVD::Sha3 { data: concat }, Provenance::Synthetic, None);
+        let slot_key = RSV::new_value(4, Provenance::Synthetic);
+        let s_load = RSV::new_synthetic(
+            5,
+            RSVD::SLoad {
+                key:   slot_key.clone(),
+                value: hash.clone(),
+            },
+        );
 
         let state = InferenceState::empty();
-        let result = MappingAccess.run(hash, &state)?;
+        let result = MappingAccess.run(s_load, &state)?;
 
         match result.data() {
-            RSVD::MappingIndex {
-                key,
-                slot,
-                projection,
-            } => {
-                assert!(projection.is_none());
-                assert_eq!(key, &input_key);
-                assert_eq!(slot, &input_slot);
+            RSVD::SLoad { key, value } => {
+                assert_eq!(key, &slot_key);
+
+                match value.data() {
+                    RSVD::MappingIndex {
+                        key,
+                        slot,
+                        projection,
+                    } => {
+                        assert!(projection.is_none());
+                        assert_eq!(key, &input_key);
+                        assert_eq!(slot, &input_slot);
+                    }
+                    _ => panic!("Invalid payload"),
+                }
             }
             _ => panic!("Invalid payload"),
         }
@@ -113,41 +149,56 @@ mod test {
         );
         let inner_hash = RSV::new(3, RSVD::Sha3 { data: concat }, Provenance::Synthetic, None);
         let outer_concat = RSV::new(
-            2,
+            4,
             RSVD::Concat {
                 values: vec![input_key.clone(), inner_hash],
             },
             Provenance::Synthetic,
             None,
         );
         let outer_hash = RSV::new(
-            3,
+            5,
             RSVD::Sha3 { data: outer_concat },
             Provenance::Synthetic,
             None,
         );
+        let slot_key = RSV::new_value(6, Provenance::Synthetic);
+        let s_store = RSV::new_synthetic(
+            7,
+            RSVD::StorageWrite {
+                key:   slot_key.clone(),
+                value: outer_hash.clone(),
+            },
+        );
 
         let state = InferenceState::empty();
-        let result = MappingAccess.run(outer_hash, &state)?;
+        let result = MappingAccess.run(s_store, &state)?;
 
         match result.data() {
-            RSVD::MappingIndex {
-                key,
-                slot,
-                projection,
-            } => {
-                assert!(projection.is_none());
-                assert_eq!(key, &input_key);
-
-                match slot.data() {
+            RSVD::StorageWrite { key, value } => {
+                assert_eq!(key, &slot_key);
+
+                match value.data() {
                     RSVD::MappingIndex {
                         key,
                         slot,
                         projection,
                     } => {
                         assert!(projection.is_none());
                         assert_eq!(key, &input_key);
-                        assert_eq!(slot, &input_slot);
+
+                        match slot.data() {
+                            RSVD::MappingIndex {
+                                key,
+                                slot,
+                                projection,
+                            } => {
+                                assert!(projection.is_none());
+                                assert_eq!(key, &input_key);
+                                assert_eq!(slot, &input_slot);
+                            }
+                            _ => panic!("Invalid payload"),
+                        }
                     }
                     _ => panic!("Invalid payload"),
                 }

src/opcode/util.rs

@@ -26,7 +26,7 @@ use crate::{
 /// - When the jump target is not a valid [`JumpDest`] instruction.
 ///
 /// It is assumed that all errors returned by this function are instances of
-/// [`VMError`].
+/// [`crate::error::execution::Error`].
 #[allow(clippy::boxed_local)] // We always pass around boxed values during execution
 pub fn validate_jump_destination(counter: &RuntimeBoxedVal, vm: &mut VM) -> execution::Result<u32> {
     let instruction_pointer = vm.instruction_pointer()?;

tests/batch_ens_lookups_no_storage_opcodes.rs

@@ -0,0 +1,24 @@
+//! This module tests the library's analysis capabilities on the local
+//! `BatchEnsLookups` contract`.
+#![cfg(test)]
+
+use storage_layout_analyzer::watchdog::LazyWatchdog;
+
+mod common;
+
+/// Tests the analyser on the bytecode of the BatchEnsLookups contract
+/// found [locally](../asset/BatchEnsLookups.sol).
+#[test]
+fn correctly_generates_a_layout() -> anyhow::Result<()> {
+    // Create the analyzer
+    let bytecode = "0x6080604052348015600f57600080fd5b506004361060285760003560e01c8063b709959614602d575b600080fd5b60336035565b005b6028600020604051602001604b91815260200190565b60408051601f198184030190525256fea2646970667358221220645e42249fb219b90bda62dd9e9539000ea399d67aa68ba2a3521720a942364964736f6c634300080f0033";
+    let analyzer = common::new_analyzer_from_bytecode(bytecode, LazyWatchdog.in_rc())?;
+
+    // Get the final storage layout for the input contract
+    let layout = analyzer.analyze()?;
+
+    // We should see no slots as this contract never uses storage opcodes.
+    assert!(layout.slots().is_empty());
+
+    Ok(())
+}

tests/merkle_tree_no_storage_opcodes.rs

@@ -0,0 +1,24 @@
+//! This module tests the library's analysis capabilities on the local
+//! `MerkleTree` contract`.
+#![cfg(test)]
+
+use storage_layout_analyzer::watchdog::LazyWatchdog;
+
+mod common;
+
+/// Tests the analyser on the bytecode of the MerkleTree contract
+/// found [locally](../asset/MerkleTree.sol).
+#[test]
+fn correctly_generates_a_layout() -> anyhow::Result<()> {
+    // Create the analyzer
+    let bytecode = "0x6080604052348015600f57600080fd5b506004361060285760003560e01c806388572a5c14602d575b600080fd5b60336035565b005b604080516001808252818301909252600091602080830190803683370190505090506000805b8251811015609d57608c838281518110607457607460a2565b60200260200101518360009182526020526040902090565b91508060968160b8565b915050605b565b505050565b634e487b7160e01b600052603260045260246000fd5b60006001820160d757634e487b7160e01b600052601160045260246000fd5b506001019056fea26469706673582212204208c6f43bc343f0a115564be7291cf69f0cd24036b2d9d1701db9b25bf6cb0264736f6c634300080f0033";
+    let analyzer = common::new_analyzer_from_bytecode(bytecode, LazyWatchdog.in_rc())?;
+
+    // Get the final storage layout for the input contract
+    let layout = analyzer.analyze()?;
+
+    // We should see no slots as this contract never uses storage opcodes.
+    assert!(layout.slots().is_empty());
+
+    Ok(())
+}