From 30ca12e6e38d107aa80e0b626078a345ff0f06f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 31 May 2022 12:54:41 -0500 Subject: [PATCH] Util: Allow `v` to be `0` or `1` for EIP1559 transactions (#1905) * Util: Allow v to be `0` or `1` for EIP1559 transactions * vm: fix ecrecover precompile for v=0 and v=1 * tx/legacyTransaction: add `v` guard for non-EIP155 txs * tx: switch to earlier v validation and throwing before v common/EIP-155 determination logic * tx: remove v==0 check which always defaults to the default common Co-authored-by: Jochem Brouwer Co-authored-by: Holger Drewes --- packages/tx/src/legacyTransaction.ts | 12 ++++- packages/tx/test/legacy.spec.ts | 31 +++++++++--- packages/util/src/signature.ts | 9 ++++ packages/util/test/signature.spec.ts | 48 ++++++++++++++++++- .../vm/src/evm/precompiles/01-ecrecover.ts | 12 +++++ 5 files changed, 103 insertions(+), 9 deletions(-) diff --git a/packages/tx/src/legacyTransaction.ts b/packages/tx/src/legacyTransaction.ts index 0df16cb579..dc26a1dcf5 100644 --- a/packages/tx/src/legacyTransaction.ts +++ b/packages/tx/src/legacyTransaction.ts @@ -378,11 +378,21 @@ export default class Transaction extends BaseTransaction { * Validates tx's `v` value */ private _validateTxV(v?: BN, common?: Common): Common { + // Check for valid v values in the scope of a signed legacy tx + if (v !== undefined) { + // v is 1. not matching the EIP-155 chainId included case and... + // v is 2. not matching the classic v=27 or v=28 case + if (v.ltn(37) && !v.eqn(27) && !v.eqn(28)) { + throw new Error( + `Legacy txs need either v = 27/28 or v >= 37 (EIP-155 replay protection), got v = ${v}` + ) + } + } + let chainIdBN // No unsigned tx and EIP-155 activated and chain ID included if ( v !== undefined && - !v.eqn(0) && (!common || common.gteHardfork('spuriousDragon')) && !v.eqn(27) && !v.eqn(28) diff --git a/packages/tx/test/legacy.spec.ts b/packages/tx/test/legacy.spec.ts index 3397585b87..766b7d4a0b 100644 --- a/packages/tx/test/legacy.spec.ts +++ b/packages/tx/test/legacy.spec.ts @@ -389,8 +389,6 @@ tape('[Transaction]', function (t) { 'hex' ) - const fixtureTxSignedWithEIP155 = Transaction.fromTxData(txData).sign(privateKey) - const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.TangerineWhistle, @@ -400,9 +398,7 @@ tape('[Transaction]', function (t) { common, }).sign(privateKey) - let signedWithEIP155 = Transaction.fromTxData(fixtureTxSignedWithEIP155.toJSON()).sign( - privateKey - ) + let signedWithEIP155 = Transaction.fromTxData(txData).sign(privateKey) st.true(signedWithEIP155.verifySignature()) st.notEqual(signedWithEIP155.v?.toString('hex'), '1c') @@ -416,7 +412,7 @@ tape('[Transaction]', function (t) { st.notEqual(signedWithEIP155.v?.toString('hex'), '1c') st.notEqual(signedWithEIP155.v?.toString('hex'), '1b') - let signedWithoutEIP155 = Transaction.fromTxData(fixtureTxSignedWithEIP155.toJSON(), { + let signedWithoutEIP155 = Transaction.fromTxData(txData, { common, }).sign(privateKey) @@ -427,7 +423,7 @@ tape('[Transaction]', function (t) { "v shouldn't be EIP155 encoded" ) - signedWithoutEIP155 = Transaction.fromTxData(fixtureTxSignedWithoutEIP155.toJSON(), { + signedWithoutEIP155 = Transaction.fromTxData(txData, { common, }).sign(privateKey) @@ -442,6 +438,27 @@ tape('[Transaction]', function (t) { } ) + t.test( + 'constructor: throw on legacy transactions which have v != 27 and v != 28 and v < 37', + function (st) { + function getTxData(v: number) { + return { + v, + } + } + for (let n = 0; n < 27; n++) { + st.throws(() => Transaction.fromTxData(getTxData(n))) + } + st.throws(() => Transaction.fromTxData(getTxData(29))) + st.throws(() => Transaction.fromTxData(getTxData(36))) + + st.doesNotThrow(() => Transaction.fromTxData(getTxData(27))) + st.doesNotThrow(() => Transaction.fromTxData(getTxData(28))) + st.doesNotThrow(() => Transaction.fromTxData(getTxData(37))) + st.end() + } + ) + t.test('sign(), verifySignature(): sign tx with chainId specified in params', function (st) { const common = new Common({ chain: 42, hardfork: Hardfork.Petersburg }) let tx = Transaction.fromTxData({}, { common }) diff --git a/packages/util/src/signature.ts b/packages/util/src/signature.ts index aef0b12dcb..cea64fe6d6 100644 --- a/packages/util/src/signature.ts +++ b/packages/util/src/signature.ts @@ -46,6 +46,9 @@ export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any { function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN { const vBN = toType(v, TypeOutput.BN) + + if (vBN.eqn(0) || vBN.eqn(1)) return toType(v, TypeOutput.BN) + if (!chainId) { return vBN.subn(27) } @@ -60,6 +63,7 @@ function isValidSigRecovery(recovery: number | BN): boolean { /** * ECDSA public key recovery from signature. + * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions * @returns Recovered public key */ export const ecrecover = function ( @@ -80,6 +84,7 @@ export const ecrecover = function ( /** * Convert signature parameters into the format of `eth_sign` RPC method. + * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions * @returns Signature */ export const toRpcSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?: BNLike): string { @@ -94,6 +99,7 @@ export const toRpcSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?: BNL /** * Convert signature parameters into the format of Compact Signature Representation (EIP-2098). + * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions * @returns Signature */ export const toCompactSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?: BNLike): string { @@ -115,6 +121,8 @@ export const toCompactSig = function (v: BNLike, r: Buffer, s: Buffer, chainId?: /** * Convert signature format of the `eth_sign` RPC method to signature parameters * NOTE: all because of a bug in geth: https://github.com/ethereum/go-ethereum/issues/2053 + * NOTE: After EIP1559, `v` could be `0` or `1` but this function assumes + * it's a signed message (EIP-191 or EIP-712) adding `27` at the end. Remove if needed. */ export const fromRpcSig = function (sig: string): ECDSASignature { const buf: Buffer = toBuffer(sig) @@ -150,6 +158,7 @@ export const fromRpcSig = function (sig: string): ECDSASignature { /** * Validate a ECDSA signature. + * NOTE: Accepts `v == 0 | v == 1` for EIP1559 transactions * @param homesteadOrLater Indicates whether this is being used on either the homestead hardfork or a later one */ export const isValidSignature = function ( diff --git a/packages/util/test/signature.spec.ts b/packages/util/test/signature.spec.ts index 9163568be4..78fd9c83b7 100644 --- a/packages/util/test/signature.spec.ts +++ b/packages/util/test/signature.spec.ts @@ -150,6 +150,14 @@ tape('ecrecover', function (t) { st.ok(pubkey.equals(privateToPublic(ecprivkey))) st.end() }) + t.test('should recover a public key (v = 0)', function (st) { + const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') + const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') + const v = 0 + const pubkey = ecrecover(echash, v, r, s) + st.ok(pubkey.equals(privateToPublic(ecprivkey))) + st.end() + }) t.test('should fail on an invalid signature (v = 21)', function (st) { const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') @@ -312,6 +320,20 @@ tape('isValidSignature', function (t) { st.ok(isValidSignature(v, r, s)) st.end() }) + t.test('should work otherwise (v=0)', function (st) { + const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') + const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') + const v = 0 + st.ok(isValidSignature(v, r, s)) + st.end() + }) + t.test('should work otherwise (v=1)', function (st) { + const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') + const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') + const v = 1 + st.ok(isValidSignature(v, r, s)) + st.end() + }) t.test('should work otherwise (chainId=3)', function (st) { const r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') const s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') @@ -396,6 +418,18 @@ tape('message sig', function (t) { st.end() }) + t.test('should support compact signature representation (EIP-2098) (v=0)', function (st) { + const sig = + '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66' + st.equal(toCompactSig(0, r, s), sig) + st.deepEqual(fromRpcSig(sig), { + v: 27, + r, + s, + }) + st.end() + }) + t.test('should support compact signature representation 2 (EIP-2098)', function (st) { const sig = '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9929ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66' @@ -408,6 +442,18 @@ tape('message sig', function (t) { st.end() }) + t.test('should support compact signature representation 2 (EIP-2098) (v=1)', function (st) { + const sig = + '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9929ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66' + st.equal(toCompactSig(1, r, s), sig) + st.deepEqual(fromRpcSig(sig), { + v: 28, + r, + s, + }) + st.end() + }) + t.test('should return hex strings that the RPC can use (chainId=150)', function (st) { const sig = '0x99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66014f' @@ -478,7 +524,7 @@ tape('message sig', function (t) { t.test('should throw on invalid v value', function (st) { st.throws(function () { - toRpcSig(1, r, s) + toRpcSig(2, r, s) }) st.end() }) diff --git a/packages/vm/src/evm/precompiles/01-ecrecover.ts b/packages/vm/src/evm/precompiles/01-ecrecover.ts index 0557210d45..8d5a96d249 100644 --- a/packages/vm/src/evm/precompiles/01-ecrecover.ts +++ b/packages/vm/src/evm/precompiles/01-ecrecover.ts @@ -16,6 +16,18 @@ export default function (opts: PrecompileInput): ExecResult { const msgHash = data.slice(0, 32) const v = data.slice(32, 64) + const vBN = new BN(v) + + // Guard against util's `ecrecover`: without providing chainId this will return + // a signature in most of the cases in the cases that `v=0` or `v=1` + // However, this should throw, only 27 and 28 is allowed as input + if (!vBN.eqn(27) && !vBN.eqn(28)) { + return { + gasUsed, + returnValue: Buffer.alloc(0), + } + } + const r = data.slice(64, 96) const s = data.slice(96, 128)