From 51dcbe159813cf5c0c2ef7bb1f61e2870367ce60 Mon Sep 17 00:00:00 2001 From: Matias Romeo Date: Tue, 11 Jul 2023 19:12:11 -0300 Subject: [PATCH 1/2] Enforce chain-id in transactions pushed to the contract --- contract/src/actions.cpp | 4 +++ contract/tests/CMakeLists.txt | 1 + contract/tests/basic_evm_tester.cpp | 9 +++++-- contract/tests/basic_evm_tester.hpp | 1 + contract/tests/chainid_tests.cpp | 42 +++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 contract/tests/chainid_tests.cpp diff --git a/contract/src/actions.cpp b/contract/src/actions.cpp index e2171a7a..5498f3f0 100644 --- a/contract/src/actions.cpp +++ b/contract/src/actions.cpp @@ -244,6 +244,10 @@ Receipt evm_contract::execute_tx( eosio::name miner, Block& block, Transaction& else if(is_reserved_address(*tx.from)) check(from_self, "bridge signature used outside of bridge transaction"); + if(!from_self) { + check(tx.chain_id.has_value(), "tx without chain-id"); + } + ValidationResult r = consensus::pre_validate_transaction(tx, ep.evm().block().header.number, ep.evm().config(), ep.evm().block().header.base_fee_per_gas); check_result( r, tx, "pre_validate_transaction error" ); diff --git a/contract/tests/CMakeLists.txt b/contract/tests/CMakeLists.txt index 65541ff0..ab15b757 100644 --- a/contract/tests/CMakeLists.txt +++ b/contract/tests/CMakeLists.txt @@ -33,6 +33,7 @@ add_eosio_test_executable( unit_test ${CMAKE_SOURCE_DIR}/gas_fee_tests.cpp ${CMAKE_SOURCE_DIR}/blockhash_tests.cpp ${CMAKE_SOURCE_DIR}/exec_tests.cpp + ${CMAKE_SOURCE_DIR}/chainid_tests.cpp ${CMAKE_SOURCE_DIR}/main.cpp ${CMAKE_SOURCE_DIR}/silkworm/core/silkworm/rlp/encode.cpp ${CMAKE_SOURCE_DIR}/silkworm/core/silkworm/rlp/decode.cpp diff --git a/contract/tests/basic_evm_tester.cpp b/contract/tests/basic_evm_tester.cpp index 7c028ef4..a9d7ece5 100644 --- a/contract/tests/basic_evm_tester.cpp +++ b/contract/tests/basic_evm_tester.cpp @@ -95,10 +95,15 @@ key256_t evm_eoa::address_key256() const return fixed_bytes<32>(buffer).get_array(); } -void evm_eoa::sign(silkworm::Transaction& trx) +void evm_eoa::sign(silkworm::Transaction& trx) { + sign(trx, basic_evm_tester::evm_chain_id); +} + +void evm_eoa::sign(silkworm::Transaction& trx, std::optional evm_chain_id) { silkworm::Bytes rlp; - trx.chain_id = basic_evm_tester::evm_chain_id; + if(evm_chain_id.has_value()) + trx.chain_id = evm_chain_id.value(); trx.nonce = next_nonce++; silkworm::rlp::encode(rlp, trx, true, false); ethash::hash256 hash{silkworm::keccak256(rlp)}; diff --git a/contract/tests/basic_evm_tester.hpp b/contract/tests/basic_evm_tester.hpp index a554faaa..56b79940 100644 --- a/contract/tests/basic_evm_tester.hpp +++ b/contract/tests/basic_evm_tester.hpp @@ -134,6 +134,7 @@ class evm_eoa key256_t address_key256() const; void sign(silkworm::Transaction& trx); + void sign(silkworm::Transaction& trx, std::optional chain_id); ~evm_eoa(); diff --git a/contract/tests/chainid_tests.cpp b/contract/tests/chainid_tests.cpp new file mode 100644 index 00000000..99e3ac89 --- /dev/null +++ b/contract/tests/chainid_tests.cpp @@ -0,0 +1,42 @@ +#include "basic_evm_tester.hpp" +#include + +using namespace evm_test; +struct chain_id_tester : basic_evm_tester { + chain_id_tester() { + create_accounts({"alice"_n}); + transfer_token(faucet_account_name, "alice"_n, make_asset(10000'0000)); + init(); + } +}; + +BOOST_AUTO_TEST_SUITE(chainid_evm_tests) +BOOST_FIXTURE_TEST_CASE(chainid_tests, chain_id_tester) try { + + // Bridge transactions are sent without chain id specified (must succeed) + evm_eoa evm1; + const int64_t to_bridge = 1000000; //100.000 EOS + transfer_token("alice"_n, "evm"_n, make_asset(to_bridge), evm1.address_0x()); + auto balance_evm1 = evm_balance(evm1); + BOOST_REQUIRE(balance_evm1.has_value() && *balance_evm1 == 100_ether); + + // Transactions without chain id specified must fail + evm_eoa evm2; + auto txn = generate_tx(evm2.address, 1_ether, 21000); + evm1.sign(txn, {}); + BOOST_REQUIRE_EXCEPTION(pushtx(txn), eosio_assert_message_exception, + [](const eosio_assert_message_exception& e) {return testing::expect_assert_message(e, "assertion failure with message: tx without chain-id");}); + + // reuse nonce since previous signed transaction was rejected + evm1.next_nonce--; + + // Transactions with chain id must succeed + evm_eoa evm3; + txn = generate_tx(evm3.address, 20_ether, 21000); + evm1.sign(txn, basic_evm_tester::evm_chain_id); + pushtx(txn); + auto balance_evm3 = evm_balance(evm3); + BOOST_REQUIRE(balance_evm3.has_value() && *balance_evm3 == 20_ether); + +} FC_LOG_AND_RETHROW() +BOOST_AUTO_TEST_SUITE_END() From 2e6be433542bae672b3cb8a85a56c9eccf1dd9c9 Mon Sep 17 00:00:00 2001 From: Matias Romeo Date: Tue, 11 Jul 2023 20:45:05 -0300 Subject: [PATCH 2/2] Skip chain-id enforcing in testtx action --- contract/include/evm_runtime/evm_contract.hpp | 2 +- contract/src/actions.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contract/include/evm_runtime/evm_contract.hpp b/contract/include/evm_runtime/evm_contract.hpp index e1893d8e..f6955fba 100644 --- a/contract/include/evm_runtime/evm_contract.hpp +++ b/contract/include/evm_runtime/evm_contract.hpp @@ -131,7 +131,7 @@ class [[eosio::contract]] evm_contract : public contract check((_config.get().status & static_cast(status_flags::frozen)) == 0, "contract is frozen"); } - silkworm::Receipt execute_tx(eosio::name miner, silkworm::Block& block, silkworm::Transaction& tx, silkworm::ExecutionProcessor& ep); + silkworm::Receipt execute_tx(eosio::name miner, silkworm::Block& block, silkworm::Transaction& tx, silkworm::ExecutionProcessor& ep, bool enforce_chain_id); uint64_t get_and_increment_nonce(const name owner); diff --git a/contract/src/actions.cpp b/contract/src/actions.cpp index 5498f3f0..0c202c48 100644 --- a/contract/src/actions.cpp +++ b/contract/src/actions.cpp @@ -194,7 +194,7 @@ void check_result( ValidationResult r, const Transaction& txn, const char* desc eosio::check( false, std::move(err_msg)); } -Receipt evm_contract::execute_tx( eosio::name miner, Block& block, Transaction& tx, silkworm::ExecutionProcessor& ep ) { +Receipt evm_contract::execute_tx( eosio::name miner, Block& block, Transaction& tx, silkworm::ExecutionProcessor& ep, bool enforce_chain_id) { //when being called as an inline action, clutch in allowance for reserved addresses & signatures by setting from_self=true const bool from_self = get_sender() == get_self(); @@ -244,7 +244,7 @@ Receipt evm_contract::execute_tx( eosio::name miner, Block& block, Transaction& else if(is_reserved_address(*tx.from)) check(from_self, "bridge signature used outside of bridge transaction"); - if(!from_self) { + if(enforce_chain_id && !from_self) { check(tx.chain_id.has_value(), "tx without chain-id"); } @@ -406,7 +406,7 @@ void evm_contract::pushtx( eosio::name miner, const bytes& rlptx ) { check(tx.max_priority_fee_per_gas == tx.max_fee_per_gas, "max_priority_fee_per_gas must be equal to max_fee_per_gas"); check(tx.max_fee_per_gas >= current_config.gas_price, "gas price is too low"); - auto receipt = execute_tx(miner, block, tx, ep); + auto receipt = execute_tx(miner, block, tx, ep, true); engine.finalize(ep.state(), ep.evm().block(), ep.evm().revision()); ep.state().write_to_db(ep.evm().block().header.number); @@ -573,7 +573,7 @@ bool evm_contract::gc(uint32_t max) { ByteView bv{(const uint8_t*)orlptx->data(), orlptx->size()}; eosio::check(rlp::decode(bv,tx) == DecodingResult::kOk && bv.empty(), "unable to decode transaction"); - execute_tx(eosio::name{}, block, tx, ep); + execute_tx(eosio::name{}, block, tx, ep, false); } engine.finalize(ep.state(), ep.evm().block(), ep.evm().revision()); ep.state().write_to_db(ep.evm().block().header.number);