Repository navigation
bug: legacy (type 0) transactions and high chain IDs fail signature recovery #41
Description
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.
Activity
mikeshultz commentedon Aug 5, 2023
Relevant
MockDonglechange:611798f#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R45
mikeshultz commentedon Aug 5, 2023
Took a couple hours of approving signatures on a hardware device, but it seems that signatures start failing at chain ID of
4294887296.Coincadence or
mikeshultz commentedon Aug 5, 2023
My iterator in my test was added on top of a start number, which was
80001... FFSOkay, so, it seems the limit is a 32-bit number. At least that's a clue.
mikeshultz commentedon Aug 5, 2023
I found EIP-2294 but it doesn't appear to have been adopted. Some things apparently have limits, like Metamask at
4503599627370476but that's way above what we're running into. Could be anapp-ethereumlimitation.mikeshultz commentedon Aug 5, 2023
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 commentedon Aug 5, 2023
app-ethereummoved from a 32-bit uint to 64-bit in this commit: LedgerHQ/app-ethereum@b2172e4But 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 commentedon Aug 5, 2023
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 commentedon Aug 13, 2023
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
fix: throw errors on chain IDs above what app-ethereum plays well wit…