From 274a781e8ca1a9432c7ec87593bd93214abbff50 Mon Sep 17 00:00:00 2001
From: xermicus <cyrill@parity.io>
Date: Mon, 3 Feb 2025 16:59:39 +0100
Subject: [PATCH] [pallet-revive] do not trap the caller on instantiations with
 duplicate contracts (#7414)

This PR changes the behavior of `instantiate` when the resulting
contract address already exists (because the caller tried to instantiate
the same contract with the same salt multiple times): Instead of
trapping the caller, return an error code.

Solidity allows `catch`ing this, which doesn't work if we are trapping
the caller. For example, the change makes the following snippet work:

```Solidity
try new Foo{salt: hex"00"}() returns (Foo) {
    // Instantiation was successful (contract address was free and constructor did not revert)
} catch {
    // This branch is expected to be taken if the instantiation failed because of a duplicate salt
}
```

`revive` PR: https://github.com/paritytech/revive/pull/188

---------

Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
---
 prdoc/pr_7414.prdoc                        | 20 ++++++++++++++++++++
 substrate/frame/revive/src/tests.rs        | 12 ++++++++++++
 substrate/frame/revive/src/wasm/runtime.rs |  2 ++
 substrate/frame/revive/uapi/src/lib.rs     |  3 +++
 4 files changed, 37 insertions(+)
 create mode 100644 prdoc/pr_7414.prdoc

diff --git a/prdoc/pr_7414.prdoc b/prdoc/pr_7414.prdoc
new file mode 100644
index 000000000000..41edc1fe7cb3
--- /dev/null
+++ b/prdoc/pr_7414.prdoc
@@ -0,0 +1,20 @@
+title: '[pallet-revive] do not trap the caller on instantiations with duplicate contracts'
+doc:
+- audience: Runtime Dev
+  description: |-
+    This PR changes the behavior of `instantiate` when the resulting contract address already exists (because the caller tried to instantiate the same contract with the same salt multiple times): Instead of trapping the caller, return an error code.
+
+    Solidity allows `catch`ing this, which doesn't work if we are trapping the caller. For example, the change makes the following snippet work:
+
+    ```Solidity
+    try new Foo{salt: hex"00"}() returns (Foo) {
+        // Instantiation was successful (contract address was free and constructor did not revert)
+    } catch {
+        // This branch is expected to be taken if the instantiation failed because of a duplicate salt
+    }
+    ```
+crates:
+- name: pallet-revive
+  bump: major
+- name: pallet-revive-uapi
+  bump: major
diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs
index 50b14c2a9987..366a378cfd11 100644
--- a/substrate/frame/revive/src/tests.rs
+++ b/substrate/frame/revive/src/tests.rs
@@ -1614,6 +1614,18 @@ fn instantiate_return_code() {
 			.data(callee_hash.iter().chain(&2u32.to_le_bytes()).cloned().collect())
 			.build_and_unwrap_result();
 		assert_return_code!(result, RuntimeReturnCode::CalleeTrapped);
+
+		// Contract instantiation succeeds
+		let result = builder::bare_call(contract.addr)
+			.data(callee_hash.iter().chain(&0u32.to_le_bytes()).cloned().collect())
+			.build_and_unwrap_result();
+		assert_return_code!(result, 0);
+
+		// Contract instantiation fails because the same salt is being used again.
+		let result = builder::bare_call(contract.addr)
+			.data(callee_hash.iter().chain(&0u32.to_le_bytes()).cloned().collect())
+			.build_and_unwrap_result();
+		assert_return_code!(result, RuntimeReturnCode::DuplicateContractAddress);
 	});
 }
 
diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs
index 5312ad2db1b0..ad92e5fecee2 100644
--- a/substrate/frame/revive/src/wasm/runtime.rs
+++ b/substrate/frame/revive/src/wasm/runtime.rs
@@ -792,10 +792,12 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> {
 		let transfer_failed = Error::<E::T>::TransferFailed.into();
 		let out_of_gas = Error::<E::T>::OutOfGas.into();
 		let out_of_deposit = Error::<E::T>::StorageDepositLimitExhausted.into();
+		let duplicate_contract = Error::<E::T>::DuplicateContract.into();
 
 		// errors in the callee do not trap the caller
 		match (from.error, from.origin) {
 			(err, _) if err == transfer_failed => Ok(TransferFailed),
+			(err, _) if err == duplicate_contract => Ok(DuplicateContractAddress),
 			(err, Callee) if err == out_of_gas || err == out_of_deposit => Ok(OutOfResources),
 			(_, Callee) => Ok(CalleeTrapped),
 			(err, _) => Err(err),
diff --git a/substrate/frame/revive/uapi/src/lib.rs b/substrate/frame/revive/uapi/src/lib.rs
index bbd647a7faed..744a2f0bca5d 100644
--- a/substrate/frame/revive/uapi/src/lib.rs
+++ b/substrate/frame/revive/uapi/src/lib.rs
@@ -98,6 +98,9 @@ define_error_codes! {
 	XcmExecutionFailed = 9,
 	/// The `xcm_send` call failed.
 	XcmSendFailed = 10,
+	/// Contract instantiation failed because the address already exists.
+	/// Occurs when instantiating the same contract with the same salt more than once.
+	DuplicateContractAddress = 11,
 }
 
 /// The raw return code returned by the host side.