fix(crypto-transaction): bind legacy second signature to signed transaction

packages/contracts/source/contracts/crypto/transactions.ts

@@ -81,7 +81,7 @@ export interface TransactionVerifier {
 	): Promise<SchemaValidationResult<TransactionUnsignedSerializable>>;
 	verifySchemaSigned(data: TransactionSerializable): Promise<SchemaValidationResult<TransactionSerializable>>;
 	verifySchemaStrict(data: TransactionData): Promise<SchemaValidationResult<TransactionData>>;
-	verifyLegacySecondSignature(data: TransactionSerializable, legacySecondPublicKey: string): Promise<boolean>;
+	verifyLegacySecondSignature(data: TransactionData, legacySecondPublicKey: string): Promise<boolean>;
 }
 
 export interface TransactionSigner {
@@ -90,11 +90,7 @@ export interface TransactionSigner {
 		keys: KeyPair,
 		options?: SerializeOptions,
 	): Promise<EcdsaSignature>;
-	legacySecondSign(
-		transaction: TransactionUnsignedSerializable,
-		keys: KeyPair,
-		options?: SerializeOptions,
-	): Promise<string>;
+	legacySecondSign(transaction: TransactionSerializable, keys: KeyPair, options?: SerializeOptions): Promise<string>;
 }
 
 export interface TransactionSerializer {
@@ -119,6 +115,7 @@ export interface TransactionFactory {
 export interface TransactionHashFactory {
 	toHashUnsigned(transaction: TransactionUnsignedSerializable): Promise<Buffer>;
 	toHash(transaction: TransactionSerializable): Promise<Buffer>;
+	toLegacySecondSignatureHash(transactionHash: Buffer): Buffer;
 }
 
 export type TransactionSchema = Record<string, unknown>;

packages/crypto-signature-ecdsa/source/signature.test.ts

@@ -35,4 +35,71 @@ describe("Signature", ({ assert, it }) => {
 
 		assert.false(await new Signature().verifyRecoverable(signature, message, publicKey));
 	});
+
+	it("#verifyRecoverable should return false for the high s form of a signature", async () => {
+		const signature = {
+			r: "66f1c6d9fe13834f6e348aae40426060339ed8cba7d9b2f105c8220be095877c",
+			s: "1368fffd8294f1e22086703d33511fc8bb25231d6e9dc64d6449035003184bdd",
+			v: 1,
+		};
+
+		const message = Buffer.from("64726e3da8", "hex");
+		const publicKey = Buffer.from("03e84093c072af70004a38dd95e34def119d2348d5261228175d032e5f2070e19f", "hex");
+
+		assert.true(await new Signature().verifyRecoverable(signature, message, publicKey));
+		assert.false(
+			await new Signature().verifyRecoverable(
+				{ ...signature, s: "ec9700027d6b0e1ddf798fc2ccaee035ff89b9c940aad9ee5b895b3ccd1df564", v: 0 },
+				message,
+				publicKey,
+			),
+		);
+	});
+
+	it("#verifyRecoverable should return false if v is not the recovery id", async () => {
+		const signature = {
+			r: "66f1c6d9fe13834f6e348aae40426060339ed8cba7d9b2f105c8220be095877c",
+			s: "1368fffd8294f1e22086703d33511fc8bb25231d6e9dc64d6449035003184bdd",
+			v: 1,
+		};
+
+		const message = Buffer.from("64726e3da8", "hex");
+		const publicKey = Buffer.from("03e84093c072af70004a38dd95e34def119d2348d5261228175d032e5f2070e19f", "hex");
+
+		assert.true(await new Signature().verifyRecoverable(signature, message, publicKey));
+
+		for (const v of [0, 2, 3, 27]) {
+			assert.false(await new Signature().verifyRecoverable({ ...signature, v }, message, publicKey));
+		}
+	});
+
+	it("#verifyRecoverable should return false if no public key is recovered", async () => {
+		const signature = {
+			r: "66f1c6d9fe13834f6e348aae40426060339ed8cba7d9b2f105c8220be095877c",
+			s: "1368fffd8294f1e22086703d33511fc8bb25231d6e9dc64d6449035003184bdd",
+			v: 1,
+		};
+
+		const message = Buffer.from("64726e3da8", "hex");
+		const publicKey = Buffer.from("03e84093c072af70004a38dd95e34def119d2348d5261228175d032e5f2070e19f", "hex");
+
+		assert.true(await new Signature().verifyRecoverable(signature, message, publicKey));
+		assert.false(await new Signature().verifyRecoverable({ ...signature, r: "00".repeat(32) }, message, publicKey));
+	});
+
+	it("#verifyRecoverable should verify an uncompressed public key", async () => {
+		const signature = {
+			r: "66f1c6d9fe13834f6e348aae40426060339ed8cba7d9b2f105c8220be095877c",
+			s: "1368fffd8294f1e22086703d33511fc8bb25231d6e9dc64d6449035003184bdd",
+			v: 1,
+		};
+
+		const message = Buffer.from("64726e3da8", "hex");
+		const publicKey = Buffer.from(
+			"04e84093c072af70004a38dd95e34def119d2348d5261228175d032e5f2070e19f9e02d3df88d4bb625c9a207438037f2ab6a820c143db43baa552bb980dbd8095",
+			"hex",
+		);
+
+		assert.true(await new Signature().verifyRecoverable(signature, message, publicKey));
+	});
 });

