Merge pull request #47 from mikeshultz/large-chainid-error

mikeshultz · web-flow · commit 1f306c73a0ae · 2023-08-12T19:03:51.000-06:00
fix: throw errors on chain IDs above what app-ethereum plays well with
diff --git 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
@@ -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
@@ -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
@@ -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]
