Skip to content

Commit 329c960

Browse files
stiefnStefan EffenbergerStefan Effenberger
authored
Fix double slots (#64)
* changed vector to map * added another uniqueness check * fmt + clippy * fixed ordering * fixed test case file * fmt * added clean to ci tests * fixed ci cache keys * merge main * updated test dvfs and changed test name * updated another test dvf --------- Co-authored-by: Stefan Effenberger <stefan.effenberger@chainsecurity.com> Co-authored-by: Stefan Effenberger <stiefn@fedora.fritz.box>
1 parent 089fd71 commit 329c960

File tree

14 files changed

+176
-54
lines changed

14 files changed

+176
-54
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,10 @@ jobs:
3535
~/.cargo/registry/cache/
3636
~/.cargo/git/db/
3737
target/
38-
39-
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
38+
39+
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}-${{ hashFiles('**/*.rs', '**/Cargo.toml') }}
4040
restore-keys: |
41+
${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}-
4142
${{ runner.os }}-cargo-
4243
4344
- name: Install dependencies

‎lib/bytecode_verification/parse_json.rs‎

Lines changed: 45 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -324,12 +324,14 @@ impl ProjectInfo {
324324
);
325325
// add base type
326326
if base_identifier.starts_with("t_struct") {
327-
let struct_slots: Vec<(u64, U256, Option<String>)> = vec![(
328-
type_name["baseType"]["referencedDeclaration"]
329-
.as_u64()
330-
.unwrap(),
327+
let struct_slots_vec: Vec<(U256, (u64, Option<String>))> = vec![(
331328
U256::from(0),
332-
None,
329+
(
330+
type_name["baseType"]["referencedDeclaration"]
331+
.as_u64()
332+
.unwrap(),
333+
None,
334+
),
333335
)];
334336
// we only need the types, so we use a dummy storage vector
335337
let mut storage: Vec<StateVariable> = vec![];
@@ -340,7 +342,7 @@ impl ProjectInfo {
340342
sources,
341343
node,
342344
type_defs,
343-
&struct_slots,
345+
&struct_slots_vec,
344346
types,
345347
&mut storage,
346348
);
@@ -371,15 +373,17 @@ impl ProjectInfo {
371373
.unwrap()
372374
.to_string();
373375
if identifier.starts_with("t_struct") {
374-
let struct_slots: Vec<(u64, U256, Option<String>)> = vec![(
375-
type_name
376-
.get("referencedDeclaration")
377-
.unwrap()
378-
.as_u64()
379-
.unwrap(),
376+
let struct_slots_vec: Vec<(U256, (u64, Option<String>))> = Vec::from([(
380377
U256::from_str("0x0").unwrap(), // this won't be used as we only have to add the types
381-
None,
382-
)];
378+
(
379+
type_name
380+
.get("referencedDeclaration")
381+
.unwrap()
382+
.as_u64()
383+
.unwrap(),
384+
None,
385+
),
386+
)]);
383387
let mut storage: Vec<StateVariable> = vec![]; // this won't be used as we only have to add the types
384388
for source in sources.values() {
385389
if let Some(ast) = source.ast.clone() {
@@ -388,7 +392,7 @@ impl ProjectInfo {
388392
sources,
389393
top_node,
390394
type_defs,
391-
&struct_slots,
395+
&struct_slots_vec,
392396
types,
393397
&mut storage,
394398
);
@@ -485,14 +489,14 @@ impl ProjectInfo {
485489
sources: &BTreeMap<PathBuf, SourceFile>,
486490
node: &EAstNode,
487491
type_defs: &Types,
488-
struct_slots: &Vec<(u64, U256, Option<String>)>,
492+
struct_slots: &Vec<(U256, (u64, Option<String>))>,
489493
types: &mut HashMap<String, TypeDescription>,
490494
storage: &mut Vec<StateVariable>,
491495
) {
492496
if node.node_type == NodeType::StructDefinition && node.id.is_some() {
493497
let mut storage_var_id: Option<usize> = None;
494498
// parse all struct definitions for each struct -> slot pair.
495-
for (struct_id, slot, name) in struct_slots {
499+
for (slot, (struct_id, name)) in struct_slots {
496500
let struct_id = *struct_id;
497501
let node_id = node.id.unwrap() as u64;
498502
if node_id == struct_id {
@@ -638,7 +642,7 @@ impl ProjectInfo {
638642
types: &mut HashMap<String, TypeDescription>,
639643
) {
640644
// Tuple: (struct AST ID, slot, name of variable containing the slot)
641-
let mut struct_slots: Vec<(u64, U256, Option<String>)> = vec![];
645+
let mut struct_slots: HashMap<U256, (u64, Option<String>)> = HashMap::new();
642646
// find pairs (storage slot => struct AST ID)
643647
for source in sources.values() {
644648
if let Some(ast) = source.ast.clone() {
@@ -647,6 +651,12 @@ impl ProjectInfo {
647651
}
648652
}
649653
}
654+
655+
// Order struct_slots by key for deterministic results
656+
let mut struct_slots_vec: Vec<(U256, (u64, Option<String>))> =
657+
struct_slots.iter().map(|(k, v)| (*k, v.clone())).collect();
658+
struct_slots_vec.sort_by(|a, b| a.0.cmp(&b.0));
659+
650660
// parse the struct members + types
651661
for source in sources.values() {
652662
if let Some(ast) = source.ast.clone() {
@@ -655,7 +665,7 @@ impl ProjectInfo {
655665
sources,
656666
node,
657667
type_defs,
658-
&struct_slots,
668+
&struct_slots_vec,
659669
types,
660670
storage,
661671
);
@@ -671,7 +681,7 @@ impl ProjectInfo {
671681
sources: &BTreeMap<PathBuf, SourceFile>,
672682
node: &EAstNode,
673683
exported_ids: &Vec<usize>,
674-
struct_slots: &mut Vec<(u64, U256, Option<String>)>,
684+
struct_slots: &mut HashMap<U256, (u64, Option<String>)>,
675685
) {
676686
if node.node_type == NodeType::ContractDefinition
677687
&& node.id.is_some()
@@ -745,7 +755,7 @@ impl ProjectInfo {
745755
top_node,
746756
stmt_ref["declaration"].as_u64().unwrap()
747757
) {
748-
struct_slots.push((struct_id, var_slot, Some(var_name)));
758+
struct_slots.insert(var_slot, (struct_id, Some(var_name)));
749759
// if no variable declaration can be found, try to find
750760
// functions with the variable as parameter.
751761
} else if let Some((_, _, function_id, param_id))
@@ -799,25 +809,15 @@ impl ProjectInfo {
799809
top_node,
800810
var_ref_id.as_u64().unwrap()
801811
) {
802-
if !struct_slots.iter().any(|(_, slot, _)| slot.eq(&var_slot)) {
803-
struct_slots.push((struct_id, var_slot, Some(var_name)));
804-
}
812+
struct_slots.entry(var_slot).or_insert((struct_id, Some(var_name)));
805813
}
806814
}
807815
} else if let Some(slot_value) = arg[param_id].get("value") {
808816
// if a value is passed, use it as slot.
809817
// as we have no associated variable for the slot,
810818
// we use the name of the outer function.
811819
let var_slot = U256::from_str(slot_value.as_str().unwrap()).unwrap();
812-
if !struct_slots.iter().any(|(_, slot, _)| slot.eq(&var_slot)) {
813-
struct_slots.push(
814-
(
815-
struct_id,
816-
var_slot,
817-
Some(format!("[{}]", outer_function))
818-
)
819-
);
820-
}
820+
struct_slots.entry(var_slot).or_insert((struct_id, Some(outer_function)));
821821
}
822822
}
823823
}
@@ -855,16 +855,18 @@ impl ProjectInfo {
855855
} else {
856856
function_name = None;
857857
}
858-
struct_slots.push((
859-
struct_id,
860-
U256::from_str(
861-
slot_value
862-
.as_str()
863-
.unwrap(),
864-
)
865-
.unwrap(),
866-
function_name,
867-
));
858+
let var_slot = U256::from_str(
859+
slot_value
860+
.as_str()
861+
.unwrap(),
862+
)
863+
.unwrap();
864+
struct_slots
865+
.entry(var_slot)
866+
.or_insert((
867+
struct_id,
868+
function_name,
869+
));
868870
}
869871
}
870872
}

‎lib/dvf/discovery.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,15 @@ pub fn discover_storage_and_events(
174174
);
175175
contract_state.add_forge_inspect(&fi_impl_layout, &fi_impl_ir);
176176

177-
storage_layout.extend(tmp_project_info.storage.clone());
177+
// Extend storage with implementation storage variables, ensuring unique slots
178+
for storage_var in &tmp_project_info.storage {
179+
if !storage_layout
180+
.iter()
181+
.any(|existing| existing.slot == storage_var.slot)
182+
{
183+
storage_layout.push(storage_var.clone());
184+
}
185+
}
178186
types.extend(tmp_project_info.types.clone());
179187
imp_project_info = Some(tmp_project_info);
180188
}

‎test1.json‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
{
2+
"version": "0.9.1",
3+
"id": "0x9c82796f5193c4db0965d63046ed83572b2e8140f2be2f9587125b37e8d8710c",
4+
"contract_name": "ConstructorFactory",
5+
"address": "0x5fbdb2315678afecb367f032d93f642f64180aa3",
6+
"chain_id": 31337,
7+
"deployment_block_num": 1,
8+
"init_block_num": 2,
9+
"deployment_tx": "0x9f852dc8e8b0ad7dee5533ba7854bb676f3f270a3720c7501bbd0a65b1194663",
10+
"codehash": "0x035d6d745ec488636551786986451c4f4d368d1fd9facc2c4f11b8b3733f5aa9",
11+
"insecure": false,
12+
"immutables": [],
13+
"constructor_args": [],
14+
"critical_storage_variables": [
15+
{
16+
"slot": "0x0",
17+
"offset": 0,
18+
"var_name": "children.length",
19+
"var_type": "t_uint256",
20+
"value": "0x0000000000000000000000000000000000000000000000000000000000000002",
21+
"value_hint": "2",
22+
"comparison_operator": "Equal"
23+
},
24+
{
25+
"slot": "0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563",
26+
"offset": 0,
27+
"var_name": "children[0]",
28+
"var_type": "t_address",
29+
"value": "0xa16e02e87b7454126e5e10d957a927a7f5b5d2be",
30+
"comparison_operator": "Equal"
31+
},
32+
{
33+
"slot": "0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e564",
34+
"offset": 0,
35+
"var_name": "children[1]",
36+
"var_type": "t_address",
37+
"value": "0xb7a5bd0345ef1cc5e66bf61bdec17d2461fbd968",
38+
"comparison_operator": "Equal"
39+
}
40+
],
41+
"critical_events": [],
42+
"unvalidated_metadata": {
43+
"author_name": "Author",
44+
"description": "System Description",
45+
"hardfork": [
46+
"paris",
47+
"shanghai"
48+
],
49+
"audit_report": "https://example.org/report.pdf",
50+
"source_url": "https://github.com/source/code",
51+
"security_contact": "security@example.org"
52+
}
53+
}

‎tests/Contracts/script/Deploy_ConstructorFactory.s.sol‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
pragma solidity ^0.8.12;
2+
3+
import "forge-std/Script.sol";
4+
import "../src/ConstructorFactory.sol";
5+
6+
contract Deploy is Script {
7+
function run() external {
8+
uint256 anvilDefaultKey = 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80;
9+
vm.startBroadcast(anvilDefaultKey);
10+
11+
ConstructorFactory factory = new ConstructorFactory();
12+
factory.createChild(100);
13+
factory.createChild(200);
14+
15+
for (uint256 i = 0; i < 5; i++) {
16+
factory.dummy();
17+
}
18+
19+
vm.stopBroadcast();
20+
}
21+
}

‎tests/Contracts/src/ConstructorFactory.sol‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
// SPDX-License-Identifier: UNLICENSED
2+
pragma solidity ^0.8.13;
3+
4+
contract Child {
5+
address public owner;
6+
uint256 public value;
7+
8+
constructor(address _owner, uint256 _value) {
9+
owner = _owner;
10+
value = _value;
11+
}
12+
13+
function setValue(uint256 _value) external {
14+
value = _value;
15+
}
16+
}
17+
18+
contract ConstructorFactory {
19+
address[] public children;
20+
21+
constructor() {
22+
Child child = new Child(msg.sender, 42);
23+
children.push(address(child));
24+
}
25+
26+
function createChild(uint256 _value) external returns (address) {
27+
Child child = new Child(msg.sender, _value);
28+
children.push(address(child));
29+
return address(child);
30+
}
31+
32+
function getChildrenCount() external view returns (uint256) {
33+
return children.length;
34+
}
35+
36+
function dummy() external {}
37+
}

‎tests/ci_tests.sh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ pkill geth || true
2424
pkill anvil || true
2525
pkill cached_proxy || true
2626
rm -rf /tmp/uni-factory /tmp/usdc_implementation2
27+
cargo clean
2728
cargo build
2829
cargo clippy
2930
mkdir -p /tmp/dvfs

‎tests/expected_dvfs/CrazyHiddenStruct.dvf.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"version": "0.9.1",
3-
"id": "0x71f27bd042cf53e6783d03336d86171f7d728e61f867e60071ae32818de10da2",
3+
"id": "0x294a145622ea4fa416a4fe6395b3a307e7d692e6956c2fd0e2192831c7e776fa",
44
"contract_name": "CrazyHiddenStruct",
55
"address": "0x5fbdb2315678afecb367f032d93f642f64180aa3",
66
"chain_id": 1337,
@@ -484,7 +484,7 @@
484484
{
485485
"slot": "0xa83659c989cfe332581a2ed207e0e6d23d9199b0de773442a1e23a9b8c5138f0",
486486
"offset": 0,
487-
"var_name": "CrazyHiddenStruct.[_setOwner].AddressSlot.value",
487+
"var_name": "CrazyHiddenStruct._setOwner.AddressSlot.value",
488488
"var_type": "t_address",
489489
"value": "0x5fbdb2315678afecb367f032d93f642f64180aa3",
490490
"comparison_operator": "Equal"

‎tests/expected_dvfs/PullPayment.dvf.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
"deployment_block_num": 2,
88
"init_block_num": 3,
99
"deployment_tx": "0x42d75b67073ebd107239df89b1f512be1972205d5bc728bcafeb3dc5675d0d15",
10-
"codehash": "0x0d737bb1623f9f2c92d870330a5e6e657949fc96c0cd7b09d93be7565d352580",
10+
"codehash": "0x2f5330b21d5be1ae2b7e5e526f3a309b0bb64480c1cf0d9f6be0794a5a7f6ce4",
1111
"insecure": false,
1212
"immutables": [
1313
{

‎tests/expected_dvfs/PureChild.dvf.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
"deployment_block_num": 2,
88
"init_block_num": 3,
99
"deployment_tx": "0xc3f03c210e501eaca1459141ce84bb36675ca2a1cc28d6d716c9794d2b9d4e88",
10-
"codehash": "0x58f7c91b706c7b2e5694cdb86ba16504d4d8851df9522b29299fe8bb95403d57",
10+
"codehash": "0x96e74684860511bd54dc85352acf15d678b3e88a6ab3bc7414aebbaec5571159",
1111
"insecure": false,
1212
"immutables": [],
1313
"constructor_args": [],

0 commit comments

Comments
 (0)