diff --git a/packages/snap/CHANGELOG.md b/packages/snap/CHANGELOG.md index 3025bde4..0d278ddf 100644 --- a/packages/snap/CHANGELOG.md +++ b/packages/snap/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Validate that account IDs passed to `keyring_setSelectedAccounts` belong to the snap, rejecting unknown IDs with `InvalidParamsError` ([#604](https://github.com/MetaMask/snap-solana-wallet/pull/604)) +### Fixed + +- **BREAKING:** Preserve dapp-origin `signTransaction` and `signAndSendTransaction` payloads by signing the decoded transaction directly ([#608](https://github.com/MetaMask/snap-solana-wallet/pull/608)) + ## [2.8.0] ### Added diff --git a/packages/snap/snap.manifest.json b/packages/snap/snap.manifest.json index 26c0286f..2485c430 100644 --- a/packages/snap/snap.manifest.json +++ b/packages/snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snap-solana-wallet.git" }, "source": { - "shasum": "AzXzPnYm37l2ijVy4ZM6GRu3DouI8hs75QHbfXJ/sBQ=", + "shasum": "dQYNwGUz41FQ0ODk5AFIcEgBsLkDgK2DS19eD2s8a0Q=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/snap/src/core/sdk-extensions/codecs.test.ts b/packages/snap/src/core/sdk-extensions/codecs.test.ts index f49efd88..ec0f8003 100644 --- a/packages/snap/src/core/sdk-extensions/codecs.test.ts +++ b/packages/snap/src/core/sdk-extensions/codecs.test.ts @@ -14,6 +14,7 @@ import { fromBytesToCompilableTransactionMessage, fromCompilableTransactionMessageToBase64String, fromTransactionToBase64String, + fromUnknowBase64StringToTransaction, fromUnknowBase64StringToTransactionOrTransactionMessage, } from './codecs'; @@ -93,6 +94,35 @@ describe('codecs', () => { }); }); + describe(`fromUnknowBase64StringToTransaction | Scenario: ${name}`, () => { + it('wraps bare compiled message bytes without changing the message bytes', async () => { + const transactionMessageBytes = pipe( + transactionMessage, + compileTransactionMessage, + getCompiledTransactionMessageEncoder().encode, + ); + + const result = await fromUnknowBase64StringToTransaction( + transactionMessageBase64Encoded, + ); + + expect(result.messageBytes).toStrictEqual(transactionMessageBytes); + expect(Object.values(result.signatures)).toStrictEqual( + Object.keys(result.signatures).map(() => null), + ); + }); + + it('decodes a full transaction without changing the message bytes', async () => { + const result = await fromUnknowBase64StringToTransaction( + signedTransactionBase64Encoded, + ); + + const { lifetimeConstraint, ...rest } = signedTransaction; + + expect(result).toStrictEqual(rest); + }); + }); + describe('fromUnknowBase64StringToTransactionOrTransactionMessage', () => { it('decodes a base64 encoded transaction message to a transaction message', async () => { const result = diff --git a/packages/snap/src/core/sdk-extensions/codecs.ts b/packages/snap/src/core/sdk-extensions/codecs.ts index 5d06954d..1603dfa9 100644 --- a/packages/snap/src/core/sdk-extensions/codecs.ts +++ b/packages/snap/src/core/sdk-extensions/codecs.ts @@ -124,6 +124,35 @@ export const fromBase64StringToTransaction = async ( ): Promise => pipe(base64String, getBase64Encoder().encode, getTransactionDecoder().decode); +/** + * Decodes a base64 encoded compiled transaction message to a Transaction shape + * without changing the original message bytes. + * + * @param base64String - The base64 encoded compiled transaction message. + * @returns A transaction with null signature slots for all required signers. + */ +export const fromBase64StringCompiledMessageToTransaction = async ( + base64String: Infer, +): Promise => { + const messageBytes = getBase64Encoder().encode( + base64String, + ) as TransactionMessageBytes; + + const compiledTransactionMessage = + getCompiledTransactionMessageDecoder().decode(messageBytes); + + const signerAccounts = compiledTransactionMessage.staticAccounts + .slice(0, compiledTransactionMessage.header.numSignerAccounts) + .map((address) => [address, null]); + + const signatures = Object.fromEntries(signerAccounts); + + return { + messageBytes, + signatures, + }; +}; + /** * Decodes a base64 string to a transaction or a compilable transaction message. * @@ -141,3 +170,17 @@ export const fromUnknowBase64StringToTransactionOrTransactionMessage = async ( fromBase64StringToTransaction(base64String), fromBase64StringToCompilableTransactionMessage(base64String, rpc, config), ]); + +/** + * Decodes a base64 string to a transaction. + * + * @param base64String - The base64 string to decode that represents either a transaction or a transaction message. + * @returns The decoded transaction. + */ +export const fromUnknowBase64StringToTransaction = async ( + base64String: Infer, +): Promise => + PromiseAny([ + fromBase64StringToTransaction(base64String), + fromBase64StringCompiledMessageToTransaction(base64String), + ]); diff --git a/packages/snap/src/core/services/signer/Signer.test.ts b/packages/snap/src/core/services/signer/Signer.test.ts index f45c0460..f3f50f24 100644 --- a/packages/snap/src/core/services/signer/Signer.test.ts +++ b/packages/snap/src/core/services/signer/Signer.test.ts @@ -3,6 +3,7 @@ /* eslint-disable @typescript-eslint/no-var-requires */ /* eslint-disable @typescript-eslint/no-require-imports */ import { + getBase64Encoder, getSignatureFromTransaction, isTransactionMessageWithBlockhashLifetime, } from '@solana/kit'; @@ -121,6 +122,25 @@ describe('Signer', () => { ), ).toBe(true); }); + + it(`Scenario ${name}: preserves message bytes when requested`, async () => { + const originalMessageBytes = getBase64Encoder().encode( + transactionMessageBase64Encoded, + ); + + const result = await signer.partiallySignBase64String( + transactionMessageBase64Encoded, + fromAccount, + scope, + undefined, + true, + ); + + expect(result.messageBytes).toStrictEqual(originalMessageBytes); + expect(Object.values(result.signatures)).toStrictEqual([ + expect.any(Uint8Array), + ]); + }); }); }); }); diff --git a/packages/snap/src/core/services/signer/Signer.ts b/packages/snap/src/core/services/signer/Signer.ts index 51211f2e..d143d2e7 100644 --- a/packages/snap/src/core/services/signer/Signer.ts +++ b/packages/snap/src/core/services/signer/Signer.ts @@ -19,6 +19,7 @@ import type { Network } from '../../constants/solana'; import type { DecompileTransactionMessageFetchingLookupTablesConfig } from '../../sdk-extensions/codecs'; import { fromBytesToCompilableTransactionMessage, + fromUnknowBase64StringToTransaction, fromUnknowBase64StringToTransactionOrTransactionMessage, } from '../../sdk-extensions/codecs'; import { @@ -60,6 +61,7 @@ export class Signer { * @param account - The account to sign the transaction or transaction message with. * @param network - The network on which the transaction is being sent. * @param config - The configuration for the request. + * @param preserveMessageBytes - Whether to preserve the original message bytes. * @returns The signed transaction. * @throws If the base64 string is not a valid transaction or transaction message. */ @@ -68,6 +70,7 @@ export class Signer { account: SolanaKeyringAccount, network: Network, config?: DecompileTransactionMessageFetchingLookupTablesConfig, + preserveMessageBytes = false, ): Promise { this.#logger.log('Partially sign base64 string', { base64String, @@ -76,6 +79,13 @@ export class Signer { config, }); + if (preserveMessageBytes) { + const transaction = + await fromUnknowBase64StringToTransaction(base64String); + + return this.#partiallySignTransaction(transaction, account); + } + const rpc = this.#connection.getRpc(network); // The received base64 string can either represent a transaction or a transaction message. diff --git a/packages/snap/src/core/services/wallet/WalletService.test.ts b/packages/snap/src/core/services/wallet/WalletService.test.ts index 77299121..fa20c958 100644 --- a/packages/snap/src/core/services/wallet/WalletService.test.ts +++ b/packages/snap/src/core/services/wallet/WalletService.test.ts @@ -1,6 +1,6 @@ import { SolMethod } from '@metamask/keyring-api'; -import { Network } from '../../constants/solana'; +import { METAMASK_ORIGIN, Network } from '../../constants/solana'; import { MOCK_SOLANA_KEYRING_ACCOUNT_0, MOCK_SOLANA_KEYRING_ACCOUNT_1, @@ -243,6 +243,40 @@ describe('WalletService', () => { }); }); + it('requests byte preservation for dapp origins', async () => { + await service.signTransaction( + fromAccount, + transactionMessageBase64Encoded, + scope, + origin, + ); + + expect(mockSigner.partiallySignBase64String).toHaveBeenCalledWith( + transactionMessageBase64Encoded, + fromAccount, + scope, + undefined, + true, + ); + }); + + it('does not request byte preservation for MetaMask origin', async () => { + await service.signTransaction( + fromAccount, + transactionMessageBase64Encoded, + scope, + METAMASK_ORIGIN, + ); + + expect(mockSigner.partiallySignBase64String).toHaveBeenCalledWith( + transactionMessageBase64Encoded, + fromAccount, + scope, + undefined, + false, + ); + }); + it('starts monitoring the transaction for commitment "confirmed"', async () => { await service.signTransaction( fromAccount, @@ -275,6 +309,40 @@ describe('WalletService', () => { }); }); + it('requests byte preservation for dapp origins', async () => { + await service.signAndSendTransaction( + fromAccount, + transactionMessageBase64Encoded, + scope, + origin, + ); + + expect(mockSigner.partiallySignBase64String).toHaveBeenCalledWith( + transactionMessageBase64Encoded, + fromAccount, + scope, + undefined, + true, + ); + }); + + it('does not request byte preservation for MetaMask origin', async () => { + await service.signAndSendTransaction( + fromAccount, + transactionMessageBase64Encoded, + scope, + METAMASK_ORIGIN, + ); + + expect(mockSigner.partiallySignBase64String).toHaveBeenCalledWith( + transactionMessageBase64Encoded, + fromAccount, + scope, + undefined, + false, + ); + }); + it('starts monitoring the transaction for commitment "confirmed"', async () => { await service.signAndSendTransaction( fromAccount, diff --git a/packages/snap/src/core/services/wallet/WalletService.ts b/packages/snap/src/core/services/wallet/WalletService.ts index 3a2591cb..07747924 100644 --- a/packages/snap/src/core/services/wallet/WalletService.ts +++ b/packages/snap/src/core/services/wallet/WalletService.ts @@ -18,7 +18,11 @@ import { } from '@solana/kit'; import type { SolanaKeyringAccount } from '../../../entities'; -import type { Caip10Address, Network } from '../../constants/solana'; +import { + METAMASK_ORIGIN, + type Caip10Address, + type Network, +} from '../../constants/solana'; import type { DecompileTransactionMessageFetchingLookupTablesConfig } from '../../sdk-extensions/codecs'; import { fromTransactionToBase64String } from '../../sdk-extensions/codecs'; import { addressToCaip10 } from '../../utils/addressToCaip10'; @@ -179,12 +183,17 @@ export class WalletService { } : undefined; + // For transactions coming from DApps, preserve the original message bytes. + // Mutating them would change the signing payload and break multisig flows. + const shouldPreserveMessageBytes = origin !== METAMASK_ORIGIN; + const partiallySignedTransaction = await this.#signer.partiallySignBase64String( transaction, account, scope, config, + shouldPreserveMessageBytes, ); const signedTransactionBase64 = fromTransactionToBase64String( @@ -250,12 +259,17 @@ export class WalletService { } : undefined; + // For transactions coming from DApps, preserve the original message bytes. + // Mutating them would change the signing payload and break multisig flows. + const shouldPreserveMessageBytes = origin !== METAMASK_ORIGIN; + const partiallySignedTransaction = await this.#signer.partiallySignBase64String( transactionMessageBase64Encoded, account, scope, signConfig, + shouldPreserveMessageBytes, ); const signature = getSignatureFromTransaction(partiallySignedTransaction);