Do not overwrite account balances when creating contracts (#437)

Transaction [0x9aac76c9ed191da631b5e70c8824f2ad106a0f5147d57c611ed64653734dd85f](https://etherscan.io/tx/0x9aac76c9ed191da631b5e70c8824f2ad106a0f5147d57c611ed64653734dd85f) was failing consensus at [block 243826](https://etherscan.io/block/243826) because it was a `Contract Create` to an account that already had a balance. Before creating a contract we make sure that contract doesn't already exist. Before this commit we were using using `Account.exists?` for that check. `Account.exists?` checks the balance of an account as well as whether it's contract code is set. This was incorrect. Instead now use `Account.is_simple_account?` which just checks if an account has it's contract code set. This allows for a sneaky trick called hiding ether described [here](http://swende.se/blog/Ethereum_quirks_and_vulns.html) as `Quirk #1 - hiding in plain sight`.
mana-ethereum · Sep 25, 2018 · bf7f7a4 · bf7f7a4
1 parent 59cf2ea
commit bf7f7a4
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 48 deletions.
diff --git a/apps/blockchain/lib/blockchain/account.ex b/apps/blockchain/lib/blockchain/account.ex
@@ -139,11 +139,16 @@ defmodule Blockchain.Account do
   end
 
   @doc """
-  Checks that an account is neither nil nor empty
+  Checks for an empty code hash and a zero nonce. This is the equivalent of
+  any empty account check regardless of whether the account has a balance.
+  This check is needed when an account hides money in one of it's contract
+  addresses and later creates a contract to get the money out. This technique
+  is documented as `Quirk #1 - hiding in plain sight` in the [Ethereum quirks and vulns](http://swende.se/blog/Ethereum_quirks_and_vulns.html)
+  blog post.
   """
-  @spec exists?(t() | nil) :: boolean()
-  def exists?(account) do
-    !(is_nil(account) || empty?(account))
+  @spec uninitialized_contract?(t()) :: boolean()
+  def uninitialized_contract?(account) do
+    account.nonce == 0 && account.code_hash == @empty_keccak
   end
 
   @doc """

diff --git a/apps/blockchain/lib/blockchain/contract/create_contract.ex b/apps/blockchain/lib/blockchain/contract/create_contract.ex
@@ -53,26 +53,7 @@ defmodule Blockchain.Contract.CreateContract do
     contract_address = new_account_address(params)
     account = Account.get_account(original_account_interface.state, contract_address)
 
-    if Account.exists?(account) do
-      cond do
-        account_will_collide?(account) ->
-          error(original_account_interface)
-
-        account.nonce == 0 && Account.is_simple_account?(account) &&
-            not_in_contract_create_transaction?(params) ->
-          new_state =
-            increment_nonce_of_touched_account(
-              original_account_interface.state,
-              params.config,
-              contract_address
-            )
-
-          {:ok, {AccountInterface.new(new_state), params.available_gas, SubState.empty()}}
-
-        true ->
-          {:ok, {original_account_interface, 0, SubState.empty()}}
-      end
-    else
+    if is_nil(account) || Account.uninitialized_contract?(account) do
       result = {rem_gas, _, _, output} = create(params, contract_address)
 
       # From the Yellow Paper:
@@ -87,6 +68,12 @@ defmodule Blockchain.Contract.CreateContract do
         {:revert, _} -> {:error, {original_account_interface, rem_gas, SubState.empty()}}
         _ -> finalize(result, params, contract_address)
       end
+    else
+      if account_will_collide?(account) do
+        error(original_account_interface)
+      else
+        {:ok, {original_account_interface, 0, SubState.empty()}}
+      end
     end
   end
 
@@ -100,14 +87,6 @@ defmodule Blockchain.Contract.CreateContract do
     end
   end
 
-  @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)
@@ -122,7 +101,7 @@ defmodule Blockchain.Contract.CreateContract do
   defp create(params, address) do
     state_with_blank_contract =
       params
-      |> init_blank_account(address)
+      |> init_account(address)
       |> increment_nonce_of_touched_account(params.config, address)
 
     account_interface =
@@ -148,10 +127,19 @@ defmodule Blockchain.Contract.CreateContract do
     EVM.VM.run(params.available_gas, exec_env)
   end
 
-  @spec init_blank_account(t, EVM.address()) :: EVM.state()
-  defp init_blank_account(params, address) do
-    params.account_interface.state
-    |> Account.put_account(address, %Account{nonce: 0})
+  @spec init_account(t, EVM.address()) :: EVM.state()
+  defp init_account(params, address) do
+    account = Account.get_account(params.account_interface.state, address)
+
+    state =
+      if is_nil(account) do
+        params.account_interface.state
+        |> Account.put_account(address, %Account{})
+      else
+        params.account_interface.state
+      end
+
+    state
     |> Account.transfer!(params.sender, address, params.endowment)
   end
 

diff --git a/apps/blockchain/scripts/sync_with_infura.ex b/apps/blockchain/scripts/sync_with_infura.ex
@@ -12,14 +12,16 @@ defmodule SyncWithInfura do
 
   def add_block_to_tree(db, chain, tree, n) do
     next_block = get_block(n)
+
     case Blockchain.Blocktree.verify_and_add_block(tree, chain, next_block, db) do
       {:ok, next_tree} ->
         Logger.info("Verified Block #{n}")
         MerklePatriciaTree.DB.put!(db, "current_block_hash", Blockchain.Block.hash(next_block))
         add_block_to_tree(db, chain, next_tree, n + 1)
+
       {:invalid, error} ->
         Logger.info("Failed to Verify Block #{n}")
-        Logger.error(inspect error)
+        Logger.error(inspect(error))
         MerklePatriciaTree.DB.put!(db, "current_block_tree", :erlang.term_to_binary(tree))
     end
   end
@@ -142,7 +144,9 @@ tree =
   case MerklePatriciaTree.DB.get(db, "current_block_tree") do
     {:ok, current_block_tree} ->
       :erlang.binary_to_term(current_block_tree)
-    _ -> Blockchain.Blocktree.new_tree()
+
+    _ ->
+      Blockchain.Blocktree.new_tree()
   end
 
 SyncWithInfura.add_block_to_tree(db, chain, tree, current_block.header.number)
diff --git a/apps/blockchain/test/blockchain/contract/create_contract_test.exs b/apps/blockchain/test/blockchain/contract/create_contract_test.exs
@@ -2,6 +2,7 @@ defmodule Blockchain.Contract.CreateContractTest do
   use ExUnit.Case
   doctest Blockchain.Contract.CreateContract
 
+  alias ExthCrypto.Hash.Keccak
   alias Blockchain.{Account, Contract}
   alias EVM.{SubState, MachineCode}
   alias MerklePatriciaTree.{Trie, DB}
@@ -28,13 +29,12 @@ defmodule Blockchain.Contract.CreateContractTest do
         available_gas: 100_000_000,
         gas_price: 1,
         endowment: 5,
-        init_code: build_sample_code(),
+        init_code: init_code(),
         stack_depth: 5,
         block_header: %Block.Header{nonce: 1}
       }
 
-      {_, {account_interface, gas, sub_state}} = Contract.create(params)
-      state = account_interface.state
+      {_, {%{state: state}, gas, sub_state}} = Contract.create(params)
 
       expected_root_hash =
         <<9, 235, 32, 146, 153, 242, 209, 192, 224, 61, 214, 174, 48, 24, 148, 28, 51, 254, 7, 82,
@@ -68,7 +68,7 @@ defmodule Blockchain.Contract.CreateContractTest do
     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 = %Account{nonce: 1, code_hash: init_code_result_hash()}
       collision_account_address = Account.Address.new(account_address, account.nonce)
 
       state =
@@ -84,7 +84,7 @@ defmodule Blockchain.Contract.CreateContractTest do
         available_gas: 100_000_000,
         gas_price: 1,
         endowment: 5,
-        init_code: build_sample_code(),
+        init_code: init_code(),
         stack_depth: 5,
         block_header: %Block.Header{nonce: 1}
       }
@@ -98,7 +98,7 @@ defmodule Blockchain.Contract.CreateContractTest do
     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 = %Account{code_hash: init_code_result_hash(), nonce: 0}
       collision_account_address = Account.Address.new(account_address, account.nonce)
 
       state =
@@ -114,7 +114,7 @@ defmodule Blockchain.Contract.CreateContractTest do
         available_gas: 100_000_000,
         gas_price: 1,
         endowment: 5,
-        init_code: build_sample_code(),
+        init_code: init_code(),
         stack_depth: 5,
         block_header: %Block.Header{nonce: 1}
       }
@@ -124,9 +124,56 @@ defmodule Blockchain.Contract.CreateContractTest do
 
       assert SubState.empty?(sub_state)
     end
+
+    test "creates a contract even if the address already has a balance", %{db: db} do
+      account = %Account{balance: 10, nonce: 2}
+      contract_account = %Account{balance: 10}
+
+      contract_address = Account.Address.new(<<0x10::160>>, account.nonce)
+
+      state =
+        db
+        |> Trie.new()
+        |> Account.put_account(<<0x10::160>>, account)
+        |> Account.put_account(contract_address, contract_account)
+
+      params = %Contract.CreateContract{
+        account_interface: AccountInterface.new(state),
+        sender: <<0x10::160>>,
+        originator: <<0x10::160>>,
+        available_gas: 100_000_000,
+        gas_price: 1,
+        endowment: 5,
+        init_code: init_code(),
+        stack_depth: 5,
+        block_header: %Block.Header{nonce: 1}
+      }
+
+      {_, {%{state: state}, gas, sub_state}} = Contract.create(params)
+
+      addresses = [<<0x10::160>>, Account.Address.new(<<0x10::160>>, 2)]
+      actual_accounts = Account.get_accounts(state, addresses)
+
+      expected_accounts = [
+        %Account{
+          balance: 5,
+          nonce: 2
+        },
+        %Account{
+          balance: 15,
+          code_hash: init_code_result_hash()
+        }
+      ]
+
+      assert actual_accounts == expected_accounts
+    end
+  end
+
+  defp init_code_result_hash do
+    Keccak.kec(<<8::256>>)
   end
 
-  defp build_sample_code do
+  defp init_code do
     assembly = [
       :push1,
       3,

diff --git a/apps/blockchain/test/blockchain/state_test.exs b/apps/blockchain/test/blockchain/state_test.exs
@@ -46,6 +46,7 @@ defmodule Blockchain.StateTest do
       "stCreate2/create2callPrecompiles",
       "stCreate2/create2checkFieldsInInitcode",
       "stCreate2/create2collisionBalance",
+      "stCreate2/create2collisionStorage",
       "stCreate2/returndatacopy_afterFailing_create",
       "stCreate2/returndatacopy_following_revert_in_create",
       "stRevertTest/RevertOpcodeMultipleSubCalls",

diff --git a/apps/blockchain/test/blockchain/transaction_test.exs b/apps/blockchain/test/blockchain/transaction_test.exs
@@ -436,7 +436,7 @@ defmodule Blockchain.TransactionTest do
       assert beneficiary_account == %Account{balance: gas * gas_price}
 
       contract_address = Account.Address.new(sender.address, sender.nonce)
-      refute Account.exists?(Account.get_account(state, contract_address))
+      assert is_nil(Account.get_account(state, contract_address))
     end
   end
 end
diff --git a/apps/blockchain/test/blockchain_test.exs b/apps/blockchain/test/blockchain_test.exs
@@ -18,6 +18,9 @@ defmodule BlockchainTest do
     ],
     "Byzantium" => String.split(@failing_byzantium_tests, "\n"),
     "Constantinople" => [
+      "GeneralStateTests/stCreate2/create2collisionStorage_d0g0v0.json",
+      "GeneralStateTests/stCreate2/create2collisionStorage_d1g0v0.json",
+      "GeneralStateTests/stCreate2/create2collisionStorage_d2g0v0.json",
       "GeneralStateTests/stCreate2/Create2OnDepth1023_d0g0v0.json",
       "GeneralStateTests/stCreate2/Create2OnDepth1024_d0g0v0.json",
       "GeneralStateTests/stCreate2/Create2Recursive_d0g0v0.json",