fix: use custom address recovery for fake signature

[agostbiro]

Make sure we're not trying to recover the address from fake signatures. Fixes NomicFoundation/edr#294

[changeset-bot]

⚠️ No Changeset found

Latest commit: f481801

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hardhat	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 19, 2024 7:41pm

[Wodann]

I think I would have solved this differently, but this fixes the issue for now.

The function transaction_from_block can return the ExecutableTransaction which includes the from-address, voiding the need for recovery. This PR shows my intended solution.

Something we should wonder is whether the concept of faking a transaction should be incorporated in the raw transaction type? This PR does so now, which I'm not a fan of.
A limiting factor atm is that we don't have a way of expressing that an Address is a valid Ethereum address.

E.g. it's currently still possible that someone uses a JSON-RPC client with fake transactions for testing, which would result in the recovery of an invalid Ethereum address.

I'll approve to expedite the merging process, but if you agree with the above, I'd prefer a minimal change to use the ExecutionTransaction (which already contains the correct address) and to revisit this discussion when we deal with NomicFoundation/edr#260

[fvictorio]

Something we should wonder is whether the concept of faking a transaction should be incorporated in the raw transaction type?

I don't think it should.

A limiting factor atm is that we don't have a way of expressing that an Address is a valid Ethereum address.
E.g. it's currently still possible that someone uses a JSON-RPC client with fake transactions for testing, which would result in the recovery of an invalid Ethereum address.

I'm not sure I understand this. Could you elaborate?

[agostbiro]

Thanks for thinking deeply about this @Wodann! I'm answering between the lines below.

Something we should wonder is whether the concept of faking a transaction should be incorporated in the raw transaction type? This PR does so now, which I'm not a fan of.

Faking a transaction is already incorporated into the raw transaction type:

hardhat/crates/edr_eth/src/transaction/request.rs

Line 103 in f6eb936

pub fn fake_sign(self, sender: &Address) -> SignedTransaction {

The source of the bug is that this is not taken into account when doing recovery which this PR fixes.

A limiting factor atm is that we don't have a way of expressing that an Address is a valid Ethereum address.

E.g. it's currently still possible that someone uses a JSON-RPC client with fake transactions for testing, which would result in the recovery of an invalid Ethereum address.

I don't follow either, sorry. Please elaborate :)

The function transaction_from_block can return the ExecutableTransaction which includes the from-address, voiding the need for recovery. #4911 shows my intended solution.

That's a good idea, but it still leaves SignedTransaction::recover in a broken state, so this bug is bound to reoccur in the future unless we fix that.

I'll approve to expedite the merging process, but if you agree with the above, I'd prefer a minimal change to use the ExecutionTransaction (which already contains the correct address) and to revisit this discussion when we deal with NomicFoundation/edr#260

The right way to handle this imo would be to have two signed transaction types: SignedTransaction and FakeSignedTransaction (or possibly a SignedTransaction<SignatureType>), as that'd ensure at compile time (instead of the runtime check solution in this PR) that the recovery logic is appropriate for the signature method.

But moving the check to compile time is a bigger change, hence the approach in this PR.

All in all, I'd prefer the solution in this PR over #4911 as while the latter fixes the immediate problem, it doesn't prevent it from reoccurring.

[Wodann]

The right way to handle this imo would be to have two signed transaction types: SignedTransaction and FakeSignedTransaction (or possibly a SignedTransaction), as that'd ensure at compile time (instead of the runtime check solution in this PR) that the recovery logic is appropriate for the signature method.

I agree with this. The SignedTransaction<SignatureType> would allow us to re-use as much of the data as possible. Could you create an issue for that, @agostbiro?

All in all, I'd prefer the solution in this PR over #4911 as while the latter fixes the immediate problem, it doesn't prevent it from reoccurring.

It's unnecessary to use recover() given that we already know the Address when we call it. So at the very least we should merge both solutions; given that they solve different things.