From fac0f141f4e66e88f2d54725041ec72390b0bc85 Mon Sep 17 00:00:00 2001 From: Damian Ramirez Date: Wed, 18 Dec 2024 16:40:09 -0300 Subject: [PATCH] fix(levm): create to an existing address (#1523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Motivation** The `CREATE` transaction logic did not handle the case where the address where we the contract will be deploy already exists. **Description** - Add a check in `VM::new()` where the transaction is `CREATE`. If `new_contract_address` exists in the db and it has nonce or bytecode, we don't create the new account and we don't add it in the cache. We know this is an error and the revert will be handled in `transact()` - Something similar in `transact()`, we check if the transaction is `CREATE` and if `new_address_acc` has code or nonce. In that case we return a `TransactionReport` with `TxResult::Revert`. - Support the creation of a contract even if the account has balance. Related #1517 --------- Co-authored-by: Tomas Fabrizio Orsi Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com> Co-authored-by: JereSalo --- crates/vm/levm/src/opcode_handlers/system.rs | 12 ++++- crates/vm/levm/src/vm.rs | 53 ++++++++++++++++---- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/crates/vm/levm/src/opcode_handlers/system.rs b/crates/vm/levm/src/opcode_handlers/system.rs index 2cb3700a3..a16effa0e 100644 --- a/crates/vm/levm/src/opcode_handlers/system.rs +++ b/crates/vm/levm/src/opcode_handlers/system.rs @@ -528,14 +528,22 @@ impl VM { }; // 3. Account has nonce or code. - if self.get_account(new_address).has_code_or_nonce() { + let new_account = self.get_account(new_address); + if new_account.has_code_or_nonce() { + self.increment_account_nonce(deployer_address)?; current_call_frame.stack.push(CREATE_DEPLOYMENT_FAIL)?; return Ok(OpcodeSuccess::Continue); } // THIRD: Changes to the state // 1. Creating contract. - let new_account = Account::new(value_in_wei_to_send, Bytes::new(), 1, Default::default()); + + // If the address has balance but there is no account associated with it, we need to add the value to it + let new_balance = value_in_wei_to_send + .checked_add(new_account.info.balance) + .ok_or(VMError::BalanceOverflow)?; + + let new_account = Account::new(new_balance, Bytes::new(), 1, Default::default()); cache::insert_account(&mut self.cache, new_address, new_account); // 2. Increment sender's nonce. diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index d33388af2..1e0e65e24 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -209,9 +209,17 @@ impl VM { default_touched_accounts.insert(new_contract_address); - let created_contract = Account::new(value, Bytes::new(), 1, HashMap::new()); - - cache::insert_account(&mut cache, new_contract_address, created_contract); + // Since we are in a CREATE transaction, we need to check if the address is already occupied. + // If it is, we should not continue with the transaction. We will handle the revert in the next step. + let new_account = db.get_account_info(new_contract_address); + let balance = value + .checked_add(new_account.balance) + .ok_or(VMError::BalanceOverflow)?; + + if !new_account.has_code() && !new_account.has_nonce() { + let created_contract = Account::new(balance, Bytes::new(), 1, HashMap::new()); + cache::insert_account(&mut cache, new_contract_address, created_contract); + } let initial_call_frame = CallFrame::new( env.origin, @@ -616,14 +624,6 @@ impl VM { /// - It calculates and adds intrinsic gas to the 'gas used' of callframe and environment. /// See 'docs' for more information about validations. fn prepare_execution(&mut self, initial_call_frame: &mut CallFrame) -> Result<(), VMError> { - //TODO: This should revert the transaction, not throw an error. And I don't know if it should be done here... - // if self.is_create() { - // // If address is already in db, there's an error - // let new_address_acc = self.db.get_account_info(call_frame.to); - // if !new_address_acc.is_empty() { - // return Err(VMError::AddressAlreadyOccupied); - // } - // } let sender_address = self.env.origin; let sender_account = self.get_account(sender_address); @@ -880,6 +880,17 @@ impl VM { self.prepare_execution(&mut initial_call_frame)?; + // The transaction should be reverted if: + // - The transaction is a CREATE transaction and + // - The address is already in the database and + // - The address is not empty + if self.is_create() { + let new_address_acc = self.db.get_account_info(initial_call_frame.to); + if new_address_acc.has_code() || new_address_acc.has_nonce() { + return self.handle_create_non_empty_account(&initial_call_frame); + } + } + let mut report = self.execute(&mut initial_call_frame)?; if self.is_create() && !report.is_success() { remove_account(&mut self.cache, &initial_call_frame.to); @@ -1142,6 +1153,26 @@ impl VM { } } } + + fn handle_create_non_empty_account( + &mut self, + initial_call_frame: &CallFrame, + ) -> Result { + let mut report = TransactionReport { + result: TxResult::Revert(VMError::AddressAlreadyOccupied), + gas_used: self.env.gas_limit.low_u64(), + gas_refunded: 0, + logs: vec![], + new_state: self.cache.clone(), + output: Bytes::new(), + created_address: None, + }; + + self.post_execution_changes(initial_call_frame, &mut report)?; + report.new_state.clone_from(&self.cache); + + Ok(report) + } } fn get_n_value(op: Opcode, base_opcode: Opcode) -> Result {