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; diff --git a/prdoc/pr_3377.prdoc b/prdoc/pr_3377.prdoc new file mode 100644 index 000000000000..8e5b3935512b --- /dev/null +++ b/prdoc/pr_3377.prdoc @@ -0,0 +1,14 @@ +# 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. However, this check is not enforced when + a contract instantiates another contract. + +crates: +- name: pallet-contracts diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 24c8f6f48e96..4a423a308c36 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1357,6 +1357,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 = (); diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 533085f2b874..3ff937143a63 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -367,6 +367,24 @@ pub mod pallet { #[pallet::constant] 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 + /// [`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. + type InstantiateOrigin: EnsureOrigin; + /// Overarching hold reason. type RuntimeHoldReason: From; @@ -618,7 +636,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,11 +785,17 @@ pub mod pallet { salt: Vec, ) -> DispatchResultWithPostInfo { Migration::::ensure_migrated()?; - let origin = ensure_signed(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, @@ -785,7 +809,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, @@ -826,10 +850,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 eba959e8d465..7f45102e146e 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::{ @@ -453,6 +454,33 @@ impl Contains for TestFilter { } } +parameter_types! { + pub static UploadAccount: Option<::AccountId> = None; + pub static InstantiateAccount: Option<::AccountId> = None; +} + +pub struct EnsureAccount(sp_std::marker::PhantomData<(T, A)>); +impl>>> + EnsureOrigin<::RuntimeOrigin> for EnsureAccount +where + ::AccountId: From, +{ + type Success = T::AccountId; + + fn try_origin(o: T::RuntimeOrigin) -> Result { + let who = as EnsureOrigin<_>>::try_origin(o.clone())?; + if matches!(A::get(), Some(a) if who != a) { + return Err(o) + } + + Ok(who) + } + + #[cfg(feature = "runtime-benchmarks")] + fn try_successful_origin() -> Result { + Err(()) + } +} parameter_types! { pub static UnstableInterface: bool = true; } @@ -477,6 +505,8 @@ impl Config for Test { type MaxCodeLen = ConstU32<{ 123 * 1024 }>; type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = UnstableInterface; + type UploadOrigin = EnsureAccount; + type InstantiateOrigin = EnsureAccount; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; type RuntimeHoldReason = RuntimeHoldReason; type Migrations = crate::migration::codegen::BenchMigrations; @@ -5942,7 +5972,106 @@ fn root_cannot_instantiate() { vec![], vec![], ), - DispatchError::RootNotAllowed + DispatchError::BadOrigin + ); + }); +} + +#[test] +fn only_upload_origin_can_upload() { + let (wasm, _) = compile_module::("dummy").unwrap(); + 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); + + assert_err!( + Contracts::upload_code( + RuntimeOrigin::root(), + wasm.clone(), + None, + Determinism::Enforced, + ), + 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(), + None, + Determinism::Enforced, + )); + }); +} + +#[test] +fn only_instantiation_origin_can_instantiate() { + let (code, code_hash) = compile_module::("dummy").unwrap(); + 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); + + 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 ); }); }