fix(tracing): don't overwrite selfdestruct_address (#190)

fix paradigmxyz/reth#10755 https://github.com/paradigmxyz/revm-inspectors/blob/e8a5f56a7270a404408c83b4b92977d9d013882f/src/tracing/builder/parity.rs#L234-L236 this reminds me of `selfdestruct` reuses the call's struct, so we can't rewrite the call's `address`, especially in `delegatecall` waiting for the new release of https://github.com/bluealloy/revm --------- Signed-off-by: jsvisa <[email protected]> Co-authored-by: Matthias Seitz <[email protected]>
paradigmxyz · Sep 18, 2024 · bf6ee2a · bf6ee2a
1 parent 0648673
commit bf6ee2a
Show file tree

Hide file tree

Showing 3 changed files with 144 additions and 18 deletions.
diff --git a/src/tracing/mod.rs b/src/tracing/mod.rs
@@ -666,7 +666,7 @@ where
 
     fn selfdestruct(&mut self, contract: Address, target: Address, value: U256) {
         let node = self.last_trace();
-        node.trace.address = contract;
+        node.trace.selfdestruct_address = Some(contract);
         node.trace.selfdestruct_refund_target = Some(target);
         node.trace.selfdestruct_transferred_value = Some(value);
     }

diff --git a/src/tracing/types.rs b/src/tracing/types.rs
@@ -48,14 +48,15 @@ pub struct CallTrace {
     /// The target address of this call.
     ///
     /// This is:
-    /// - [`is_selfdestruct`](Self::is_selfdestruct): the address of the selfdestructed contract
     /// - [`CallKind::Call`] and alike: the callee, the address of the contract being called
     /// - [`CallKind::Create`] and alike: the address of the created contract
     pub address: Address,
     /// Whether this is a call to a precompile.
     ///
     /// Note: This is optional because not all tracers make use of this.
     pub maybe_precompile: Option<bool>,
+    /// The address of the selfdestructed contract.
+    pub selfdestruct_address: Option<Address>,
     /// Holds the target for the selfdestruct refund target.
     ///
     /// This is only `Some` if a selfdestruct was executed and the call is executed before the
@@ -323,7 +324,7 @@ impl CallTraceNode {
     pub fn parity_selfdestruct_action(&self) -> Option<Action> {
         self.is_selfdestruct().then(|| {
             Action::Selfdestruct(SelfdestructAction {
-                address: self.trace.address,
+                address: self.trace.selfdestruct_address.unwrap_or_default(),
                 refund_address: self.trace.selfdestruct_refund_target.unwrap_or_default(),
                 balance: self.trace.selfdestruct_transferred_value.unwrap_or_default(),
             })
@@ -334,7 +335,7 @@ impl CallTraceNode {
     pub fn geth_selfdestruct_call_trace(&self) -> Option<CallFrame> {
         self.is_selfdestruct().then(|| CallFrame {
             typ: "SELFDESTRUCT".to_string(),
-            from: self.trace.address,
+            from: self.trace.selfdestruct_address.unwrap_or_default(),
             to: self.trace.selfdestruct_refund_target,
             value: self.trace.selfdestruct_transferred_value,
             ..Default::default()

diff --git a/tests/it/parity.rs b/tests/it/parity.rs
@@ -89,18 +89,13 @@ fn test_parity_selfdestruct(spec_id: SpecId) {
     let (res, _) = inspect(&mut db, env, &mut insp).unwrap();
     assert!(res.result.is_success(), "{res:#?}");
 
-    // TODO: Transfer still happens in Cancun, but this is not reflected in the trace.
-    let (expected_value, expected_target) =
-        if spec_id < SpecId::CANCUN { (Some(value), Some(deployer)) } else { (None, None) };
-
-    {
-        assert_eq!(insp.traces().nodes().len(), 1);
-        let node = &insp.traces().nodes()[0];
-        assert!(node.is_selfdestruct(), "{node:#?}");
-        assert_eq!(node.trace.address, contract_address);
-        assert_eq!(node.trace.selfdestruct_refund_target, expected_target);
-        assert_eq!(node.trace.selfdestruct_transferred_value, expected_value);
-    }
+    assert_eq!(insp.traces().nodes().len(), 1);
+    let node = &insp.traces().nodes()[0];
+    assert!(node.is_selfdestruct(), "{node:#?}");
+    assert_eq!(node.trace.address, contract_address);
+    assert_eq!(node.trace.selfdestruct_address, Some(contract_address));
+    assert_eq!(node.trace.selfdestruct_refund_target, Some(deployer));
+    assert_eq!(node.trace.selfdestruct_transferred_value, Some(value));
 
     let traces =
         insp.into_parity_builder().into_localized_transaction_traces(TransactionInfo::default());
@@ -110,8 +105,8 @@ fn test_parity_selfdestruct(spec_id: SpecId) {
         traces[1].trace.action,
         Action::Selfdestruct(SelfdestructAction {
             address: contract_address,
-            refund_address: expected_target.unwrap_or_default(),
-            balance: expected_value.unwrap_or_default(),
+            refund_address: deployer,
+            balance: value,
         })
     );
 }
@@ -331,3 +326,133 @@ fn test_parity_statediff_blob_commit() {
     assert!(!state_diff.contains_key(&to));
     assert!(state_diff.contains_key(&caller));
 }
+
+#[test]
+fn test_parity_delegatecall_selfdestruct() {
+    /*
+    contract DelegateCall {
+        constructor() payable {}
+        function close(address target) public {
+            (bool success,) = target.delegatecall(abi.encodeWithSignature("close()"));
+            require(success, "Delegatecall failed");
+        }
+    }
+    contract SelfDestructTarget {
+        function close() public {
+            selfdestruct(payable(msg.sender));
+        }
+    }
+    */
+
+    // DelegateCall contract bytecode
+    let delegate_code = hex!("6080604052348015600e575f80fd5b506103158061001c5f395ff3fe608060405234801561000f575f80fd5b5060043610610029575f3560e01c8063c74073a11461002d575b5f80fd5b610047600480360381019061004291906101d4565b610049565b005b5f8173ffffffffffffffffffffffffffffffffffffffff166040516024016040516020818303038152906040527f43d726d6000000000000000000000000000000000000000000000000000000007bffffffffffffffffffffffffffffffffffffffffffffffffffffffff19166020820180517bffffffffffffffffffffffffffffffffffffffffffffffffffffffff83818316178352505050506040516100f19190610251565b5f60405180830381855af49150503d805f8114610129576040519150601f19603f3d011682016040523d82523d5f602084013e61012e565b606091505b5050905080610172576040517f08c379a0000000000000000000000000000000000000000000000000000000008152600401610169906102c1565b60405180910390fd5b5050565b5f80fd5b5f73ffffffffffffffffffffffffffffffffffffffff82169050919050565b5f6101a38261017a565b9050919050565b6101b381610199565b81146101bd575f80fd5b50565b5f813590506101ce816101aa565b92915050565b5f602082840312156101e9576101e8610176565b5b5f6101f6848285016101c0565b91505092915050565b5f81519050919050565b5f81905092915050565b8281835e5f83830152505050565b5f61022b826101ff565b6102358185610209565b9350610245818560208601610213565b80840191505092915050565b5f61025c8284610221565b915081905092915050565b5f82825260208201905092915050565b7f44656c656761746563616c6c206661696c6564000000000000000000000000005f82015250565b5f6102ab601383610267565b91506102b682610277565b602082019050919050565b5f6020820190508181035f8301526102d88161029f565b905091905056fea2646970667358221220f6409a1a1bfa02cbcb4d9818e921686c97eed1566fbd60951a91d232035e046c64736f6c634300081a0033");
+
+    // SelfDestructTarget contract bytecode
+    let target_code = hex!("6080604052348015600e575f80fd5b50608180601a5f395ff3fe6080604052348015600e575f80fd5b50600436106026575f3560e01c806343d726d614602a575b5f80fd5b60306032565b005b3373ffffffffffffffffffffffffffffffffffffffff16fffea26469706673582212202ecd1d2f481d093cc2831fe0350ce1fe0bc42bc5cf34eb0a9e40a83b564eb59464736f6c634300081a0033");
+
+    let deployer = address!("341348115259a8bf69f1f50101c227fced83bac6");
+    let mut db = CacheDB::new(EmptyDB::default());
+
+    let cfg = CfgEnvWithHandlerCfg::new(CfgEnv::default(), HandlerCfg::new(SpecId::CANCUN));
+
+    // Deploy DelegateCall contract
+    let env = EnvWithHandlerCfg::new_with_cfg_env(
+        cfg.clone(),
+        BlockEnv::default(),
+        TxEnv {
+            caller: deployer,
+            gas_limit: 1000000,
+            transact_to: TransactTo::Create,
+            data: delegate_code.into(),
+            ..Default::default()
+        },
+    );
+
+    let mut insp = TracingInspector::new(TracingInspectorConfig::default_parity());
+    let (res, _) = inspect(&mut db, env, &mut insp).unwrap();
+    let delegate_addr = match res.result {
+        ExecutionResult::Success { output, .. } => match output {
+            Output::Create(_, addr) => addr.unwrap(),
+            _ => panic!("Create failed"),
+        },
+        _ => panic!("Execution failed"),
+    };
+    db.commit(res.state);
+
+    // Deploy SelfDestructTarget contract
+    let env = EnvWithHandlerCfg::new_with_cfg_env(
+        cfg.clone(),
+        BlockEnv::default(),
+        TxEnv {
+            caller: deployer,
+            gas_limit: 1000000,
+            transact_to: TransactTo::Create,
+            data: target_code.into(),
+            ..Default::default()
+        },
+    );
+
+    let mut insp = TracingInspector::new(TracingInspectorConfig::default_parity());
+    let (res, _) = inspect(&mut db, env, &mut insp).unwrap();
+    let target_addr = match res.result {
+        ExecutionResult::Success { output, .. } => match output {
+            Output::Create(_, addr) => addr.unwrap(),
+            _ => panic!("Create failed"),
+        },
+        _ => panic!("Execution failed"),
+    };
+    db.commit(res.state);
+
+    // Prepare the input data for the close(address) function call
+    let mut input_data = hex!("c74073a1").to_vec(); // keccak256("close(address)")[:4]
+    input_data.extend_from_slice(&[0u8; 12]); // Pad with zeros
+    input_data.extend_from_slice(target_addr.as_slice());
+
+    // Call DelegateCall contract with SelfDestructTarget address
+    let mut insp = TracingInspector::new(TracingInspectorConfig::default_parity());
+    let env = EnvWithHandlerCfg::new_with_cfg_env(
+        cfg,
+        BlockEnv::default(),
+        TxEnv {
+            caller: deployer,
+            gas_limit: 1000000,
+            transact_to: TransactTo::Call(delegate_addr),
+            data: input_data.into(),
+            ..Default::default()
+        },
+    );
+
+    let (res, _) = inspect(&mut db, env, &mut insp).unwrap();
+    assert!(res.result.is_success());
+
+    let traces =
+        insp.into_parity_builder().into_localized_transaction_traces(TransactionInfo::default());
+
+    assert_eq!(traces.len(), 3);
+
+    let trace0 = &traces[0].trace;
+    assert!(trace0.action.is_call());
+    assert_eq!(trace0.trace_address.len(), 0);
+    assert_eq!(trace0.subtraces, 1);
+    let action0 = trace0.action.as_call().unwrap();
+    assert_eq!(action0.call_type, CallType::Call);
+    assert_eq!(action0.from, deployer);
+    assert_eq!(action0.to, delegate_addr);
+
+    let trace1 = &traces[1].trace;
+    assert!(trace1.action.is_call());
+    assert_eq!(trace1.trace_address, vec![0]);
+    assert_eq!(trace1.subtraces, 1);
+    let action1 = trace1.action.as_call().unwrap();
+    assert_eq!(action1.call_type, CallType::DelegateCall);
+    assert_eq!(action1.from, delegate_addr);
+    assert_eq!(action1.to, target_addr);
+
+    let trace2 = &traces[2].trace;
+    assert!(trace2.action.is_selfdestruct());
+    assert_eq!(trace2.trace_address, vec![0, 0]);
+    assert_eq!(trace2.subtraces, 0);
+    let action2 = trace2.action.as_selfdestruct().unwrap();
+    assert_eq!(action2.address, delegate_addr);
+    assert_eq!(action2.refund_address, deployer);
+}