From 1f7dd25dbc0db2b7cda6145760fb43123af67c72 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Sun, 18 Feb 2024 17:27:33 +0100 Subject: [PATCH 01/14] Permissioned contract deployment --- substrate/frame/contracts/src/lib.rs | 16 ++++++++++--- substrate/frame/contracts/src/tests.rs | 32 ++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 533085f2b874..806e5d2da28f 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -367,6 +367,12 @@ pub mod pallet { #[pallet::constant] type MaxDebugBufferLen: Get; + /// Origin allowed to upload code. + type UploadOrigin: EnsureOrigin; + + /// Origin allowed to instantiate code. + type InstantiateOrigin: EnsureOrigin; + /// Overarching hold reason. type RuntimeHoldReason: From; @@ -618,7 +624,7 @@ pub mod pallet { determinism: Determinism, ) -> DispatchResult { Migration::::ensure_migrated()?; - let origin = ensure_signed(origin)?; + let origin = T::UploadOrigin::ensure_origin(origin)?; Self::bare_upload_code(origin, code, storage_deposit_limit.map(Into::into), determinism) .map(|_| ()) } @@ -767,7 +773,10 @@ pub mod pallet { salt: Vec, ) -> DispatchResultWithPostInfo { Migration::::ensure_migrated()?; - let origin = ensure_signed(origin)?; + + T::UploadOrigin::ensure_origin(origin.clone())?; + let origin = T::InstantiateOrigin::ensure_origin(origin)?; + let code_len = code.len() as u32; let (module, upload_deposit) = Self::try_upload_code( @@ -826,10 +835,11 @@ pub mod pallet { salt: Vec, ) -> DispatchResultWithPostInfo { Migration::::ensure_migrated()?; + let origin = T::InstantiateOrigin::ensure_origin(origin)?; let data_len = data.len() as u32; let salt_len = salt.len() as u32; let common = CommonInput { - origin: Origin::from_runtime_origin(origin)?, + origin: Origin::from_account_id(origin), value, data, gas_limit, diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 815395910190..903a01fd171f 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -54,7 +54,7 @@ use frame_support::{ }, weights::{constants::WEIGHT_REF_TIME_PER_SECOND, Weight}, }; -use frame_system::{EventRecord, Phase}; +use frame_system::{EnsureSigned, EventRecord, Phase}; use pallet_contracts_fixtures::compile_module; use pretty_assertions::{assert_eq, assert_ne}; use sp_core::ByteArray; @@ -480,6 +480,8 @@ impl Config for Test { type MaxCodeLen = ConstU32<{ 123 * 1024 }>; type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = UnstableInterface; + type UploadOrigin = EnsureSigned; + type InstantiateOrigin = EnsureSigned; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; type RuntimeHoldReason = RuntimeHoldReason; type Migrations = crate::migration::codegen::BenchMigrations; @@ -5890,8 +5892,34 @@ fn root_cannot_instantiate() { vec![], vec![], ), - DispatchError::RootNotAllowed + DispatchError::BadOrigin + ); + }); +} + +#[test] +fn only_upload_origin_can_upload() { + let (wasm, _) = compile_module::("dummy").unwrap(); + + ExtBuilder::default().build().execute_with(|| { + let _ = Balances::set_balance(&ALICE, 1_000_000); + + assert_err!( + Contracts::upload_code( + RuntimeOrigin::root(), + wasm.clone(), + None, + Determinism::Enforced, + ), + DispatchError::BadOrigin ); + + assert_ok!(Contracts::upload_code( + RuntimeOrigin::signed(ALICE), + wasm.clone(), + None, + Determinism::Enforced, + )); }); } From 2bfc2e8b069be17f9462c7cf547e61ca6f73f6d7 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Sun, 18 Feb 2024 17:32:10 +0100 Subject: [PATCH 02/14] fix --- substrate/bin/node/runtime/src/lib.rs | 2 ++ .../contracts/mock-network/src/parachain/contracts_config.rs | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 37b0d93de296..152e5c5af261 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1353,6 +1353,8 @@ impl pallet_contracts::Config for Runtime { type MaxCodeLen = ConstU32<{ 123 * 1024 }>; type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = ConstBool; + type UploadOrigin = EnsureSigned; + type InstantiateOrigin = EnsureSigned; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; type RuntimeHoldReason = RuntimeHoldReason; #[cfg(not(feature = "runtime-benchmarks"))] diff --git a/substrate/frame/contracts/mock-network/src/parachain/contracts_config.rs b/substrate/frame/contracts/mock-network/src/parachain/contracts_config.rs index dadba394e264..e429978611e6 100644 --- a/substrate/frame/contracts/mock-network/src/parachain/contracts_config.rs +++ b/substrate/frame/contracts/mock-network/src/parachain/contracts_config.rs @@ -25,7 +25,7 @@ use frame_support::{ traits::{ConstBool, ConstU32, Contains, Randomness}, weights::Weight, }; -use frame_system::pallet_prelude::BlockNumberFor; +use frame_system::{pallet_prelude::BlockNumberFor, EnsureSigned}; use pallet_xcm::BalanceOf; use sp_runtime::{traits::Convert, Perbill}; @@ -90,6 +90,8 @@ impl pallet_contracts::Config for Runtime { type Schedule = Schedule; type Time = super::Timestamp; type UnsafeUnstableInterface = ConstBool; + type UploadOrigin = EnsureSigned; + type InstantiateOrigin = EnsureSigned; type WeightInfo = (); type WeightPrice = Self; type Debug = (); From c2ae53baa0c232e5cc59bdf69c440dd573e28e26 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Sun, 18 Feb 2024 18:31:26 +0100 Subject: [PATCH 03/14] prdoc --- prdoc/pr_3377.prdoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 prdoc/pr_3377.prdoc diff --git a/prdoc/pr_3377.prdoc b/prdoc/pr_3377.prdoc new file mode 100644 index 000000000000..65a9062e9fb7 --- /dev/null +++ b/prdoc/pr_3377.prdoc @@ -0,0 +1,13 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Permissioned contract deployment + +doc: + - audience: Runtime Dev + description: | + This PR introduces two new config types that specify the origins allowed to + upload and instantiate contract code. + +crates: +- name: pallet-contracts From ff3616dd8dc11a79fca37d0efdd847594604bac3 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Sun, 18 Feb 2024 18:40:39 +0100 Subject: [PATCH 04/14] add missing --- .../runtimes/contracts/contracts-rococo/src/contracts.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/contracts.rs b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/contracts.rs index 7b89f2df8077..131ee9c224b2 100644 --- a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/contracts.rs +++ b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/contracts.rs @@ -21,6 +21,7 @@ use frame_support::{ parameter_types, traits::{ConstBool, ConstU32, Nothing}, }; +use frame_system::EnsureSigned; use pallet_contracts::{ weights::SubstrateWeight, Config, DebugInfo, DefaultAddressGenerator, Frame, Schedule, }; @@ -65,6 +66,8 @@ impl Config for Runtime { type MaxCodeLen = ConstU32<{ 123 * 1024 }>; type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = ConstBool; + type UploadOrigin = EnsureSigned; + type InstantiateOrigin = EnsureSigned; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; From 5fc7551f2a1369a9d2dc1c57c18e3acc186fa772 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Mon, 26 Feb 2024 07:37:53 +0100 Subject: [PATCH 05/14] improve docs --- prdoc/pr_3377.prdoc | 3 ++- substrate/frame/contracts/src/lib.rs | 13 +++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/prdoc/pr_3377.prdoc b/prdoc/pr_3377.prdoc index 65a9062e9fb7..8e5b3935512b 100644 --- a/prdoc/pr_3377.prdoc +++ b/prdoc/pr_3377.prdoc @@ -7,7 +7,8 @@ doc: - audience: Runtime Dev description: | This PR introduces two new config types that specify the origins allowed to - upload and instantiate contract code. + upload and instantiate contract code. However, this check is not enforced when + a contract instantiates another contract. crates: - name: pallet-contracts diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 806e5d2da28f..33e93cb5b79e 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -371,6 +371,8 @@ pub mod pallet { type UploadOrigin: EnsureOrigin; /// Origin allowed to instantiate code. + /// + /// NOTE: This is not enforced when a contract instantiates another contract. type InstantiateOrigin: EnsureOrigin; /// Overarching hold reason. @@ -774,13 +776,16 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { Migration::::ensure_migrated()?; - T::UploadOrigin::ensure_origin(origin.clone())?; - let origin = T::InstantiateOrigin::ensure_origin(origin)?; + // These two origins will usually be the same; however, we treat them as separate since + // it is possible for the `Success` value of `UploadOrigin` and `InstantiateOrigin` to + // differ. + let upload_origin = T::UploadOrigin::ensure_origin(origin.clone())?; + let instantiate_origin = T::InstantiateOrigin::ensure_origin(origin)?; let code_len = code.len() as u32; let (module, upload_deposit) = Self::try_upload_code( - origin.clone(), + upload_origin, code, storage_deposit_limit.clone().map(Into::into), Determinism::Enforced, @@ -794,7 +799,7 @@ pub mod pallet { let data_len = data.len() as u32; let salt_len = salt.len() as u32; let common = CommonInput { - origin: Origin::from_account_id(origin), + origin: Origin::from_account_id(instantiate_origin), value, data, gas_limit, From d75cd9275cc1ab891ae2786f2b2d5bffe944cb0e Mon Sep 17 00:00:00 2001 From: Szegoo Date: Fri, 1 Mar 2024 10:31:15 +0100 Subject: [PATCH 06/14] more tests --- substrate/frame/contracts/src/tests.rs | 124 ++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 4 deletions(-) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index a85a0b129d5e..8422877f0fe9 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -45,6 +45,7 @@ use frame_support::{ assert_err, assert_err_ignore_postinfo, assert_err_with_weight, assert_noop, assert_ok, derive_impl, dispatch::{DispatchErrorWithPostInfo, PostDispatchInfo}, + pallet_prelude::EnsureOrigin, parameter_types, storage::child, traits::{ @@ -54,7 +55,7 @@ use frame_support::{ }, weights::{constants::WEIGHT_REF_TIME_PER_SECOND, Weight}, }; -use frame_system::{EnsureSigned, EventRecord, Phase}; +use frame_system::{EventRecord, Phase}; use pallet_contracts_fixtures::compile_module; use pretty_assertions::{assert_eq, assert_ne}; use sp_core::ByteArray; @@ -453,6 +454,48 @@ impl Contains for TestFilter { } } +parameter_types! { + static UploadOrigin: ::AccountId = ALICE; + static InstantiateOrigin: ::AccountId = ALICE; + static RestrictedUpload: bool = false; + static RestrictedInstantiation: bool = false; +} + +pub struct EnsureUploadOrigin(sp_std::marker::PhantomData); +impl EnsureOrigin<::RuntimeOrigin> for EnsureUploadOrigin +where + ::AccountId: From, +{ + type Success = T::AccountId; + + fn try_origin(o: T::RuntimeOrigin) -> Result { + let who = as EnsureOrigin<_>>::try_origin(o.clone())?; + if RestrictedUpload::get() && who != UploadOrigin::get().into() { + return Err(o) + } + + Ok(who) + } +} + +pub struct EnsureInstantiateOrigin(sp_std::marker::PhantomData); +impl EnsureOrigin<::RuntimeOrigin> + for EnsureInstantiateOrigin +where + ::AccountId: From, +{ + type Success = T::AccountId; + + fn try_origin(o: T::RuntimeOrigin) -> Result { + let who = as EnsureOrigin<_>>::try_origin(o.clone())?; + if RestrictedInstantiation::get() && who != InstantiateOrigin::get().into() { + return Err(o) + } + + Ok(who) + } +} + parameter_types! { pub static UnstableInterface: bool = true; } @@ -477,8 +520,8 @@ impl Config for Test { type MaxCodeLen = ConstU32<{ 123 * 1024 }>; type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = UnstableInterface; - type UploadOrigin = EnsureSigned; - type InstantiateOrigin = EnsureSigned; + type UploadOrigin = EnsureUploadOrigin; + type InstantiateOrigin = EnsureInstantiateOrigin; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; type RuntimeHoldReason = RuntimeHoldReason; type Migrations = crate::migration::codegen::BenchMigrations; @@ -5952,9 +5995,10 @@ fn root_cannot_instantiate() { #[test] fn only_upload_origin_can_upload() { let (wasm, _) = compile_module::("dummy").unwrap(); - + RestrictedUpload::set(true); ExtBuilder::default().build().execute_with(|| { let _ = Balances::set_balance(&ALICE, 1_000_000); + let _ = Balances::set_balance(&BOB, 1_000_000); assert_err!( Contracts::upload_code( @@ -5966,6 +6010,17 @@ fn only_upload_origin_can_upload() { DispatchError::BadOrigin ); + assert_err!( + Contracts::upload_code( + RuntimeOrigin::signed(BOB), + wasm.clone(), + None, + Determinism::Enforced, + ), + DispatchError::BadOrigin + ); + + // Only alice is allowed to upload contract code. assert_ok!(Contracts::upload_code( RuntimeOrigin::signed(ALICE), wasm.clone(), @@ -5975,6 +6030,67 @@ fn only_upload_origin_can_upload() { }); } +#[test] +fn only_instantiation_origin_can_instantiate() { + let (code, code_hash) = compile_module::("dummy").unwrap(); + RestrictedInstantiation::set(true); + ExtBuilder::default().build().execute_with(|| { + let _ = Balances::set_balance(&ALICE, 1_000_000); + let _ = Balances::set_balance(&BOB, 1_000_000); + + assert_err_ignore_postinfo!( + Contracts::instantiate_with_code( + RuntimeOrigin::root(), + 0, + GAS_LIMIT, + None, + code.clone(), + vec![], + vec![], + ), + DispatchError::BadOrigin + ); + + assert_err_ignore_postinfo!( + Contracts::instantiate_with_code( + RuntimeOrigin::signed(BOB), + 0, + GAS_LIMIT, + None, + code.clone(), + vec![], + vec![], + ), + DispatchError::BadOrigin + ); + + // Only Alice can instantiate + assert_ok!(Contracts::instantiate_with_code( + RuntimeOrigin::signed(ALICE), + 0, + GAS_LIMIT, + None, + code, + vec![], + vec![], + ),); + + // Bob cannot instantiate with either `instantiate_with_code` or `instantiate`. + assert_err_ignore_postinfo!( + Contracts::instantiate( + RuntimeOrigin::signed(BOB), + 0, + GAS_LIMIT, + None, + code_hash, + vec![], + vec![], + ), + DispatchError::BadOrigin + ); + }); +} + #[test] fn balance_api_returns_free_balance() { let (wasm, _code_hash) = compile_module::("balance").unwrap(); From 33a2cfd6d98e268782c3e055c7d8074fddc95501 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Fri, 1 Mar 2024 10:32:11 +0100 Subject: [PATCH 07/14] Update substrate/frame/contracts/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alexander Theißen --- substrate/frame/contracts/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 33e93cb5b79e..2cdb8bca19f4 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -372,7 +372,10 @@ pub mod pallet { /// Origin allowed to instantiate code. /// - /// NOTE: This is not enforced when a contract instantiates another contract. + /// # Note + /// + /// This is not enforced when a contract instantiates another contract. The [`UploadOrigin`] should + /// make sure that no code is deployed that does unwanted instantiations. type InstantiateOrigin: EnsureOrigin; /// Overarching hold reason. From a9c4d571548f41cba42e9e522225bfc41b4ee0fa Mon Sep 17 00:00:00 2001 From: Szegoo Date: Fri, 1 Mar 2024 10:36:43 +0100 Subject: [PATCH 08/14] docs --- substrate/frame/contracts/src/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 2cdb8bca19f4..abca865d1bd1 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -368,14 +368,20 @@ pub mod pallet { type MaxDebugBufferLen: Get; /// Origin allowed to upload code. + /// + /// By default, it is safe to set this to `EnsureSigned`, allowing anyone to upload contract + /// code. type UploadOrigin: EnsureOrigin; /// Origin allowed to instantiate code. /// /// # Note /// - /// This is not enforced when a contract instantiates another contract. The [`UploadOrigin`] should - /// make sure that no code is deployed that does unwanted instantiations. + /// This is not enforced when a contract instantiates another contract. The [`UploadOrigin`] + /// should make sure that no code is deployed that does unwanted instantiations. + /// + /// By default, it is safe to set this to `EnsureSigned`, allowing anyone to instantiate + /// contract code. type InstantiateOrigin: EnsureOrigin; /// Overarching hold reason. From 44e6219c92dffca0fd0b8ba4d5e1dd65b63790d0 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Fri, 1 Mar 2024 10:46:59 +0100 Subject: [PATCH 09/14] fix --- substrate/frame/contracts/src/tests.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 8422877f0fe9..c34a1fd2cf48 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -476,6 +476,11 @@ where Ok(who) } + + #[cfg(feature = "runtime-benchmarks")] + fn try_successful_origin() -> Result { + Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed(UploadOrigin::get().into()))) + } } pub struct EnsureInstantiateOrigin(sp_std::marker::PhantomData); @@ -494,6 +499,11 @@ where Ok(who) } + + #[cfg(feature = "runtime-benchmarks")] + fn try_successful_origin() -> Result { + Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed(InstantiateOrigin::get().into()))) + } } parameter_types! { From 8efe7cca98b6cd9fce926e7a96aa7bbc47a84cf5 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Sat, 2 Mar 2024 07:56:00 +0100 Subject: [PATCH 10/14] Update substrate/frame/contracts/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alexander Theißen --- substrate/frame/contracts/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index abca865d1bd1..12536e4600a5 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -377,7 +377,7 @@ pub mod pallet { /// /// # Note /// - /// This is not enforced when a contract instantiates another contract. The [`UploadOrigin`] + /// This is not enforced when a contract instantiates another contract. The [`Self::UploadOrigin`] /// should make sure that no code is deployed that does unwanted instantiations. /// /// By default, it is safe to set this to `EnsureSigned`, allowing anyone to instantiate From 0553427af7a9a96e4983022da80e73019317c3d1 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Sat, 2 Mar 2024 07:56:17 +0100 Subject: [PATCH 11/14] Update substrate/frame/contracts/src/tests.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alexander Theißen --- substrate/frame/contracts/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index c34a1fd2cf48..b930852cfe7e 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -455,8 +455,8 @@ impl Contains for TestFilter { } parameter_types! { - static UploadOrigin: ::AccountId = ALICE; - static InstantiateOrigin: ::AccountId = ALICE; + static UploadAccount: ::AccountId = ALICE; + static InstantiateAccount: ::AccountId = ALICE; static RestrictedUpload: bool = false; static RestrictedInstantiation: bool = false; } From 6ea9b4bfcb20ba0c3ed9dbf2da9647ffdf43300b Mon Sep 17 00:00:00 2001 From: Szegoo Date: Sat, 2 Mar 2024 07:58:45 +0100 Subject: [PATCH 12/14] rename --- substrate/frame/contracts/src/lib.rs | 5 +++-- substrate/frame/contracts/src/tests.rs | 22 ++++++++++++---------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 12536e4600a5..3ff937143a63 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -377,8 +377,9 @@ pub mod pallet { /// /// # Note /// - /// This is not enforced when a contract instantiates another contract. The [`Self::UploadOrigin`] - /// should make sure that no code is deployed that does unwanted instantiations. + /// This is not enforced when a contract instantiates another contract. The + /// [`Self::UploadOrigin`] should make sure that no code is deployed that does unwanted + /// instantiations. /// /// By default, it is safe to set this to `EnsureSigned`, allowing anyone to instantiate /// contract code. diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index b930852cfe7e..7ccd6101c1ab 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -461,8 +461,8 @@ parameter_types! { static RestrictedInstantiation: bool = false; } -pub struct EnsureUploadOrigin(sp_std::marker::PhantomData); -impl EnsureOrigin<::RuntimeOrigin> for EnsureUploadOrigin +pub struct EnsureUploadAccount(sp_std::marker::PhantomData); +impl EnsureOrigin<::RuntimeOrigin> for EnsureUploadAccount where ::AccountId: From, { @@ -470,7 +470,7 @@ where fn try_origin(o: T::RuntimeOrigin) -> Result { let who = as EnsureOrigin<_>>::try_origin(o.clone())?; - if RestrictedUpload::get() && who != UploadOrigin::get().into() { + if RestrictedUpload::get() && who != UploadAccount::get().into() { return Err(o) } @@ -479,13 +479,13 @@ where #[cfg(feature = "runtime-benchmarks")] fn try_successful_origin() -> Result { - Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed(UploadOrigin::get().into()))) + Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed(UploadAccount::get().into()))) } } -pub struct EnsureInstantiateOrigin(sp_std::marker::PhantomData); +pub struct EnsureInstantiateAccount(sp_std::marker::PhantomData); impl EnsureOrigin<::RuntimeOrigin> - for EnsureInstantiateOrigin + for EnsureInstantiateAccount where ::AccountId: From, { @@ -493,7 +493,7 @@ where fn try_origin(o: T::RuntimeOrigin) -> Result { let who = as EnsureOrigin<_>>::try_origin(o.clone())?; - if RestrictedInstantiation::get() && who != InstantiateOrigin::get().into() { + if RestrictedInstantiation::get() && who != InstantiateAccount::get().into() { return Err(o) } @@ -502,7 +502,9 @@ where #[cfg(feature = "runtime-benchmarks")] fn try_successful_origin() -> Result { - Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed(InstantiateOrigin::get().into()))) + Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed( + InstantiateAccount::get().into(), + ))) } } @@ -530,8 +532,8 @@ impl Config for Test { type MaxCodeLen = ConstU32<{ 123 * 1024 }>; type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = UnstableInterface; - type UploadOrigin = EnsureUploadOrigin; - type InstantiateOrigin = EnsureInstantiateOrigin; + type UploadOrigin = EnsureUploadAccount; + type InstantiateOrigin = EnsureInstantiateAccount; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; type RuntimeHoldReason = RuntimeHoldReason; type Migrations = crate::migration::codegen::BenchMigrations; From ef5beab1a4c4daf010a5319db6291808d7bfac13 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Mon, 4 Mar 2024 19:43:10 +0100 Subject: [PATCH 13/14] Update substrate/frame/contracts/src/tests.rs Co-authored-by: PG Herveou --- substrate/frame/contracts/src/tests.rs | 41 +++++--------------------- 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 7ccd6101c1ab..9dee7cc6d8ee 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -455,14 +455,13 @@ impl Contains for TestFilter { } parameter_types! { - static UploadAccount: ::AccountId = ALICE; - static InstantiateAccount: ::AccountId = ALICE; - static RestrictedUpload: bool = false; - static RestrictedInstantiation: bool = false; + pub static UploadAccount: Option<::AccountId> = None; + pub static InstantiateAccount: Option<::AccountId> = None; } -pub struct EnsureUploadAccount(sp_std::marker::PhantomData); -impl EnsureOrigin<::RuntimeOrigin> for EnsureUploadAccount +pub struct EnsureAccount(sp_std::marker::PhantomData<(T, A)>); +impl>>> + EnsureOrigin<::RuntimeOrigin> for EnsureAccount where ::AccountId: From, { @@ -470,7 +469,7 @@ where fn try_origin(o: T::RuntimeOrigin) -> Result { let who = as EnsureOrigin<_>>::try_origin(o.clone())?; - if RestrictedUpload::get() && who != UploadAccount::get().into() { + if matches!(A::get(), Some(a) if who != a) { return Err(o) } @@ -479,35 +478,9 @@ where #[cfg(feature = "runtime-benchmarks")] fn try_successful_origin() -> Result { - Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed(UploadAccount::get().into()))) + Err(()) } } - -pub struct EnsureInstantiateAccount(sp_std::marker::PhantomData); -impl EnsureOrigin<::RuntimeOrigin> - for EnsureInstantiateAccount -where - ::AccountId: From, -{ - type Success = T::AccountId; - - fn try_origin(o: T::RuntimeOrigin) -> Result { - let who = as EnsureOrigin<_>>::try_origin(o.clone())?; - if RestrictedInstantiation::get() && who != InstantiateAccount::get().into() { - return Err(o) - } - - Ok(who) - } - - #[cfg(feature = "runtime-benchmarks")] - fn try_successful_origin() -> Result { - Ok(T::RuntimeOrigin::from(frame_system::RawOrigin::Signed( - InstantiateAccount::get().into(), - ))) - } -} - parameter_types! { pub static UnstableInterface: bool = true; } From 0c7b7f28287b9d0c28ed73573e3b3f9a7449b761 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Mon, 4 Mar 2024 19:46:34 +0100 Subject: [PATCH 14/14] fix --- substrate/frame/contracts/src/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 9dee7cc6d8ee..7f45102e146e 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -505,8 +505,8 @@ impl Config for Test { type MaxCodeLen = ConstU32<{ 123 * 1024 }>; type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = UnstableInterface; - type UploadOrigin = EnsureUploadAccount; - type InstantiateOrigin = EnsureInstantiateAccount; + type UploadOrigin = EnsureAccount; + type InstantiateOrigin = EnsureAccount; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; type RuntimeHoldReason = RuntimeHoldReason; type Migrations = crate::migration::codegen::BenchMigrations; @@ -5980,7 +5980,7 @@ fn root_cannot_instantiate() { #[test] fn only_upload_origin_can_upload() { let (wasm, _) = compile_module::("dummy").unwrap(); - RestrictedUpload::set(true); + UploadAccount::set(Some(ALICE)); ExtBuilder::default().build().execute_with(|| { let _ = Balances::set_balance(&ALICE, 1_000_000); let _ = Balances::set_balance(&BOB, 1_000_000); @@ -6018,7 +6018,7 @@ fn only_upload_origin_can_upload() { #[test] fn only_instantiation_origin_can_instantiate() { let (code, code_hash) = compile_module::("dummy").unwrap(); - RestrictedInstantiation::set(true); + InstantiateAccount::set(Some(ALICE)); ExtBuilder::default().build().execute_with(|| { let _ = Balances::set_balance(&ALICE, 1_000_000); let _ = Balances::set_balance(&BOB, 1_000_000);