Prevent collisions on account creation

There was an issue with contract creation that could overwrite contracts. The solution is to check for a positive nonce or nonempty code hash in the account that is about to be created. If that's the case, the contract creation needs to fail instead of overwriting the account. For more information see ethereum/EIPs#684
mana-ethereum · Aug 15, 2018 · 894d71a · 894d71a
1 parent 0559b20
commit 894d71a
Show file tree

Hide file tree

Showing 6 changed files with 118 additions and 46 deletions.
diff --git a/README.md b/README.md
@@ -94,11 +94,11 @@ Ethereum common tests are created for all clients to test against. We plan to pr
     - [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 2231/2231 = 100% passing
     - [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 2024/2062 = 98.2% passing
 - [x] EIP150
-   - [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 1274/1275 = 99.9% passing
-   - [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1088/1114 = 97.7% passing
+   - [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 1275/1275 = 100% passing
+   - [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1089/1114 = 97.8% passing
 - [x] EIP158
-   - [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 1108/1233 = 89.9%
-   - [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1052/1181= 89.1% passing
+   - [BlockchainTests](https://github.com/ethereum/tests/tree/develop/BlockchainTests) (Includes GeneralStateTests) 1109/1233 = 89.9%
+   - [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1053/1181= 89.2% passing
 - [ ] Byzantium
 - [ ] Constantinople:  View the community [Constantinople Project Tracker](https://github.com/ethereum/pm/issues/53).
 

diff --git a/apps/blockchain/lib/blockchain/account.ex b/apps/blockchain/lib/blockchain/account.ex
@@ -139,6 +139,14 @@ defmodule Blockchain.Account do
     end
   end
 
+  @doc """
+  Checks that an account is neither nil nor empty
+  """
+  @spec exists?(t() | nil) :: boolean()
+  def exists?(account) do
+    !(is_nil(account) || empty?(account))
+  end
+
   @doc """
   Checks if an account is empty.
   """

diff --git a/apps/blockchain/lib/blockchain/contract/create_contract.ex b/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()}

diff --git a/apps/blockchain/test/blockchain/contract/create_contract_test.exs b/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
diff --git a/apps/blockchain/test/blockchain/state_test.exs b/apps/blockchain/test/blockchain/state_test.exs
@@ -22,7 +22,6 @@ defmodule Blockchain.StateTest do
       "stTransactionTest/OverflowGasRequire",
       "stTransactionTest/EmptyTransaction",
       "stTransactionTest/CreateTransactionReverted",
-      "stSystemOperationsTest/CreateHashCollision",
       "stRevertTest/RevertOpcodeWithBigOutputInInit",
       "stRevertTest/RevertOpcodeMultipleSubCalls",
       "stRevertTest/RevertOpcodeInInit",
@@ -70,7 +69,6 @@ defmodule Blockchain.StateTest do
       "stSystemOperationsTest/extcodecopy",
       "stSystemOperationsTest/doubleSelfdestructTest2",
       "stSystemOperationsTest/doubleSelfdestructTest",
-      "stSystemOperationsTest/CreateHashCollision",
       "stSystemOperationsTest/ABAcallsSuicide1",
       "stSystemOperationsTest/ABAcallsSuicide0",
       "stSpecialTest/tx_e1c174e2",

diff --git a/apps/blockchain/test/blockchain_test.exs b/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",
@@ -96,7 +94,6 @@ defmodule BlockchainTest do
       "GeneralStateTests/stSpecialTest/tx_e1c174e2_d0g0v0.json",
       "GeneralStateTests/stSystemOperationsTest/ABAcallsSuicide0_d0g0v0.json",
       "GeneralStateTests/stSystemOperationsTest/ABAcallsSuicide1_d1g0v0.json",
-      "GeneralStateTests/stSystemOperationsTest/CreateHashCollision_d0g0v0.json",
       "GeneralStateTests/stSystemOperationsTest/doubleSelfdestructTest2_d0g0v0.json",
       "GeneralStateTests/stSystemOperationsTest/doubleSelfdestructTest_d0g0v0.json",
       "GeneralStateTests/stSystemOperationsTest/extcodecopy_d0g0v0.json",