Prevent collisions on account creation

There was an issue with contract creation that would overwrite
contracts. The solution is to check for a positive nonce or nonempty
code hash in an account. If that's the case, the contract creation needs
to fail instead of overwriting the account.

For more information see ethereum/EIPs#684

apps/blockchain/lib/blockchain/account.ex

@@ -139,6 +139,14 @@ defmodule Blockchain.Account do
     end
   end
 
+  @doc """
+  Checks whether an account is nil or empty
+  """
+  @spec exists?(t() | nil) :: boolean()
+  def exists?(account) do
+    !(is_nil(account) || empty?(account))
+  end
+
   @doc """
   Checks if an account is empty.
   """

apps/blockchain/lib/blockchain/contract/create_contract.ex

@@ -42,22 +42,23 @@ defmodule Blockchain.Contract.CreateContract do
           config: EVM.Configuration.t()
         }
 
-  # TODO: Block header? "I_H has no special treatment and is determined from the blockchain"
   @spec execute(t()) :: {EVM.state(), EVM.Gas.t(), EVM.SubState.t()}
   def execute(params) do
     sender_account = Account.get_account(params.state, params.sender)
     contract_address = Address.new(params.sender, sender_account.nonce)
+    account = Account.get_account(params.state, contract_address)
 
-    if account_exists?(params, contract_address) do
-      account = Account.get_account(params.state, contract_address)
+    if Account.exists?(account) do
+      cond do
+        account_will_collide?(account) ->
+          error(params)
 
-      # params.stack_depth != 0 means that we're not in contract creation transaction
-      # because `create` evm instruction should have parameters on the stack that are pushed to it so
-      # it never is zero
-      if account.nonce == 0 && Account.is_simple_account?(account) && params.stack_depth != 0 do
-        {:ok, {params.state, params.available_gas, SubState.empty()}}
-      else
-        {:ok, {params.state, 0, SubState.empty()}}
+        account.nonce == 0 && Account.is_simple_account?(account) &&
+            not_in_contract_create_transaction?(params) ->
+          {:ok, {params.state, params.available_gas, SubState.empty()}}
+
+        true ->
+          {:ok, {params.state, 0, SubState.empty()}}
       end
     else
       result = {_, _, _, output} = create(params, contract_address)
@@ -68,19 +69,31 @@ defmodule Blockchain.Contract.CreateContract do
       # valid jump destination or invalid instruction), then no gas
       # is refunded to the caller and the state is reverted to the
       # point immediately prior to balance transfer.
+      #
       if output == :failed do
-        {:error, {params.state, 0, SubState.empty()}}
+        error(params)
       else
         finalize(result, params, contract_address)
       end
     end
   end
 
-  @spec account_exists?(t(), EVM.address()) :: boolean()
-  defp account_exists?(params, address) do
-    account = Account.get_account(params.state, address)
+  @spec not_in_contract_create_transaction?(t) :: boolean()
+  defp not_in_contract_create_transaction?(params) do
+    # params.stack_depth != 0 means that we're not in contract creation transaction
+    # because `create` evm instruction should have parameters on the stack that are pushed to it so
+    # it never is zero
+    params.stack_depth != 0
+  end
+
+  @spec account_will_collide?(Account.t()) :: boolean()
+  defp account_will_collide?(account) do
+    account.nonce > 0 || !Account.is_simple_account?(account)
+  end
 
-    !(is_nil(account) || Account.empty?(account))
+  @spec error(t) :: {:error, EVM.state(), 0, SubState.t()}
+  defp error(params) do
+    {:error, {params.state, 0, SubState.empty()}}
   end
 
   @spec create(t(), EVM.address()) :: {EVM.state(), EVM.Gas.t(), EVM.SubState.t()}

apps/blockchain/test/blockchain/contract/create_contract_test.exs

@@ -6,8 +6,6 @@ defmodule Blockchain.Contract.CreateContractTest do
   alias EVM.{SubState, MachineCode}
   alias MerklePatriciaTree.{Trie, DB}
 
