From a5be5144ac6befc27daf02c720497af39be312ca Mon Sep 17 00:00:00 2001 From: German Velasco Date: Wed, 15 Aug 2018 14:58:41 -0400 Subject: [PATCH] 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 https://github.com/ethereum/EIPs/issues/684 --- apps/blockchain/lib/blockchain/account.ex | 8 ++ .../blockchain/contract/create_contract.ex | 43 +++++--- .../contract/create_contract_test.exs | 98 +++++++++++++++---- apps/blockchain/test/blockchain_test.exs | 4 +- 4 files changed, 114 insertions(+), 39 deletions(-) diff --git a/apps/blockchain/lib/blockchain/account.ex b/apps/blockchain/lib/blockchain/account.ex index 517ef078e..46bad9d54 100644 --- 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 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. """ diff --git a/apps/blockchain/lib/blockchain/contract/create_contract.ex b/apps/blockchain/lib/blockchain/contract/create_contract.ex index 6df924db2..dbd8bd19d 100644 --- 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 index b5def5d15..3f12aa1ac 100644 --- 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_test.exs b/apps/blockchain/test/blockchain_test.exs index f61c311d0..0afaec550 100644 --- 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",