packages/crypto-signature-ecdsa/source/signature.ts

@@ -3,6 +3,8 @@ import type { Contracts } from "@mainsail/contracts";
 import { injectable } from "@mainsail/container";
 import { secp256k1 } from "bcrypto";
 
+const RECOVERY_IDS = new Set([0, 1]);
+
 @injectable()
 export class Signature implements Contracts.Crypto.SignatureEcdsa {
 	public async signRecoverable(message: Buffer, privateKey: Buffer): Promise<Contracts.Crypto.EcdsaSignature> {
@@ -20,11 +22,18 @@ export class Signature implements Contracts.Crypto.SignatureEcdsa {
 		message: Buffer,
 		publicKey: Buffer,
 	): Promise<boolean> {
-		if (!this.isLowS(signature)) {
+		if (!this.isLowS(signature) || !RECOVERY_IDS.has(signature.v)) {
 			return false;
 		}
 
-		return secp256k1.verify(message, Buffer.from(signature.r + signature.s, "hex"), publicKey);
+		const recovered = secp256k1.recover(
+			message,
+			Buffer.from(signature.r + signature.s, "hex"),
+			signature.v,
+			publicKey.length === 33,
+		);
+
+		return recovered !== null && recovered.equals(publicKey);
 	}
 
 	public isLowS(signature: Contracts.Crypto.EcdsaSignature): boolean {

packages/crypto-transaction/source/builder.test.ts

@@ -87,35 +87,35 @@ describe<{
 			r: "8f0145edea568df2dd39db91be0bff4ebf5b1e54cae49bf2090bf84fa0dd45a2",
 			s: "273f828aaa99a54e31f8f3e316acc573c6f2490fb903052accddb979647fa5ce",
 			legacySecondSignature:
-				"8f0145edea568df2dd39db91be0bff4ebf5b1e54cae49bf2090bf84fa0dd45a2273f828aaa99a54e31f8f3e316acc573c6f2490fb903052accddb979647fa5ce01",
+				"73a17206c6ec232c8041de9241573c4cfe8bd629c32d02618ee1aec550af3b1e66798e7e356e1f11a60552646ffbee9df960ec847cdcc9e3e04d54c0fffdfcf900",
 		};
 
 		const serialized =
-			"f8968085012a05f200830f4240808080824e44a08f0145edea568df2dd39db91be0bff4ebf5b1e54cae49bf2090bf84fa0dd45a2a0273f828aaa99a54e31f8f3e316acc573c6f2490fb903052accddb979647fa5ceb8418f0145edea568df2dd39db91be0bff4ebf5b1e54cae49bf2090bf84fa0dd45a2273f828aaa99a54e31f8f3e316acc573c6f2490fb903052accddb979647fa5ce01";
+			"f8968085012a05f200830f4240808080824e44a08f0145edea568df2dd39db91be0bff4ebf5b1e54cae49bf2090bf84fa0dd45a2a0273f828aaa99a54e31f8f3e316acc573c6f2490fb903052accddb979647fa5ceb84173a17206c6ec232c8041de9241573c4cfe8bd629c32d02618ee1aec550af3b1e66798e7e356e1f11a60552646ffbee9df960ec847cdcc9e3e04d54c0fffdfcf900";
 
 		let builder = app.resolve(TransactionBuilder);
-		await builder.legacySecondSign(wallet.passphrase);
 		await builder.sign(wallet.passphrase);
+		await builder.legacySecondSign(wallet.passphrase);
 		const txSignedWithPassphrase = await builder.build();
 		assert.equal(txSignedWithPassphrase.toData(), transaction);
 		assert.equal(txSignedWithPassphrase.serialized.toString("hex"), serialized);
 		assert.equal(await builder.getStruct(), transaction);
 
 		builder = app.resolve(TransactionBuilder);
+		await builder.sign(wallet.passphrase);
 		await builder.legacySecondSignWithKeyPair({
 			publicKey: wallet.publicKey,
 			privateKey: wallet.privateKey,
 			compressed: false,
 		});
-		await builder.sign(wallet.passphrase);
 		const txSignedWithKeyPair = await builder.build();
 		assert.equal(txSignedWithKeyPair.toData(), transaction);
 		assert.equal(txSignedWithKeyPair.serialized.toString("hex"), serialized);
 		assert.equal(await builder.getStruct(), transaction);
 
 		builder = app.resolve(TransactionBuilder);
-		await builder.legacySecondSignWithWif(wallet.WIF);
 		await builder.sign(wallet.passphrase);
+		await builder.legacySecondSignWithWif(wallet.WIF);
 		const txSignedWithWif = await builder.build();
 		assert.equal(txSignedWithWif.toData(), transaction);
 		assert.equal(txSignedWithWif.serialized.toString("hex"), serialized);
@@ -132,6 +132,10 @@ describe<{
 		await assert.rejects(() => builder.nonce("-1").legacySecondSign(wallet.passphrase), ValidationFailed);
 	});
 
+	it("#legacySecondSign - should throw if not signed", async ({ builder }) => {
+		await assert.rejects(() => builder.legacySecondSign(wallet.passphrase), MissingTransactionSignatureError);
+	});
+
 	it("#getStruct - should throw on missing data", async ({ app }) => {
 		let builder = app.resolve(TransactionBuilder);
 		await assert.rejects(() => builder.getStruct(), MissingTransactionSignatureError);

packages/crypto-transaction/source/builder.ts

@@ -185,7 +185,11 @@ export class TransactionBuilder {
 			throw new ValidationFailed(error);
 		}
 
-		const signature = await this.signer.legacySecondSign(data, keys);
+		if (!this.data.r || !this.data.s) {
+			throw new MissingTransactionSignatureError();
+		}
+
+		const signature = await this.signer.legacySecondSign(this.data, keys);
 
 		this.data.legacySecondSignature = signature;
 

packages/crypto-transaction/source/hash.factory.test.ts

@@ -33,6 +33,14 @@ describe<{
 		}
 	});
 
+	it("#toLegacySecondSignatureHash - should be ok", ({ hasher }) => {
+		const hash = hasher.toLegacySecondSignatureHash(
+			Buffer.from("64ff1ac71a06deb6a2d544786f8751b534c1c38284ccfa5faf2565d19b9f5151", "hex"),
+		);
+
+		assert.equal(hash.toString("hex"), "b5c9157468c8204f03389b61407f3caa91b76b28bcc158c9ab347984feb4c331");
+	});
+
 	it("#toHashUnsigned - should be ok", async ({ hasher }) => {
 		for (const [tx, expectedHash] of [
 			[Deserialized.transactionContractCall, "a037cecd15ad24cbccd89b7610f6d7e80482b935798230d3b22c897c91bddc51"],

packages/crypto-transaction/source/hash.factory.ts

@@ -3,6 +3,9 @@ import type { Contracts } from "@mainsail/contracts";
 import { Identifiers } from "@mainsail/constants";
 import { inject, injectable } from "@mainsail/container";
 
+// 0x8f cannot start a transaction or signed-message preimage (0x00-0x7f, 0xc0-0xff)
+const LEGACY_SECOND_SIGNATURE_DOMAIN = Buffer.concat([Buffer.from([0x8f]), Buffer.from("MAINSAIL_LSS_V1")]);
+
 @injectable()
 export class HashFactory implements Contracts.Crypto.TransactionHashFactory {
 	@inject(Identifiers.Cryptography.Transaction.Serializer)
@@ -14,7 +17,7 @@ export class HashFactory implements Contracts.Crypto.TransactionHashFactory {
 	public async toHash(transaction: Contracts.Crypto.TransactionSerializable): Promise<Buffer> {
 		const serialized = await this.serializer.serialize({
 			...transaction,
-			legacySecondSignature: undefined, // TODO: Decide if legacySecondSignature should be part of the hash or not. For now, we exclude it to maintain compatibility with existing hashes.
+			legacySecondSignature: undefined, // excluded, the legacy second signature signs this hash
 		});
 		return this.hashFactory.keccak256(serialized);
 	}
@@ -23,4 +26,8 @@ export class HashFactory implements Contracts.Crypto.TransactionHashFactory {
 		const serialized = await this.serializer.serializeUnsigned(transaction);
 		return this.hashFactory.keccak256(serialized);
 	}
+
+	public toLegacySecondSignatureHash(transactionHash: Buffer): Buffer {
+		return this.hashFactory.keccak256([LEGACY_SECOND_SIGNATURE_DOMAIN, transactionHash]);
+	}
 }

packages/crypto-transaction/source/signer.test.ts

@@ -4,11 +4,13 @@ import { Identifiers } from "@mainsail/constants";
 import { TransactionBuilder } from "../source/builder.js";
 import { Application } from "@mainsail/kernel";
 import { describe } from "@mainsail/test-runner";
+import { wallet } from "../test/fixtures/index";
 import { prepareSandbox } from "../test/helpers/prepare-sandbox";
 
 describe<{
 	app: Application;
 	signer: Contracts.Crypto.TransactionSigner;
+	factory: Contracts.Crypto.TransactionFactory;
 	keyPair: Contracts.Crypto.KeyPair;
 	transaction: Contracts.Crypto.Transaction;
 }>("Signer", ({ it, beforeEach, assert }) => {
@@ -18,6 +20,9 @@ describe<{
 		context.signer = context.app.get<Contracts.Crypto.TransactionSigner>(
 			Identifiers.Cryptography.Transaction.Signer,
 		);
+		context.factory = context.app.get<Contracts.Crypto.TransactionFactory>(
+			Identifiers.Cryptography.Transaction.Factory,
+		);
 
 		context.keyPair = await context.app
 			.getTagged<Contracts.Crypto.KeyPairFactory>(
@@ -53,7 +58,28 @@ describe<{
 
 		assert.equal(
 			signature,
-			"295ffb1befa5259bba46d532affa13f52f1e50f9418a2579982b121b4ef3553a1fe13d077cbcd6f2293d41c66eb5e5dee4e2bf3b8f8eb3e0556304befbbb69bb01",
+			"963e726eb3bc5f68a1bbf1c25c4324a3ff93c7853e70fc4a890665982c05ae877270a93df25d9ffdb1df2f3bd5a8816287bb9a10f85428970ac25f9ce6dacf4f00",
 		);
 	});
+
+	it("should sign legacy signature that is not a primary signature of the second key", async ({
+		factory,
+		signer,
+		transaction,
+	}) => {
+		const legacySecondSignature = await signer.legacySecondSign(transaction, {
+			compressed: false,
+			privateKey: wallet.privateKey,
+			publicKey: wallet.publicKey,
+		});
+
+		const replayed = await factory.fromData({
+			...transaction.toData(),
+			r: legacySecondSignature.slice(0, 64),
+			s: legacySecondSignature.slice(64, 128),
+			v: Number.parseInt(legacySecondSignature.slice(128), 16),
+		});
+
+		assert.not.equal(replayed.senderPublicKey, wallet.publicKey);
+	});
 });

packages/crypto-transaction/source/signer.ts

@@ -22,10 +22,10 @@ export class Signer implements Contracts.Crypto.TransactionSigner {
 	}
 
 	public async legacySecondSign(
-		transaction: Contracts.Crypto.TransactionUnsignedSerializable,
+		transaction: Contracts.Crypto.TransactionSerializable,
 		keys: Contracts.Crypto.KeyPair,
 	): Promise<string> {
-		const hash: Buffer = await this.hashFactory.toHashUnsigned(transaction);
+		const hash: Buffer = this.hashFactory.toLegacySecondSignatureHash(await this.hashFactory.toHash(transaction));
 		const { r, s, v } = await this.signatureFactory.signRecoverable(hash, Buffer.from(keys.privateKey, "hex"));
 
 		return formatEcdsaSignature(r, s, v);