diff --git a/prdoc/pr_12645.prdoc b/prdoc/pr_12645.prdoc new file mode 100644 index 000000000000..64eb7fdaa59f --- /dev/null +++ b/prdoc/pr_12645.prdoc @@ -0,0 +1,18 @@ +title: '[pallet-revive] Reject re-entrant instantiate at an in-construction address' +doc: +- audience: Runtime Dev + description: |- + Fixes https://github.com/paritytech/polkadot-sdk/issues/12639 + + A contract's `ContractInfo` is not written to `AccountInfoOf` until its constructor + frame pops, so the `is_contract` collision guard in `ContractInfo::new` could not see an + address that was still being constructed. A re-entrant `CREATE2` with the same salt and + code (which is nonce independent) therefore resolved to the same address and ran a second + constructor frame for one account, permanently leaking its consumer reference and code + refcount and orphaning the second child trie's storage deposit. + + `push_frame` now rejects a nested instantiate whose target address already appears as a + `Constructor` frame on the call stack, returning `DuplicateContract` (matching EIP-684). +crates: +- name: pallet-revive + bump: patch diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 7961f6be88b9..bf3495a174f6 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1235,6 +1235,15 @@ where input_data, self.exec_config, )? { + // EIP-684: an in-construction address is not in `AccountInfoOf` yet, so the + // `is_contract` guard in `ContractInfo::new` misses this re-entrant collision. + if frame.entry_point == ExportedFunction::Constructor && + self.frames().any(|f| { + f.entry_point == ExportedFunction::Constructor && + f.account_id == frame.account_id + }) { + return Err(Error::::DuplicateContract.into()); + } self.frames.try_push(frame).map_err(|_| Error::::MaxCallDepthReached)?; Ok(Some(executable)) } else { diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index a528c33e7e92..2014455909fe 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -1267,6 +1267,88 @@ fn instantiation_from_contract() { }); } +#[test] +fn reentrant_instantiate_at_same_address_is_rejected() { + // EIP-684: while `B1` constructs at address `X`, its constructor re-enters the deployer to + // instantiate the same code+salt. That resolves to `X` again and must be rejected rather + // than run a second constructor for one account. + let salt = [42u8; 32]; + + let constructor_ch = MockLoader::insert(Constructor, |ctx, _| { + // Re-enter the deployer (BOB) while we are still being constructed. + ctx.ext + .call( + &CallResources::NoLimits, + &BOB_ADDR, + U256::zero(), + vec![], + ReentrancyProtection::AllowReentry, + false, + ) + .unwrap(); + exec_success() + }); + + let invocations = Rc::new(RefCell::new(0u32)); + let second_instantiate_error = Rc::new(RefCell::new(None::)); + let factory_ch = MockLoader::insert(Call, { + let invocations = Rc::clone(&invocations); + let second_instantiate_error = Rc::clone(&second_instantiate_error); + move |ctx, _| { + *invocations.borrow_mut() += 1; + let n = *invocations.borrow(); + // Bound the recursion in case the guard fails to reject the collision. + if n <= 2 { + let min_balance = ::Currency::minimum_balance(); + let value = Pallet::::convert_native_to_evm(min_balance); + let result = ctx.ext.instantiate( + &CallResources::NoLimits, + Code::Existing(constructor_ch), + value, + vec![], + Some(&salt), + ); + if n == 2 { + if let Err(err) = &result { + *second_instantiate_error.borrow_mut() = Some(err.error); + } + } + } + exec_success() + } + }); + + ExtBuilder::default() + .with_code_hashes(MockLoader::code_hashes()) + .existential_deposit(15) + .build() + .execute_with(|| { + let min_balance = ::Currency::minimum_balance(); + set_balance(&ALICE, min_balance * 1000); + place_contract(&BOB, factory_ch); + let origin = Origin::from_account_id(ALICE); + let mut meter = + TransactionMeter::::new_from_limits(WEIGHT_LIMIT, min_balance * 100).unwrap(); + + // `B1` still constructs; only the colliding re-entrant instantiate fails. + assert_ok!(MockStack::run_call( + origin, + BOB_ADDR, + &mut meter, + Pallet::::convert_native_to_evm(min_balance * 100), + vec![], + &ExecConfig::new_substrate_tx(), + )); + + // Initial call plus one re-entry; without the guard it would recurse further. + assert_eq!(*invocations.borrow(), 2); + assert_eq!( + *second_instantiate_error.borrow(), + Some(>::DuplicateContract.into()) + ); + }); +} + #[test] fn instantiation_traps() { let dummy_ch = MockLoader::insert(Constructor, |_, _| Err("It's a trap!".into()));