bug: legacy (type 0) transactions and high chain IDs fail signature recovery

[mikeshultz]

type 0 transactions seem to have an issue with high chain IDs (e.g. 131277322940420). I've tested on hardware and it works up to mumbai (80001). Type 1 and 2 don't have the same problem.

I think @moh-eulith tried to address this by updating MockDongle to get the tests to pass but I'm seeing issues using a chain ID like this on hardware. Probably indicative of another issue.

Here's a test that will cause an invalid signature recovery on hardware:

def test_zero_gas_price(yield_dongle):
    """Test a transaction with a 0 gas price"""
    chain_id = 131277322940420
    destination = "0xf0155486a14539f784739be1c02e93f28eb8e960"

    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),
            # TODO
            gas_price=int(1e9),
            data="",
            nonce=2023,
            chain_id=chain_id,
            dongle=dongle,
        )

        print("signed:", signed)

        # assert signed.v in [(chain_id * 2 + 35) + x for x in (0, 1)]
        # assert signed.r
        # assert signed.s
        assert sender == Account.recover_transaction(signed.rawTransaction)

I'm not sure if it's a limitation of the transaction type, RLP, or my implementation.

[mikeshultz]

Relevant MockDongle change:

611798f#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R45

[mikeshultz]

Took a couple hours of approving signatures on a hardware device, but it seems that signatures start failing at chain ID of 4294887296.

>>> 0b11111111111111111111111111111111 - 4294887296
79999

Coincadence or

[mikeshultz]

My iterator in my test was added on top of a start number, which was 80001... FFS

Okay, so, it seems the limit is a 32-bit number. At least that's a clue.

[mikeshultz]

I found EIP-2294 but it doesn't appear to have been adopted. Some things apparently have limits, like Metamask at 4503599627370476 but that's way above what we're running into. Could be an app-ethereum limitation.

[mikeshultz]

There's this issue: LedgerHQ/app-ethereum#283

Which seems a bit vague and reports a different error that we've been seeing. For this issue, the device isn't throwing an error, just returning what appears to be an invalid signature. Could be unrelated.

[mikeshultz]

app-ethereum moved from a 32-bit uint to 64-bit in this commit: LedgerHQ/app-ethereum@b2172e4

But that was a year ago and I'm fairly certain I'm running a version after that.

Found this commit (LedgerHQ/app-ethereum@8260268) implying an invalid V and the client just having to deal with it, and the following comment talking about something similar.

https://github.com/LedgerHQ/app-ethereum/blob/6bb2d8ab97718701ebcec2cfb6861f94ddf0e553/src_features/signTx/ui_common_signTx.c#L45

[mikeshultz]

Ledger Live fiddles with signatures like so:

https://github.com/LedgerHQ/ledger-live/blob/6b3b5c8d3283fafab666a42e7c03d69aeb8f324d/libs/coin-evm/src/signOperation.ts#L27-L43

This just doesn't work for us. This one looks more like us:

https://github.com/LedgerHQ/ledger-live/blob/6b3b5c8d3283fafab666a42e7c03d69aeb8f324d/libs/ledgerjs/packages/hw-app-eth/src/Eth.ts#L267-L281

But there's a little bit different. Chain ID appears to be truncated in some fashion...

[mikeshultz]

Testing the upper limits of type 2 transactions and apparently the limiting factor there is the largest number that can be displayed on the device (tested on a Ledger S).

This is the error thrown:

https://github.com/LedgerHQ/app-ethereum/blob/6bb2d8ab97718701ebcec2cfb6861f94ddf0e553/src_common/ethUtils.c#L142-L144

Max integer appears to be what can be 15 characters long in base 10, so

>>> int(0x38D7EA4C67FFF)  # works fine
999999999999999
>>> int(0x38D7EA4C67FFF) + 1  # throws 0x6502 error
1000000000000000