CVE-2023-45130: Allocation of Resources Without Limits or Throttling (CWE-770)
value: Frontier is Substrate's Ethereum compatibility layer. Prior to commit aea528198b3b226e0d20cce878551fd4c0e3d5d0, at the end of a contract execution, when opcode SUICIDE marks a contract to be deleted, the software uses `storage::remove_prefix` (now renamed to `storage::clear_prefix`) to remove all storages associated with it. This is a single IO primitive call passing the WebAssembly boundary. For large contracts, the call (without providing a `limit` parameter) can be slow. In addition, for parachains, all storages to be deleted will be part of the PoV, which easily exceed relay chain PoV size limit. On the other hand, Frontier's maintainers only charge a fixed cost for opcode SUICIDE. The maintainers consider the severity of this issue high, because an attacker can craft a contract with a lot of storage values on a parachain, and then call opcode SUICIDE on the contract. If the transaction makes into a parachain block, the parachain will then stall because the PoV size will exceed relay chain's limit. This is especially an issue for XCM transactions, because they can't be skipped. Commit aea528198b3b226e0d20cce878551fd4c0e3d5d0 contains a patch for this issue. For parachains, it's recommended to issue an emergency runtime upgrade as soon as possible. For standalone chains, the impact is less severe because the issue mainly affects PoV sizes. It's recommended to issue a normal runtime upgrade as soon as possible. There are no known workarounds.
CWE description: The product allocates a reusable resource or group of resources on behalf of an actor without imposing any restrictions on the size or number of resources that can be allocated, in violation of the intended security policy for that actor.
CVSS v3 base score: 7.5
Published: 2023-10-13T13:15Z
Repository: https://github.com/paritytech/frontier
Language: Rust
Files changed: frame/evm/src/lib.rs, frame/evm/src/tests.rs
Commit date: 2023-10-05 10:19:39 +0200
Commit message: Initial support for asynchronous suiciding (#1212)

* Initial support for asynchronous suiciding

* Use contains_key instead of is_some

Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>

* Fix compile

* Remove unnecessary Suicided check

* Don't call dec_sufficients

* Handle pre-EIP161 case

* Add extra safeguard

* Always inc nonce

* Fix tests

---------

Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>
Diff: diff --git a/frame/evm/src/lib.rs b/frame/evm/src/lib.rs
index c4fdc0f1..012d5e79 100644
--- a/frame/evm/src/lib.rs
+++ b/frame/evm/src/lib.rs
@@ -559,10 +559,13 @@ pub mod pallet {
 		StorageMap<_, Blake2_128Concat, H160, CodeMetadata, OptionQuery>;
 
 	#[pallet::storage]
 	pub type AccountStorages<T: Config> =
 		StorageDoubleMap<_, Blake2_128Concat, H160, Blake2_128Concat, H256, H256, ValueQuery>;
+
+	#[pallet::storage]
+	pub type Suicided<T: Config> = StorageMap<_, Blake2_128Concat, H160, (), OptionQuery>;
 }
 
 /// Type alias for currency balance.
 pub type BalanceOf<T> =
 	<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
@@ -791,22 +794,33 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// Remove an account.
 	pub fn remove_account(address: &H160) {
 		if <AccountCodes<T>>::contains_key(address) {
+			// Remember to call `dec_sufficients` when clearing Suicided.
+			<Suicided<T>>::insert(address, ());
+
+			// In theory, we can always have pre-EIP161 contracts, so we
+			// make sure the account nonce is at least one.
 			let account_id = T::AddressMapping::into_account_id(*address);
-			let _ = frame_system::Pallet::<T>::dec_sufficients(&account_id);
+			frame_system::Pallet::<T>::inc_account_nonce(&account_id);
 		}
 
 		<AccountCodes<T>>::remove(address);
 		<AccountCodesMetadata<T>>::remove(address);
-		#[allow(deprecated)]
-		let _ = <AccountStorages<T>>::remove_prefix(address, None);
 	}
 
 	/// Create an account.
 	pub fn create_account(address: H160, code: Vec<u8>) {
+		if <Suicided<T>>::contains_key(address) {
+			// This branch should never trigger, because when Suicided
+			// contains an address, then its nonce will be at least one,
+			// which causes CreateCollision error in EVM, but we add it
+			// here for safeguard.
+			return;
+		}
+
 		if code.is_empty() {
 			return;
 		}
 
 		if !<AccountCodes<T>>::contains_key(address) {
diff --git a/frame/evm/src/tests.rs b/frame/evm/src/tests.rs
index ffa7bdbe..6c4c294b 100644
--- a/frame/evm/src/tests.rs
+++ b/frame/evm/src/tests.rs
@@ -1035,12 +1035,11 @@ fn handle_sufficient_reference() {
 		let account_2 = frame_system::Account::<Test>::get(substrate_addr_2);
 		// We increased the sufficient reference by 1.
 		assert_eq!(account_2.sufficients, 1);
 		EVM::remove_account(&addr_2);
 		let account_2 = frame_system::Account::<Test>::get(substrate_addr_2);
-		// We decreased the sufficient reference by 1 on removing the account.
-		assert_eq!(account_2.sufficients, 0);
+		assert_eq!(account_2.sufficients, 1);
 	});
 }
 
 #[test]
 fn runner_non_transactional_calls_with_non_balance_accounts_is_ok_without_gas_price() {

Vulnerable code: 			let _ = frame_system::Pallet::<T>::dec_sufficients(&account_id);
		#[allow(deprecated)]
		let _ = <AccountStorages<T>>::remove_prefix(address, None);
		// We decreased the sufficient reference by 1 on removing the account.
		assert_eq!(account_2.sufficients, 0);
Fixed code: 
	#[pallet::storage]
	pub type Suicided<T: Config> = StorageMap<_, Blake2_128Concat, H160, (), OptionQuery>;
			// Remember to call `dec_sufficients` when clearing Suicided.
			<Suicided<T>>::insert(address, ());

			// In theory, we can always have pre-EIP161 contracts, so we
			// make sure the account nonce is at least one.
			frame_system::Pallet::<T>::inc_account_nonce(&account_id);
		if <Suicided<T>>::contains_key(address) {
			// This branch should never trigger, because when Suicided
			// contains an address, then its nonce will be at least one,
			// which causes CreateCollision error in EVM, but we add it
			// here for safeguard.
			return;
		}

		assert_eq!(account_2.sufficients, 1);