Contracts upload with Determinism::Enforced when possible (#3540)

Co-authored-by: Cyrill Leutwiler <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Alexander Theißen <[email protected]>

prdoc/pr_3540.prdoc

@@ -0,0 +1,10 @@
+title: "[pallet-contracts] Only allow non-deterministic code to be uploaded with Determinism::Relaxed"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      The `upload_code` extrinsic, will now only allow non-deterministic code to be uploaded with the `Determinism::Relaxed` flag. 
+      This prevent an attacker from uploading "deterministic" code with the `Determinism::Relaxed` flag, preventing the code to be instantiated for on-chain execution.
+
+crates:
+  - name: pallet-contracts

substrate/frame/contracts/src/benchmarking/code.rs

@@ -26,7 +26,7 @@
 
 use crate::Config;
 use frame_support::traits::Get;
-use sp_runtime::traits::Hash;
+use sp_runtime::{traits::Hash, Saturating};
 use sp_std::{borrow::ToOwned, prelude::*};
 use wasm_instrument::parity_wasm::{
 	builder,
@@ -262,22 +262,25 @@ impl<T: Config> WasmModule<T> {
 	/// `instantiate_with_code` for different sizes of wasm modules. The generated module maximizes
 	/// instrumentation runtime by nesting blocks as deeply as possible given the byte budget.
 	/// `code_location`: Whether to place the code into `deploy` or `call`.
-	pub fn sized(target_bytes: u32, code_location: Location) -> Self {
+	pub fn sized(target_bytes: u32, code_location: Location, use_float: bool) -> Self {
 		use self::elements::Instruction::{End, GetLocal, If, Return};
 		// Base size of a contract is 63 bytes and each expansion adds 6 bytes.
 		// We do one expansion less to account for the code section and function body
 		// size fields inside the binary wasm module representation which are leb128 encoded
 		// and therefore grow in size when the contract grows. We are not allowed to overshoot
 		// because of the maximum code size that is enforced by `instantiate_with_code`.
-		let expansions = (target_bytes.saturating_sub(63) / 6).saturating_sub(1);
+		let mut expansions = (target_bytes.saturating_sub(63) / 6).saturating_sub(1);
 		const EXPANSION: [Instruction; 4] = [GetLocal(0), If(BlockType::NoResult), Return, End];
+		let mut locals = vec![Local::new(1, ValueType::I32)];
+		if use_float {
+			locals.push(Local::new(1, ValueType::F32));
+			locals.push(Local::new(2, ValueType::F32));
+			locals.push(Local::new(3, ValueType::F32));
+			expansions.saturating_dec();
+		}
 		let mut module =
 			ModuleDefinition { memory: Some(ImportedMemory::max::<T>()), ..Default::default() };
-		let body = Some(body::repeated_with_locals(
-			&[Local::new(1, ValueType::I32)],
-			expansions,
-			&EXPANSION,
-		));
+		let body = Some(body::repeated_with_locals(&locals, expansions, &EXPANSION));
 		match code_location {
 			Location::Call => module.call_body = body,
 			Location::Deploy => module.deploy_body = body,

substrate/frame/contracts/src/benchmarking/mod.rs

@@ -362,7 +362,7 @@ benchmarks! {
 	call_with_code_per_byte {
 		let c in 0 .. T::MaxCodeLen::get();
 		let instance = Contract::<T>::with_caller(
-			whitelisted_caller(), WasmModule::sized(c, Location::Deploy), vec![],
+			whitelisted_caller(), WasmModule::sized(c, Location::Deploy, false), vec![],
 		)?;
 		let value = Pallet::<T>::min_balance();
 		let origin = RawOrigin::Signed(instance.caller.clone());
@@ -389,7 +389,7 @@ benchmarks! {
 		let value = Pallet::<T>::min_balance();
 		let caller = whitelisted_caller();
 		T::Currency::set_balance(&caller, caller_funding::<T>());
-		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c, Location::Call);
+		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c, Location::Call, false);
 		let origin = RawOrigin::Signed(caller.clone());
 		let addr = Contracts::<T>::contract_address(&caller, &hash, &input, &salt);
 	}: _(origin, value, Weight::MAX, None, code, input, salt)
@@ -468,19 +468,36 @@ benchmarks! {
 	// It creates a maximum number of metering blocks per byte.
 	// `c`: Size of the code in bytes.
 	#[pov_mode = Measured]
-	upload_code {
+	upload_code_determinism_enforced {
 		let c in 0 .. T::MaxCodeLen::get();
 		let caller = whitelisted_caller();
 		T::Currency::set_balance(&caller, caller_funding::<T>());
-		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c, Location::Call);
+		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c, Location::Call, false);
 		let origin = RawOrigin::Signed(caller.clone());
-	}: _(origin, code, None, Determinism::Enforced)
+	}: upload_code(origin, code, None, Determinism::Enforced)
 	verify {
 		// uploading the code reserves some balance in the callers account
 		assert!(T::Currency::total_balance_on_hold(&caller) > 0u32.into());
 		assert!(<Contract<T>>::code_exists(&hash));
 	}
 
+	// Uploading code with [`Determinism::Relaxed`] should be more expensive than uploading code with [`Determinism::Enforced`],
+	// as we always try to save the code with [`Determinism::Enforced`] first.
+	#[pov_mode = Measured]
+	upload_code_determinism_relaxed {
+		let c in 0 .. T::MaxCodeLen::get();
+		let caller = whitelisted_caller();
+		T::Currency::set_balance(&caller, caller_funding::<T>());
+		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c, Location::Call, true);
+		let origin = RawOrigin::Signed(caller.clone());
+	}: upload_code(origin, code, None, Determinism::Relaxed)
+	verify {
+		assert!(T::Currency::total_balance_on_hold(&caller) > 0u32.into());
+		assert!(<Contract<T>>::code_exists(&hash));
+		// Ensure that the benchmark follows the most expensive path, i.e., the code is saved with [`Determinism::Relaxed`] after trying to save it with [`Determinism::Enforced`].
+		assert_eq!(CodeInfoOf::<T>::get(&hash).unwrap().determinism(), Determinism::Relaxed);
+	}
+
 	// Removing code does not depend on the size of the contract because all the information
 	// needed to verify the removal claim (refcount, owner) is stored in a separate storage
 	// item (`CodeInfoOf`).

substrate/frame/contracts/src/lib.rs

@@ -645,8 +645,17 @@ pub mod pallet {
 		/// To avoid this situation a constructor could employ access control so that it can
 		/// only be instantiated by permissioned entities. The same is true when uploading
 		/// through [`Self::instantiate_with_code`].
+		///
+		/// Use [`Determinism::Relaxed`] exclusively for non-deterministic code. If the uploaded
+		/// code is deterministic, specifying [`Determinism::Relaxed`] will be disregarded and
+		/// result in higher gas costs.
 		#[pallet::call_index(3)]
-		#[pallet::weight(T::WeightInfo::upload_code(code.len() as u32))]
+		#[pallet::weight(
+			match determinism {
+				Determinism::Enforced => T::WeightInfo::upload_code_determinism_enforced(code.len() as u32),
+				Determinism::Relaxed => T::WeightInfo::upload_code_determinism_relaxed(code.len() as u32),
+			}
+		)]
 		pub fn upload_code(
 			origin: OriginFor<T>,
 			code: Vec<u8>,

substrate/frame/contracts/src/tests.rs

@@ -5185,6 +5185,26 @@ fn deposit_limit_honors_min_leftover() {
 	});
 }
 
+#[test]
+fn upload_should_enforce_deterministic_mode_when_possible() {
+	let upload = |fixture, determinism| {
+		let (wasm, code_hash) = compile_module::<Test>(fixture).unwrap();
+		ExtBuilder::default()
+			.build()
+			.execute_with(|| -> Result<Determinism, DispatchError> {
+				let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+				Contracts::bare_upload_code(ALICE, wasm, None, determinism)?;
+				let info = CodeInfoOf::<Test>::get(code_hash).unwrap();
+				Ok(info.determinism())
+			})
+	};
+
+	assert_eq!(upload("dummy", Determinism::Enforced), Ok(Determinism::Enforced));
+	assert_eq!(upload("dummy", Determinism::Relaxed), Ok(Determinism::Enforced));
+	assert_eq!(upload("float_instruction", Determinism::Relaxed), Ok(Determinism::Relaxed));
+	assert!(upload("float_instruction", Determinism::Enforced).is_err());
+}
+
 #[test]
 fn cannot_instantiate_indeterministic_code() {
 	let (wasm, code_hash) = compile_module::<Test>("float_instruction").unwrap();

substrate/frame/contracts/src/wasm/mod.rs

@@ -313,6 +313,11 @@ impl<T: Config> CodeInfo<T> {
 		}
 	}
 
+	/// Returns the determinism of the module.
+	pub fn determinism(&self) -> Determinism {
+		self.determinism
+	}
+
 	/// Returns reference count of the module.
 	pub fn refcount(&self) -> u64 {
 		self.refcount

substrate/frame/contracts/src/wasm/prepare.rs

@@ -79,7 +79,10 @@ impl LoadedModule {
 		}
 
 		let engine = Engine::new(&config);
-		let module = Module::new(&engine, code).map_err(|_| "Can't load the module into wasmi!")?;
+		let module = Module::new(&engine, code).map_err(|err| {
+			log::debug!(target: LOG_TARGET, "Module creation failed: {:?}", err);
+			"Can't load the module into wasmi!"
+		})?;
 
 		// Return a `LoadedModule` instance with
 		// __valid__ module.
@@ -220,7 +223,7 @@ impl LoadedModule {
 fn validate<E, T>(
 	code: &[u8],
 	schedule: &Schedule<T>,
-	determinism: Determinism,
+	determinism: &mut Determinism,
 ) -> Result<(), (DispatchError, &'static str)>
 where
 	E: Environment<()>,
@@ -229,7 +232,17 @@ where
 	(|| {
 		// We check that the module is generally valid,
 		// and does not have restricted WebAssembly features, here.
-		let contract_module = LoadedModule::new::<T>(code, determinism, None)?;
+		let contract_module = match *determinism {
+			Determinism::Relaxed =>
+				if let Ok(module) = LoadedModule::new::<T>(code, Determinism::Enforced, None) {
+					*determinism = Determinism::Enforced;
+					module
+				} else {
+					LoadedModule::new::<T>(code, Determinism::Relaxed, None)?
+				},
+			Determinism::Enforced => LoadedModule::new::<T>(code, Determinism::Enforced, None)?,
+		};
+
 		// The we check that module satisfies constraints the pallet puts on contracts.
 		contract_module.scan_exports()?;
 		contract_module.scan_imports::<T>(schedule)?;
@@ -252,7 +265,7 @@ where
 		&code,
 		(),
 		schedule,
-		determinism,
+		*determinism,
 		stack_limits,
 		AllowDeprecatedInterface::No,
 	)
@@ -276,13 +289,13 @@ pub fn prepare<E, T>(
 	code: CodeVec<T>,
 	schedule: &Schedule<T>,
 	owner: AccountIdOf<T>,
-	determinism: Determinism,
+	mut determinism: Determinism,
 ) -> Result<WasmBlob<T>, (DispatchError, &'static str)>
 where
 	E: Environment<()>,
 	T: Config,
 {
-	validate::<E, T>(code.as_ref(), schedule, determinism)?;
+	validate::<E, T>(code.as_ref(), schedule, &mut determinism)?;
 
 	// Calculate deposit for storing contract code and `code_info` in two different storage items.
 	let code_len = code.len() as u32;