From bfb7374807ff8a9f47dcbc9d412591b9468c6fdb Mon Sep 17 00:00:00 2001 From: Mike Shultz Date: Sat, 12 Aug 2023 18:48:44 -0600 Subject: [PATCH] fix: throw errors on chain IDs above what app-ethereum plays well with. Issue #41 --- ledgereth/objects.py | 47 ++++++++++++++ tests/conftest.py | 3 +- tests/test_chain_id.py | 143 +++++++++++++++++++++++++++++++++++++++++ tests/test_web3.py | 7 +- 4 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 tests/test_chain_id.py diff --git a/ledgereth/objects.py b/ledgereth/objects.py index f8b63ad..51f9d3c 100644 --- a/ledgereth/objects.py +++ b/ledgereth/objects.py @@ -53,6 +53,8 @@ "chainId", "accessList", ] +MAX_LEGACY_CHAIN_ID = 0xFFFFFFFF + 1 +MAX_CHAIN_ID = 0x38D7EA4C67FFF class TransactionType(IntEnum): @@ -241,6 +243,9 @@ def to_rpc_dict(self) -> Dict[str, Any]: class Transaction(SerializableTransaction): """Unsigned legacy or `EIP-155`_ transaction + .. warning:: chain_id for type 0 ("Legacy") transactions must be less than + 4294967295, the largest 32-bit unsigned integer. + .. note:: A chain_id is set by default (``1``). It is not required to be a valid legacy transaction, but without it your transaction is suceptible to replay attack. If for some reason you absolutely do not @@ -289,6 +294,18 @@ def __init__( :param dummy1: (``int``) **DO NOT SET** :param dummy2: (``int``) **DO NOT SET** """ + + if chain_id > MAX_LEGACY_CHAIN_ID: + """Chain IDs above 32-bits seems to cause app-ethereum to create + invalid signatures. It's not yet clear why this is, or where the + bug is, or even if it's a bug. See the following issue for details: + + https://github.com/mikeshultz/ledger-eth-lib/issues/41 + """ + raise ValueError( + "chain_id must be a 32-bit integer for type 0 transactions. (See issue #41)" + ) + super().__init__( nonce, gas_price, @@ -321,6 +338,9 @@ def from_rawtx(cls, rawtx: bytes) -> Transaction: class Type1Transaction(SerializableTransaction): """An unsigned Type 1 transaction. + .. warning:: chain_id for type 1 transactions must be less than 999999999999999, + the largest unsigned integer that the device can render on-screen. + Encoded tx format spec: .. code:: @@ -366,6 +386,18 @@ def __init__( list """ access_list = access_list or [] + + if chain_id > MAX_CHAIN_ID: + """Chain IDs above 999999999999999 cause app-ethereum to throw an error + because its unable to render on the device. + + Ref: https://github.com/mikeshultz/ledger-eth-lib/issues/41 + Ref: https://github.com/LedgerHQ/app-ethereum/issues/283 + """ + raise ValueError( + "chain_id must not be above 999999999999999. (See issue #41)" + ) + super().__init__( chain_id, nonce, @@ -400,6 +432,9 @@ def from_rawtx(cls, rawtx: bytes) -> Type1Transaction: class Type2Transaction(SerializableTransaction): """An unsigned Type 2 transaction. + .. warning:: chain_id for type 2 transactions must be less than 999999999999999, + the largest unsigned integer that the device can render on-screen. + Encoded TX format spec: .. code:: @@ -450,6 +485,18 @@ def __init__( list """ access_list = access_list or [] + + if chain_id > MAX_CHAIN_ID: + """Chain IDs above 999999999999999 cause app-ethereum to throw an error + because its unable to render on the device. + + Ref: https://github.com/mikeshultz/ledger-eth-lib/issues/41 + Ref: https://github.com/LedgerHQ/app-ethereum/issues/283 + """ + raise ValueError( + "chain_id must not be above 999999999999999. (See issue #41)" + ) + super().__init__( chain_id, nonce, diff --git a/tests/conftest.py b/tests/conftest.py index f6e2e88..a8a2744 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -41,7 +41,8 @@ def _reset(self): self.stack = [] def _resp_to_bytearray(self, resp): - # for large chain_id, v can be bigger than a byte. we'll take the lowest byte for the signature + # for large chain_id, v can be bigger than a byte. we'll take the lowest + # byte for the signature v = resp.v.to_bytes(8, "big")[7].to_bytes(1, "big") r = resp.r.to_bytes(32, "big") s = resp.s.to_bytes(32, "big") diff --git a/tests/test_chain_id.py b/tests/test_chain_id.py new file mode 100644 index 0000000..5221969 --- /dev/null +++ b/tests/test_chain_id.py @@ -0,0 +1,143 @@ +"""Test signing for multiple chains""" +import pytest +from eth_account import Account + +from ledgereth.accounts import get_accounts +from ledgereth.objects import MAX_CHAIN_ID, MAX_LEGACY_CHAIN_ID +from ledgereth.transactions import create_transaction + + +def test_max_legacy_chain_ids(yield_dongle): + """Test that the max legacy chain ID works""" + destination = "0xf0155486a14539f784739be1c02e93f28eb8e900" + + with yield_dongle() as dongle: + sender = get_accounts(dongle=dongle, count=1)[0].address + + signed = create_transaction( + destination=destination, + amount=int(10e17), + gas=int(1e6), + gas_price=int(1e9), + data="", + nonce=2023, + chain_id=MAX_LEGACY_CHAIN_ID, + dongle=dongle, + ) + + assert sender == Account.recover_transaction(signed.rawTransaction) + + +def test_invalid_legacy_chain_ids(yield_dongle): + """Test that chain IDs above max legacy chain ID fail""" + destination = "0xf0155486a14539f784739be1c02e93f28eb8e901" + + with yield_dongle() as dongle: + sender = get_accounts(dongle=dongle, count=1)[0].address + + with pytest.raises( + ValueError, + match="chain_id must be a 32-bit integer for type 0 transactions", + ): + create_transaction( + destination=destination, + amount=int(10e17), + gas=int(1e6), + gas_price=int(1e9), + data="", + nonce=2023, + chain_id=MAX_LEGACY_CHAIN_ID + 1, + dongle=dongle, + ) + + +def test_max_type1_chain_ids(yield_dongle): + """Test that the max type-1 chain ID works""" + destination = "0xf0155486a14539f784739be1c02e93f28eb8e902" + + with yield_dongle() as dongle: + sender = get_accounts(dongle=dongle, count=1)[0].address + + signed = create_transaction( + destination=destination, + amount=int(10e17), + gas=int(1e6), + access_list=[], + gas_price=int(1e9), + data="", + nonce=2023, + chain_id=MAX_CHAIN_ID, + dongle=dongle, + ) + + assert sender == Account.recover_transaction(signed.rawTransaction) + + +def test_invalid_type1_chain_ids(yield_dongle): + """Test that IDs above the max chain ID fail""" + destination = "0xf0155486a14539f784739be1c02e93f28eb8e903" + + with yield_dongle() as dongle: + sender = get_accounts(dongle=dongle, count=1)[0].address + + with pytest.raises( + ValueError, + match="chain_id must not be above 999999999999999", + ): + create_transaction( + destination=destination, + amount=int(10e17), + gas=int(1e6), + access_list=[], + gas_price=int(1e9), + data="", + nonce=2023, + chain_id=MAX_CHAIN_ID + 1, + dongle=dongle, + ) + + +def test_max_type2_chain_ids(yield_dongle): + """Test that the max chain ID works for type-2 transactions""" + destination = "0xf0155486a14539f784739be1c02e93f28eb8e904" + + with yield_dongle() as dongle: + sender = get_accounts(dongle=dongle, count=1)[0].address + + signed = create_transaction( + destination=destination, + amount=int(10e17), + gas=int(1e6), + max_fee_per_gas=int(1e9), + max_priority_fee_per_gas=int(1e8), + data="", + nonce=2023, + chain_id=MAX_CHAIN_ID, + dongle=dongle, + ) + + assert sender == Account.recover_transaction(signed.rawTransaction) + + +def test_invalid_type2_chain_ids(yield_dongle): + """Test that IDs above the max chain ID fail for type-2 transactions""" + destination = "0xf0155486a14539f784739be1c02e93f28eb8e905" + + with yield_dongle() as dongle: + sender = get_accounts(dongle=dongle, count=1)[0].address + + with pytest.raises( + ValueError, + match="chain_id must not be above 999999999999999", + ): + create_transaction( + destination=destination, + amount=int(10e17), + gas=int(1e6), + max_fee_per_gas=int(1e9), + max_priority_fee_per_gas=int(1e8), + data="", + nonce=2023, + chain_id=MAX_CHAIN_ID + 1, + dongle=dongle, + ) diff --git a/tests/test_web3.py b/tests/test_web3.py index 106e958..a7c475b 100644 --- a/tests/test_web3.py +++ b/tests/test_web3.py @@ -4,6 +4,7 @@ from web3 import Web3 from web3.datastructures import AttributeDict from web3.providers.eth_tester import EthereumTesterProvider +from web3.providers.eth_tester.defaults import API_ENDPOINTS, static_return from web3.types import TxReceipt, Wei from ledgereth.accounts import get_accounts @@ -38,7 +39,11 @@ def fund_account(web3: Web3, address: str, amount: int = int(1e18)) -> TxReceipt def test_web3_middleware_legacy(yield_dongle): """Test LedgerSignerMiddleware with a legacy transaction""" - provider = EthereumTesterProvider() + endpoints = {**API_ENDPOINTS} + # Max chain ID for legacy transactions + # Ref: https://github.com/mikeshultz/ledger-eth-lib/issues/41 + endpoints["eth"]["chainId"] = static_return(4294967295) + provider = EthereumTesterProvider(api_endpoints=endpoints) web3 = Web3(provider) clean_web3 = Web3(provider) alice_address = web3.eth.accounts[0]