-  # TODO: Add rich tests for contract creation
-
   setup do
     db = MerklePatriciaTree.Test.random_ets_db(:contract_test)
     {:ok, %{db: db}}
@@ -17,24 +15,6 @@ defmodule Blockchain.Contract.CreateContractTest do
     test "creates a new contract", %{db: db} do
       account = %Account{balance: 11, nonce: 5}
 
-      assembly = [
-        :push1,
-        3,
-        :push1,
-        5,
-        :add,
-        :push1,
-        0x00,
-        :mstore,
-        :push1,
-        32,
-        :push1,
-        0,
-        :return
-      ]
-
-      init_code = MachineCode.compile(assembly)
-
       state =
         db
         |> Trie.new()
@@ -47,7 +27,7 @@ defmodule Blockchain.Contract.CreateContractTest do
         available_gas: 100_000_000,
         gas_price: 1,
         endowment: 5,
-        init_code: init_code,
+        init_code: build_sample_code(),
         stack_depth: 5,
         block_header: %Block.Header{nonce: 1}
       }
@@ -82,5 +62,81 @@ defmodule Blockchain.Contract.CreateContractTest do
       assert Account.get_machine_code(state, contract_address) == {:ok, <<0x08::256>>}
       assert state |> Trie.Inspector.all_keys() |> Enum.count() == 2
     end
+
+    test "does not create contract if address already exists with nonce", %{db: db} do
+      account = %Account{balance: 11, nonce: 5}
+      account_address = <<0x10::160>>
+      collision_account = %Account{nonce: 1}
+      collision_account_address = Contract.Address.new(account_address, account.nonce)
+
+      state =
+        db
+        |> Trie.new()
+        |> Account.put_account(account_address, account)
+        |> Account.put_account(collision_account_address, collision_account)
+
+      params = %Contract.CreateContract{
+        state: state,
+        sender: account_address,
+        originator: account_address,
+        available_gas: 100_000_000,
+        gas_price: 1,
+        endowment: 5,
+        init_code: build_sample_code(),
+        stack_depth: 5,
+        block_header: %Block.Header{nonce: 1}
+      }
+
+      assert {:error, {^state, 0, sub_state}} = Contract.create(params)
+      assert SubState.empty?(sub_state)
+    end
+
+    test "does not create contract if address has code", %{db: db} do
+      account = %Account{balance: 11, nonce: 5}
+      account_address = <<0x10::160>>
+      collision_account = %Account{code_hash: build_sample_code(), nonce: 0}
+      collision_account_address = Contract.Address.new(account_address, account.nonce)
+
+      state =
+        db
+        |> Trie.new()
+        |> Account.put_account(account_address, account)
+        |> Account.put_account(collision_account_address, collision_account)
+
+      params = %Contract.CreateContract{
+        state: state,
+        sender: account_address,
+        originator: account_address,
+        available_gas: 100_000_000,
+        gas_price: 1,
+        endowment: 5,
+        init_code: build_sample_code(),
+        stack_depth: 5,
+        block_header: %Block.Header{nonce: 1}
+      }
+
+      assert {:error, {^state, 0, sub_state}} = Contract.create(params)
+      assert SubState.empty?(sub_state)
+    end
+  end
+
+  defp build_sample_code do
+    assembly = [
+      :push1,
+      3,
+      :push1,
+      5,
+      :add,
+      :push1,
+      0x00,
+      :mstore,
+      :push1,
+      32,
+      :push1,
+      0,
+      :return
+    ]
+
+    MachineCode.compile(assembly)
   end
 end

apps/blockchain/test/blockchain_test.exs

@@ -14,9 +14,7 @@ defmodule BlockchainTest do
   @failing_tests %{
     "Frontier" => [],
     "Homestead" => [],
-    "EIP150" => [
-      "GeneralStateTests/stSystemOperationsTest/CreateHashCollision_d0g0v0.json"
-    ],
+    "EIP150" => [],
     "EIP158" => [
       "GeneralStateTests/stAttackTest/ContractCreationSpam_d0g0v0.json",
       "GeneralStateTests/stAttackTest/CrashingTransaction_d0g0v0.json",