precompiles: Enforce state mutability (#10080)

`pallet-assets-precompile`, `pallet-xcm-precompiles` and revive builtin
precompile implementations currently violate [Solidity state
mutability](https://docs.soliditylang.org/en/latest/grammar.html#syntax-rule-SolidityParser.stateMutability),
potentially introducing a new attack vector. This PR implements
corresponding checks at the function dispatch.

Could be enforced in `pallet-revive`, however:
1. Adding something like a `const MUTATES: bool` to the `Precompile`
trait won't help because whether the call is mutating or not depends on
the [Solidity function
selector.](https://docs.soliditylang.org/en/latest/abi-spec.html#function-selector).
2. Alloy, which we are using to parse the interface definitions prior to
calling precompile implementations, doesn't provide a mapping from
function selector to its mutability
[modifier](https://docs.soliditylang.org/en/latest/cheatsheet.html#modifiers).

---------

Signed-off-by: Cyrill Leutwiler <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: xermicus

polkadot/xcm/pallet-xcm/precompiles/src/lib.rs

@@ -82,6 +82,8 @@ where
 		};
 
 		match input {
+			IXcmCalls::send(_) | IXcmCalls::execute(_) if env.is_read_only() =>
+				Err(Error::Error(pallet_revive::Error::<Self::T>::StateChangeDenied.into())),
 			IXcmCalls::send(IXcm::sendCall { destination, message }) => {
 				let _ = env.charge(<Runtime as Config>::WeightInfo::send())?;
 

prdoc/pr_10080.prdoc

@@ -0,0 +1,16 @@
+title: 'precompiles: Enforce state mutability'
+doc:
+- audience: Runtime Dev
+  description: |-
+    `pallet-assets-precompile`, `pallet-xcm-precompiles` and revive builtin precompile implementations currently violate [Solidity state mutability](https://docs.soliditylang.org/en/latest/grammar.html#syntax-rule-SolidityParser.stateMutability), potentially introducing a new attack vector. This PR implements corresponding checks at the function dispatch.
+
+    Could be enforced in `pallet-revive`, however:
+    1. Adding something like a `const MUTATES: bool` to the `Precompile` trait won't help because whether the call is mutating or not depends on the [Solidity function selector.](https://docs.soliditylang.org/en/latest/abi-spec.html#function-selector).
+    2. Alloy, which we are using to parse the interface definitions prior to calling precompile implementations, doesn't provide a mapping from function selector to its mutability [modifier](https://docs.soliditylang.org/en/latest/cheatsheet.html#modifiers).
+crates:
+- name: pallet-assets-precompiles
+  bump: patch
+- name: pallet-xcm-precompiles
+  bump: patch
+- name: pallet-revive
+  bump: patch

substrate/frame/assets/precompiles/src/lib.rs

@@ -108,6 +108,10 @@ where
 		let asset_id = PrecompileConfig::AssetIdExtractor::asset_id_from_address(address)?.into();
 
 		match input {
+			IERC20Calls::transfer(_) | IERC20Calls::approve(_) | IERC20Calls::transferFrom(_)
+				if env.is_read_only() =>
+				Err(Error::Error(pallet_revive::Error::<Self::T>::StateChangeDenied.into())),
+
 			IERC20Calls::transfer(call) => Self::transfer(asset_id, call, env),
 			IERC20Calls::totalSupply(_) => Self::total_supply(asset_id, env),
 			IERC20Calls::balanceOf(call) => Self::balance_of(asset_id, call, env),

substrate/frame/revive/src/precompiles/builtin/storage.rs

@@ -54,6 +54,10 @@ impl<T: Config> BuiltinPrecompile for Storage<T> {
 		use IStorage::IStorageCalls;
 		let max_size = limits::STORAGE_BYTES;
 		match input {
+			IStorageCalls::clearStorage(_) | IStorageCalls::takeStorage(_)
+				if env.is_read_only() =>
+				Err(Error::Error(crate::Error::<Self::T>::StateChangeDenied.into())),
+
 			IStorageCalls::clearStorage(IStorage::clearStorageCall { flags, key, isFixedKey }) => {
 				let transient = is_transient(*flags)
 					.map_err(|_| Error::Revert("invalid storage flag".into()))?;