From 894d71a78bdbffa23fc22dcb3228aa579ba96087 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 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 | 8 +- 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, 118 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 0105516e3..d6d167d9d 100644 --- 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 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 360e2dd36..1c91d27bd 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", @@ -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 index f61c311d0..d0e1efa49 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", @@ -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",