diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97b092b..c414ee7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,9 +35,10 @@ jobs: ~/.cargo/registry/cache/ ~/.cargo/git/db/ target/ - - key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} + + key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}-${{ hashFiles('**/*.rs', '**/Cargo.toml') }} restore-keys: | + ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}- ${{ runner.os }}-cargo- - name: Install dependencies diff --git a/lib/bytecode_verification/parse_json.rs b/lib/bytecode_verification/parse_json.rs index e992a60..186b6f3 100644 --- a/lib/bytecode_verification/parse_json.rs +++ b/lib/bytecode_verification/parse_json.rs @@ -324,12 +324,14 @@ impl ProjectInfo { ); // add base type if base_identifier.starts_with("t_struct") { - let struct_slots: Vec<(u64, U256, Option)> = vec![( - type_name["baseType"]["referencedDeclaration"] - .as_u64() - .unwrap(), + let struct_slots_vec: Vec<(U256, (u64, Option))> = vec![( U256::from(0), - None, + ( + type_name["baseType"]["referencedDeclaration"] + .as_u64() + .unwrap(), + None, + ), )]; // we only need the types, so we use a dummy storage vector let mut storage: Vec = vec![]; @@ -340,7 +342,7 @@ impl ProjectInfo { sources, node, type_defs, - &struct_slots, + &struct_slots_vec, types, &mut storage, ); @@ -371,15 +373,17 @@ impl ProjectInfo { .unwrap() .to_string(); if identifier.starts_with("t_struct") { - let struct_slots: Vec<(u64, U256, Option)> = vec![( - type_name - .get("referencedDeclaration") - .unwrap() - .as_u64() - .unwrap(), + let struct_slots_vec: Vec<(U256, (u64, Option))> = Vec::from([( U256::from_str("0x0").unwrap(), // this won't be used as we only have to add the types - None, - )]; + ( + type_name + .get("referencedDeclaration") + .unwrap() + .as_u64() + .unwrap(), + None, + ), + )]); let mut storage: Vec = vec![]; // this won't be used as we only have to add the types for source in sources.values() { if let Some(ast) = source.ast.clone() { @@ -388,7 +392,7 @@ impl ProjectInfo { sources, top_node, type_defs, - &struct_slots, + &struct_slots_vec, types, &mut storage, ); @@ -485,14 +489,14 @@ impl ProjectInfo { sources: &BTreeMap, node: &EAstNode, type_defs: &Types, - struct_slots: &Vec<(u64, U256, Option)>, + struct_slots: &Vec<(U256, (u64, Option))>, types: &mut HashMap, storage: &mut Vec, ) { if node.node_type == NodeType::StructDefinition && node.id.is_some() { let mut storage_var_id: Option = None; // parse all struct definitions for each struct -> slot pair. - for (struct_id, slot, name) in struct_slots { + for (slot, (struct_id, name)) in struct_slots { let struct_id = *struct_id; let node_id = node.id.unwrap() as u64; if node_id == struct_id { @@ -638,7 +642,7 @@ impl ProjectInfo { types: &mut HashMap, ) { // Tuple: (struct AST ID, slot, name of variable containing the slot) - let mut struct_slots: Vec<(u64, U256, Option)> = vec![]; + let mut struct_slots: HashMap)> = HashMap::new(); // find pairs (storage slot => struct AST ID) for source in sources.values() { if let Some(ast) = source.ast.clone() { @@ -647,6 +651,12 @@ impl ProjectInfo { } } } + + // Order struct_slots by key for deterministic results + let mut struct_slots_vec: Vec<(U256, (u64, Option))> = + struct_slots.iter().map(|(k, v)| (*k, v.clone())).collect(); + struct_slots_vec.sort_by(|a, b| a.0.cmp(&b.0)); + // parse the struct members + types for source in sources.values() { if let Some(ast) = source.ast.clone() { @@ -655,7 +665,7 @@ impl ProjectInfo { sources, node, type_defs, - &struct_slots, + &struct_slots_vec, types, storage, ); @@ -671,7 +681,7 @@ impl ProjectInfo { sources: &BTreeMap, node: &EAstNode, exported_ids: &Vec, - struct_slots: &mut Vec<(u64, U256, Option)>, + struct_slots: &mut HashMap)>, ) { if node.node_type == NodeType::ContractDefinition && node.id.is_some() @@ -745,7 +755,7 @@ impl ProjectInfo { top_node, stmt_ref["declaration"].as_u64().unwrap() ) { - struct_slots.push((struct_id, var_slot, Some(var_name))); + struct_slots.insert(var_slot, (struct_id, Some(var_name))); // if no variable declaration can be found, try to find // functions with the variable as parameter. } else if let Some((_, _, function_id, param_id)) @@ -799,9 +809,7 @@ impl ProjectInfo { top_node, var_ref_id.as_u64().unwrap() ) { - if !struct_slots.iter().any(|(_, slot, _)| slot.eq(&var_slot)) { - struct_slots.push((struct_id, var_slot, Some(var_name))); - } + struct_slots.entry(var_slot).or_insert((struct_id, Some(var_name))); } } } else if let Some(slot_value) = arg[param_id].get("value") { @@ -809,15 +817,7 @@ impl ProjectInfo { // as we have no associated variable for the slot, // we use the name of the outer function. let var_slot = U256::from_str(slot_value.as_str().unwrap()).unwrap(); - if !struct_slots.iter().any(|(_, slot, _)| slot.eq(&var_slot)) { - struct_slots.push( - ( - struct_id, - var_slot, - Some(format!("[{}]", outer_function)) - ) - ); - } + struct_slots.entry(var_slot).or_insert((struct_id, Some(outer_function))); } } } @@ -855,16 +855,18 @@ impl ProjectInfo { } else { function_name = None; } - struct_slots.push(( - struct_id, - U256::from_str( - slot_value - .as_str() - .unwrap(), - ) - .unwrap(), - function_name, - )); + let var_slot = U256::from_str( + slot_value + .as_str() + .unwrap(), + ) + .unwrap(); + struct_slots + .entry(var_slot) + .or_insert(( + struct_id, + function_name, + )); } } } diff --git a/lib/dvf/discovery.rs b/lib/dvf/discovery.rs index ca5e960..b028fb9 100644 --- a/lib/dvf/discovery.rs +++ b/lib/dvf/discovery.rs @@ -174,7 +174,15 @@ pub fn discover_storage_and_events( ); contract_state.add_forge_inspect(&fi_impl_layout, &fi_impl_ir); - storage_layout.extend(tmp_project_info.storage.clone()); + // Extend storage with implementation storage variables, ensuring unique slots + for storage_var in &tmp_project_info.storage { + if !storage_layout + .iter() + .any(|existing| existing.slot == storage_var.slot) + { + storage_layout.push(storage_var.clone()); + } + } types.extend(tmp_project_info.types.clone()); imp_project_info = Some(tmp_project_info); } diff --git a/test1.json b/test1.json new file mode 100644 index 0000000..e73753a --- /dev/null +++ b/test1.json @@ -0,0 +1,53 @@ +{ + "version": "0.9.1", + "id": "0x9c82796f5193c4db0965d63046ed83572b2e8140f2be2f9587125b37e8d8710c", + "contract_name": "ConstructorFactory", + "address": "0x5fbdb2315678afecb367f032d93f642f64180aa3", + "chain_id": 31337, + "deployment_block_num": 1, + "init_block_num": 2, + "deployment_tx": "0x9f852dc8e8b0ad7dee5533ba7854bb676f3f270a3720c7501bbd0a65b1194663", + "codehash": "0x035d6d745ec488636551786986451c4f4d368d1fd9facc2c4f11b8b3733f5aa9", + "insecure": false, + "immutables": [], + "constructor_args": [], + "critical_storage_variables": [ + { + "slot": "0x0", + "offset": 0, + "var_name": "children.length", + "var_type": "t_uint256", + "value": "0x0000000000000000000000000000000000000000000000000000000000000002", + "value_hint": "2", + "comparison_operator": "Equal" + }, + { + "slot": "0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563", + "offset": 0, + "var_name": "children[0]", + "var_type": "t_address", + "value": "0xa16e02e87b7454126e5e10d957a927a7f5b5d2be", + "comparison_operator": "Equal" + }, + { + "slot": "0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e564", + "offset": 0, + "var_name": "children[1]", + "var_type": "t_address", + "value": "0xb7a5bd0345ef1cc5e66bf61bdec17d2461fbd968", + "comparison_operator": "Equal" + } + ], + "critical_events": [], + "unvalidated_metadata": { + "author_name": "Author", + "description": "System Description", + "hardfork": [ + "paris", + "shanghai" + ], + "audit_report": "https://example.org/report.pdf", + "source_url": "https://github.com/source/code", + "security_contact": "security@example.org" + } +} \ No newline at end of file diff --git a/tests/Contracts/script/Deploy_ConstructorFactory.s.sol b/tests/Contracts/script/Deploy_ConstructorFactory.s.sol new file mode 100644 index 0000000..0c0284f --- /dev/null +++ b/tests/Contracts/script/Deploy_ConstructorFactory.s.sol @@ -0,0 +1,21 @@ +pragma solidity ^0.8.12; + +import "forge-std/Script.sol"; +import "../src/ConstructorFactory.sol"; + +contract Deploy is Script { + function run() external { + uint256 anvilDefaultKey = 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80; + vm.startBroadcast(anvilDefaultKey); + + ConstructorFactory factory = new ConstructorFactory(); + factory.createChild(100); + factory.createChild(200); + + for (uint256 i = 0; i < 5; i++) { + factory.dummy(); + } + + vm.stopBroadcast(); + } +} diff --git a/tests/Contracts/src/ConstructorFactory.sol b/tests/Contracts/src/ConstructorFactory.sol new file mode 100644 index 0000000..6571ed3 --- /dev/null +++ b/tests/Contracts/src/ConstructorFactory.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.13; + +contract Child { + address public owner; + uint256 public value; + + constructor(address _owner, uint256 _value) { + owner = _owner; + value = _value; + } + + function setValue(uint256 _value) external { + value = _value; + } +} + +contract ConstructorFactory { + address[] public children; + + constructor() { + Child child = new Child(msg.sender, 42); + children.push(address(child)); + } + + function createChild(uint256 _value) external returns (address) { + Child child = new Child(msg.sender, _value); + children.push(address(child)); + return address(child); + } + + function getChildrenCount() external view returns (uint256) { + return children.length; + } + + function dummy() external {} +} diff --git a/tests/ci_tests.sh b/tests/ci_tests.sh index bddd580..64ff676 100755 --- a/tests/ci_tests.sh +++ b/tests/ci_tests.sh @@ -24,6 +24,7 @@ pkill geth || true pkill anvil || true pkill cached_proxy || true rm -rf /tmp/uni-factory /tmp/usdc_implementation2 +cargo clean cargo build cargo clippy mkdir -p /tmp/dvfs diff --git a/tests/expected_dvfs/CrazyHiddenStruct.dvf.json b/tests/expected_dvfs/CrazyHiddenStruct.dvf.json index 9fcc21a..cf2bd21 100644 --- a/tests/expected_dvfs/CrazyHiddenStruct.dvf.json +++ b/tests/expected_dvfs/CrazyHiddenStruct.dvf.json @@ -1,6 +1,6 @@ { "version": "0.9.1", - "id": "0x71f27bd042cf53e6783d03336d86171f7d728e61f867e60071ae32818de10da2", + "id": "0x294a145622ea4fa416a4fe6395b3a307e7d692e6956c2fd0e2192831c7e776fa", "contract_name": "CrazyHiddenStruct", "address": "0x5fbdb2315678afecb367f032d93f642f64180aa3", "chain_id": 1337, @@ -484,7 +484,7 @@ { "slot": "0xa83659c989cfe332581a2ed207e0e6d23d9199b0de773442a1e23a9b8c5138f0", "offset": 0, - "var_name": "CrazyHiddenStruct.[_setOwner].AddressSlot.value", + "var_name": "CrazyHiddenStruct._setOwner.AddressSlot.value", "var_type": "t_address", "value": "0x5fbdb2315678afecb367f032d93f642f64180aa3", "comparison_operator": "Equal" diff --git a/tests/expected_dvfs/PullPayment.dvf.json b/tests/expected_dvfs/PullPayment.dvf.json index 803c02e..b978e06 100644 --- a/tests/expected_dvfs/PullPayment.dvf.json +++ b/tests/expected_dvfs/PullPayment.dvf.json @@ -7,7 +7,7 @@ "deployment_block_num": 2, "init_block_num": 3, "deployment_tx": "0x42d75b67073ebd107239df89b1f512be1972205d5bc728bcafeb3dc5675d0d15", - "codehash": "0x0d737bb1623f9f2c92d870330a5e6e657949fc96c0cd7b09d93be7565d352580", + "codehash": "0x2f5330b21d5be1ae2b7e5e526f3a309b0bb64480c1cf0d9f6be0794a5a7f6ce4", "insecure": false, "immutables": [ { diff --git a/tests/expected_dvfs/PureChild.dvf.json b/tests/expected_dvfs/PureChild.dvf.json index 88d5a01..6f3f0ee 100644 --- a/tests/expected_dvfs/PureChild.dvf.json +++ b/tests/expected_dvfs/PureChild.dvf.json @@ -7,7 +7,7 @@ "deployment_block_num": 2, "init_block_num": 3, "deployment_tx": "0xc3f03c210e501eaca1459141ce84bb36675ca2a1cc28d6d716c9794d2b9d4e88", - "codehash": "0x58f7c91b706c7b2e5694cdb86ba16504d4d8851df9522b29299fe8bb95403d57", + "codehash": "0x96e74684860511bd54dc85352acf15d678b3e88a6ab3bc7414aebbaec5571159", "insecure": false, "immutables": [], "constructor_args": [], diff --git a/tests/expected_dvfs/PureDeployer.dvf.json b/tests/expected_dvfs/PureDeployer.dvf.json index 83b64af..0b110bb 100644 --- a/tests/expected_dvfs/PureDeployer.dvf.json +++ b/tests/expected_dvfs/PureDeployer.dvf.json @@ -7,7 +7,7 @@ "deployment_block_num": 2, "init_block_num": 3, "deployment_tx": "0xc3f03c210e501eaca1459141ce84bb36675ca2a1cc28d6d716c9794d2b9d4e88", - "codehash": "0xaf3123d8bc1c73c93f3c9b0176b44bd11c0db94032b5a1326f28b7ce6967a06b", + "codehash": "0x6347979e9c068d544e8acaff418347562606d8f9ee014ac90cf06c0c7cb0ea93", "insecure": false, "immutables": [], "constructor_args": [], diff --git a/tests/test_end_to_end.rs b/tests/test_end_to_end.rs index e53d806..bfe6295 100644 --- a/tests/test_end_to_end.rs +++ b/tests/test_end_to_end.rs @@ -1507,7 +1507,7 @@ mod tests { } #[test] - fn test_pure_factory() { + fn test_e2e_pure_factory() { let port = 8553u16; let config_file = match DVFConfig::test_config_file(Some(port)) { Ok(config) => config, diff --git a/tests/with_metadata/src/PullPayment.sol b/tests/with_metadata/src/PullPayment.sol index 8aa6261..32c329c 100644 --- a/tests/with_metadata/src/PullPayment.sol +++ b/tests/with_metadata/src/PullPayment.sol @@ -105,6 +105,7 @@ abstract contract Ownable is Context { emit OwnershipTransferred(oldOwner, newOwner); } } + // OpenZeppelin Contracts (last updated v4.9.0) (utils/Address.sol) /** @@ -474,4 +475,3 @@ contract PullPayment { function dummy() external {} } - \ No newline at end of file diff --git a/tests/with_metadata/src/PureFactory.sol b/tests/with_metadata/src/PureFactory.sol index 4223daf..7e4c0d9 100644 --- a/tests/with_metadata/src/PureFactory.sol +++ b/tests/with_metadata/src/PureFactory.sol @@ -23,4 +23,3 @@ contract PureDeployer { function dummy() external {} } - \ No newline at end of file