From 4bb6c2fffb61b44fac07ea3d8a937e2d61ea30cd Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Sun, 13 Sep 2020 09:30:51 +0800 Subject: [PATCH] fix: ensure signatures are ordered correctly (#12165) --- web3.js/src/message.js | 10 ++ web3.js/src/transaction.js | 76 ++++++++++----- web3.js/test/bpf-loader.test.js | 21 ---- web3.js/test/transaction.test.js | 158 ++++++++++++++++++++++++++----- 4 files changed, 196 insertions(+), 69 deletions(-) diff --git a/web3.js/src/message.js b/web3.js/src/message.js index 444381c1ec..84857c0cb6 100644 --- a/web3.js/src/message.js +++ b/web3.js/src/message.js @@ -83,6 +83,16 @@ export class Message { ); } + findSignerIndex(signer: PublicKey): number { + const index = this.accountKeys.findIndex(accountKey => { + return accountKey.equals(signer); + }); + if (index < 0) { + throw new Error(`unknown signer: ${signer.toString()}`); + } + return index; + } + serialize(): Buffer { const numKeys = this.accountKeys.length; diff --git a/web3.js/src/transaction.js b/web3.js/src/transaction.js index 8b1d08e26f..8ef22778dd 100644 --- a/web3.js/src/transaction.js +++ b/web3.js/src/transaction.js @@ -220,14 +220,11 @@ export class Transaction { throw new Error('Transaction feePayer required'); } - let numReadonlySignedAccounts = 0; - let numReadonlyUnsignedAccounts = 0; - const programIds: string[] = []; const accountMetas: AccountMeta[] = []; this.instructions.forEach(instruction => { instruction.keys.forEach(accountMeta => { - accountMetas.push(accountMeta); + accountMetas.push({...accountMeta}); }); const programId = instruction.programId.toString(); @@ -268,16 +265,17 @@ export class Transaction { } }); - // Move payer to the front and append other unknown signers as read-only + // Move payer to the front and disallow unknown signers this.signatures.forEach((signature, signatureIndex) => { const isPayer = signatureIndex === 0; const uniqueIndex = uniqueMetas.findIndex(x => { return x.pubkey.equals(signature.publicKey); }); if (uniqueIndex > -1) { - if (isPayer && uniqueIndex !== 0) { + if (isPayer) { const [payerMeta] = uniqueMetas.splice(uniqueIndex, 1); payerMeta.isSigner = true; + payerMeta.isWritable = true; uniqueMetas.unshift(payerMeta); } else { uniqueMetas[uniqueIndex].isSigner = true; @@ -289,23 +287,22 @@ export class Transaction { isWritable: true, }); } else { - uniqueMetas.push({ - pubkey: signature.publicKey, - isSigner: true, - isWritable: false, - }); + throw new Error(`unknown signer: ${signature.publicKey.toString()}`); } }); - // Split out signing from non-signing keys and count read-only keys + let numRequiredSignatures = 0; + let numReadonlySignedAccounts = 0; + let numReadonlyUnsignedAccounts = 0; + + // Split out signing from non-signing keys and count header values const signedKeys: string[] = []; const unsignedKeys: string[] = []; uniqueMetas.forEach(({pubkey, isSigner, isWritable}) => { if (isSigner) { - // Promote the first signer to writable as it is the fee payer - const first = signedKeys.length === 0; signedKeys.push(pubkey.toString()); - if (!first && !isWritable) { + numRequiredSignatures += 1; + if (!isWritable) { numReadonlySignedAccounts += 1; } } else { @@ -316,6 +313,10 @@ export class Transaction { } }); + if (numRequiredSignatures !== this.signatures.length) { + throw new Error('missing signer(s)'); + } + const accountKeys = signedKeys.concat(unsignedKeys); const instructions: CompiledInstruction[] = this.instructions.map( instruction => { @@ -337,7 +338,7 @@ export class Transaction { return new Message({ header: { - numRequiredSignatures: signedKeys.length, + numRequiredSignatures, numReadonlySignedAccounts, numReadonlyUnsignedAccounts, }, @@ -365,7 +366,18 @@ export class Transaction { throw new Error('No signers'); } - this.signatures = signers.map(publicKey => ({signature: null, publicKey})); + const seen = new Set(); + this.signatures = signers + .filter(publicKey => { + const key = publicKey.toString(); + if (seen.has(key)) { + return false; + } else { + seen.add(key); + return true; + } + }) + .map(publicKey => ({signature: null, publicKey})); } /** @@ -385,10 +397,21 @@ export class Transaction { throw new Error('No signers'); } - this.signatures = signers.map(signer => ({ - signature: null, - publicKey: signer.publicKey, - })); + const seen = new Set(); + this.signatures = signers + .filter(signer => { + const key = signer.publicKey.toString(); + if (seen.has(key)) { + return false; + } else { + seen.add(key); + return true; + } + }) + .map(signer => ({ + signature: null, + publicKey: signer.publicKey, + })); this.partialSign(...signers); } @@ -404,7 +427,14 @@ export class Transaction { throw new Error('No signers'); } - const signData = this.serializeMessage(); + const message = this.compileMessage(); + this.signatures.sort(function (x, y) { + const xIndex = message.findSignerIndex(x.publicKey); + const yIndex = message.findSignerIndex(y.publicKey); + return xIndex < yIndex ? -1 : 1; + }); + + const signData = message.serialize(); signers.forEach(signer => { const signature = nacl.sign.detached(signData, signer.secretKey); this.addSignature(signer.publicKey, signature); @@ -422,7 +452,7 @@ export class Transaction { pubkey.equals(sigpair.publicKey), ); if (index < 0) { - throw new Error(`Unknown signer: ${pubkey.toString()}`); + throw new Error(`unknown signer: ${pubkey.toString()}`); } this.signatures[index].signature = Buffer.from(signature); diff --git a/web3.js/test/bpf-loader.test.js b/web3.js/test/bpf-loader.test.js index d81830cd24..6127a3bb78 100644 --- a/web3.js/test/bpf-loader.test.js +++ b/web3.js/test/bpf-loader.test.js @@ -196,25 +196,4 @@ describe('load BPF Rust program', () => { expect(logs.length).toEqual(0); }); - - test('simulate transaction with bad signer', async () => { - const simulatedTransaction = new Transaction().add({ - keys: [ - {pubkey: payerAccount.publicKey, isSigner: true, isWritable: true}, - ], - programId: program.publicKey, - }); - - const {err, logs} = ( - await connection.simulateTransaction(simulatedTransaction, [program]) - ).value; - expect(err).toEqual('SanitizeFailure'); - - if (logs === null) { - expect(logs).not.toBeNull(); - return; - } - - expect(logs.length).toEqual(0); - }); }); diff --git a/web3.js/test/transaction.test.js b/web3.js/test/transaction.test.js index 2748fb4863..48f6bd3a89 100644 --- a/web3.js/test/transaction.test.js +++ b/web3.js/test/transaction.test.js @@ -10,6 +10,33 @@ import {SystemProgram} from '../src/system-program'; import {Message} from '../src/message'; describe('compileMessage', () => { + test('accountKeys are ordered', () => { + const payer = new Account(); + const account2 = new Account(); + const account3 = new Account(); + const recentBlockhash = new Account().publicKey.toBase58(); + const programId = new Account().publicKey; + const transaction = new Transaction({recentBlockhash}).add({ + keys: [ + {pubkey: account3.publicKey, isSigner: true, isWritable: false}, + {pubkey: payer.publicKey, isSigner: true, isWritable: true}, + {pubkey: account2.publicKey, isSigner: true, isWritable: true}, + ], + programId, + }); + + transaction.setSigners( + payer.publicKey, + account2.publicKey, + account3.publicKey, + ); + + const message = transaction.compileMessage(); + expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true); + expect(message.accountKeys[1].equals(account2.publicKey)).toBe(true); + expect(message.accountKeys[2].equals(account3.publicKey)).toBe(true); + }); + test('payer is first account meta', () => { const payer = new Account(); const other = new Account(); @@ -32,6 +59,48 @@ describe('compileMessage', () => { expect(message.header.numReadonlyUnsignedAccounts).toEqual(1); }); + test('validation', () => { + const payer = new Account(); + const other = new Account(); + const recentBlockhash = new Account().publicKey.toBase58(); + const programId = new Account().publicKey; + + const transaction = new Transaction(); + expect(() => { + transaction.compileMessage(); + }).toThrow('Transaction recentBlockhash required'); + + transaction.recentBlockhash = recentBlockhash; + + expect(() => { + transaction.compileMessage(); + }).toThrow('No instructions provided'); + + transaction.add({ + keys: [ + {pubkey: other.publicKey, isSigner: true, isWritable: true}, + {pubkey: payer.publicKey, isSigner: true, isWritable: true}, + ], + programId, + }); + + expect(() => { + transaction.compileMessage(); + }).toThrow('Transaction feePayer required'); + + transaction.setSigners(payer.publicKey); + + expect(() => { + transaction.compileMessage(); + }).toThrow('missing signer'); + + transaction.setSigners(payer.publicKey, new Account().publicKey); + + expect(() => { + transaction.compileMessage(); + }).toThrow('unknown signer'); + }); + test('payer is writable', () => { const payer = new Account(); const recentBlockhash = new Account().publicKey.toBase58(); @@ -48,31 +117,6 @@ describe('compileMessage', () => { expect(message.header.numReadonlySignedAccounts).toEqual(0); expect(message.header.numReadonlyUnsignedAccounts).toEqual(1); }); - - test('signers are ordered and only writable if necessary', () => { - const payer = new Account(); - const account1 = new Account(); - const account2 = new Account(); - const recentBlockhash = new Account().publicKey.toBase58(); - const programId = new Account().publicKey; - const transaction = new Transaction({recentBlockhash}).add({ - keys: [ - {pubkey: payer.publicKey, isSigner: true, isWritable: false}, - {pubkey: account1.publicKey, isSigner: false, isWritable: true}, - ], - programId, - }); - - // account2 is an extra signer, not involved in any instructions - transaction.sign(payer, account1, account2); - const message = transaction.compileMessage(); - expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true); - expect(message.accountKeys[1].equals(account1.publicKey)).toBe(true); - expect(message.accountKeys[2].equals(account2.publicKey)).toBe(true); - expect(message.header.numRequiredSignatures).toEqual(3); - expect(message.header.numReadonlySignedAccounts).toEqual(1); - expect(message.header.numReadonlyUnsignedAccounts).toEqual(1); - }); }); test('partialSign', () => { @@ -97,6 +141,70 @@ test('partialSign', () => { expect(partialTransaction).toEqual(transaction); }); +describe('dedupe', () => { + const payer = new Account(); + const duplicate1 = payer; + const duplicate2 = payer; + const recentBlockhash = new Account().publicKey.toBase58(); + const programId = new Account().publicKey; + + test('setSigners', () => { + const transaction = new Transaction({recentBlockhash}).add({ + keys: [ + {pubkey: duplicate1.publicKey, isSigner: true, isWritable: true}, + {pubkey: payer.publicKey, isSigner: false, isWritable: true}, + {pubkey: duplicate2.publicKey, isSigner: true, isWritable: false}, + ], + programId, + }); + + transaction.setSigners( + payer.publicKey, + duplicate1.publicKey, + duplicate2.publicKey, + ); + + expect(transaction.signatures.length).toEqual(1); + expect(transaction.signatures[0].publicKey.equals(payer.publicKey)).toBe( + true, + ); + + const message = transaction.compileMessage(); + expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true); + expect(message.header.numRequiredSignatures).toEqual(1); + expect(message.header.numReadonlySignedAccounts).toEqual(0); + expect(message.header.numReadonlyUnsignedAccounts).toEqual(1); + + transaction.signatures; + }); + + test('sign', () => { + const transaction = new Transaction({recentBlockhash}).add({ + keys: [ + {pubkey: duplicate1.publicKey, isSigner: true, isWritable: true}, + {pubkey: payer.publicKey, isSigner: false, isWritable: true}, + {pubkey: duplicate2.publicKey, isSigner: true, isWritable: false}, + ], + programId, + }); + + transaction.sign(payer, duplicate1, duplicate2); + + expect(transaction.signatures.length).toEqual(1); + expect(transaction.signatures[0].publicKey.equals(payer.publicKey)).toBe( + true, + ); + + const message = transaction.compileMessage(); + expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true); + expect(message.header.numRequiredSignatures).toEqual(1); + expect(message.header.numReadonlySignedAccounts).toEqual(0); + expect(message.header.numReadonlyUnsignedAccounts).toEqual(1); + + transaction.signatures; + }); +}); + test('transfer signatures', () => { const account1 = new Account(); const account2 = new Account();