From 6d2321b89a1216075ecefcf6ae1997e0bd5f5fa1 Mon Sep 17 00:00:00 2001 From: German Velasco Date: Thu, 16 Aug 2018 12:51:54 -0400 Subject: [PATCH] Prevent collisions on account creation (#358) 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 https://github.com/ethereum/EIPs/issues/684 --- README.md | 4 +- apps/blockchain/lib/blockchain/account.ex | 8 ++ .../blockchain/contract/create_contract.ex | 43 +++++--- .../contract/create_contract_test.exs | 98 +++++++++++++++---- .../blockchain/test/blockchain/state_test.exs | 2 - apps/blockchain/test/blockchain_test.exs | 5 +- 6 files changed, 116 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index ed148168b..ec36827f4 100644 --- a/README.md +++ b/README.md @@ -94,8 +94,8 @@ 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) 1167/1233 = 94.6% passing - [GeneralStateTests](https://github.com/ethereum/tests/tree/develop/GeneralStateTests) 1107/1181= 93.7% passing diff --git a/apps/blockchain/lib/blockchain/account.ex b/apps/blockchain/lib/blockchain/account.ex index ffd5f27a4..7daf25743 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 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 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/state_test.exs b/apps/blockchain/test/blockchain/state_test.exs index 1a66b948c..40e27a3e2 100644 --- 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", @@ -53,7 +52,6 @@ defmodule Blockchain.StateTest do "stSystemOperationsTest/suicideCoinbase", "stSystemOperationsTest/doubleSelfdestructTest2", "stSystemOperationsTest/doubleSelfdestructTest", - "stSystemOperationsTest/CreateHashCollision", "stSpecialTest/tx_e1c174e2", "stSpecialTest/failed_tx_xcf416c53", "stRevertTest/RevertPrefoundEmptyCall", diff --git a/apps/blockchain/test/blockchain_test.exs b/apps/blockchain/test/blockchain_test.exs index 4b968c710..88f91403a 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/stCallCodes/call_OOG_additionalGasCosts1_d0g0v0.json", "GeneralStateTests/stCreateTest/CREATE_AcreateB_BSuicide_BStore_d0g0v0.json", @@ -65,7 +63,6 @@ defmodule BlockchainTest do "GeneralStateTests/stRevertTest/RevertPrefound_d0g0v0.json", "GeneralStateTests/stSpecialTest/failed_tx_xcf416c53_d0g0v0.json", "GeneralStateTests/stSpecialTest/tx_e1c174e2_d0g0v0.json", - "GeneralStateTests/stSystemOperationsTest/CreateHashCollision_d0g0v0.json", "GeneralStateTests/stSystemOperationsTest/doubleSelfdestructTest2_d0g0v0.json", "GeneralStateTests/stSystemOperationsTest/doubleSelfdestructTest_d0g0v0.json", "GeneralStateTests/stTransactionTest/OverflowGasRequire2_d0g0v0.json",