From e54be1b6638c6034880231951b6c797392eaaf02 Mon Sep 17 00:00:00 2001 From: Hrishikesh Jain Date: Wed, 11 Nov 2026 17:18:39 +0530 Subject: [PATCH 1/2] fix(sdk-coin-sol): stop PrepareForRevoke memo from hiding transfer outputs A plain SOL transfer carrying a PrepareForRevoke memo was explained as a StakingDeactivate with empty outputs, since the memo is attacker-controlled and was the only signal used. Always surface the transfer recipient and amount in explain output (WASM and legacy paths), and make verifyTransaction reject a transfer intent that decodes as a staking deactivate. Ticket: CHALO-2127 Co-Authored-By: Claude --- .../src/lib/explainTransactionWasm.ts | 6 --- modules/sdk-coin-sol/src/lib/transaction.ts | 11 ++++++ modules/sdk-coin-sol/src/sol.ts | 5 +++ .../test/unit/explainTransactionWasm.ts | 38 +++++++++++++++++++ modules/sdk-coin-sol/test/unit/sol.ts | 23 +++++++++++ 5 files changed, 77 insertions(+), 6 deletions(-) diff --git a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts index c2f92cfa4b5..7d64261ea6e 100644 --- a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts +++ b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts @@ -257,11 +257,6 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So const combined = detectCombinedPattern(parsed.instructionsData); const txType = deriveTransactionType(parsed.instructionsData, combined, memo); - // Marinade deactivate: Transfer + PrepareForRevoke memo. - // The Transfer is a contract interaction, not real value transfer — skip from outputs. - const isMarinadeDeactivate = - txType === TransactionType.StakingDeactivate && memo !== undefined && memo.includes('PrepareForRevoke'); - // --- Extract outputs and inputs --- const outputs: { address: string; amount: bigint; tokenName?: string }[] = []; const inputs: { address: string; value: bigint }[] = []; @@ -276,7 +271,6 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So for (const instr of parsed.instructionsData) { switch (instr.type) { case 'Transfer': - if (isMarinadeDeactivate) break; outputs.push({ address: instr.toAddress, amount: instr.amount }); inputs.push({ address: instr.fromAddress, value: instr.amount }); break; diff --git a/modules/sdk-coin-sol/src/lib/transaction.ts b/modules/sdk-coin-sol/src/lib/transaction.ts index aead086ff1c..1b804d0a9a4 100644 --- a/modules/sdk-coin-sol/src/lib/transaction.ts +++ b/modules/sdk-coin-sol/src/lib/transaction.ts @@ -36,6 +36,7 @@ import { Nonce, StakingActivate, StakingAuthorizeParams, + StakingDeactivate, StakingWithdraw, TokenTransfer, TransactionExplanation, @@ -751,6 +752,16 @@ export class Transaction extends BaseTransaction { }); outputAmount = outputAmount.plus(stakingActivateInstruction.params.amount); break; + case InstructionBuilderTypes.StakingDeactivate: + // A Marinade deactivate is a real SOL transfer; never hide it from the explained outputs. + const stakingDeactivateInstruction = instruction as StakingDeactivate; + if (stakingDeactivateInstruction.params.stakingType === SolStakingTypeEnum.MARINADE) { + for (const recipient of stakingDeactivateInstruction.params.recipients ?? []) { + outputs.push({ address: recipient.address, amount: recipient.amount }); + outputAmount = outputAmount.plus(recipient.amount); + } + } + break; case InstructionBuilderTypes.StakingWithdraw: const stakingWithdrawInstruction = instruction as StakingWithdraw; outputs.push({ diff --git a/modules/sdk-coin-sol/src/sol.ts b/modules/sdk-coin-sol/src/sol.ts index 09a5fff3b74..1c86000ee33 100644 --- a/modules/sdk-coin-sol/src/sol.ts +++ b/modules/sdk-coin-sol/src/sol.ts @@ -610,6 +610,11 @@ export class Sol extends BaseCoin { const explainedTx = transaction.explainTransaction(); const isCloseAssociatedTokenAccountTx = transaction.type === TransactionType.CloseAssociatedTokenAccount; + // The memo is attacker-controlled, so a payment intent must never decode as a staking deactivate. + if (txParams.type === 'transfer' && transaction.type === TransactionType.StakingDeactivate) { + throw new Error('Invalid transaction type: transfer intent decoded as staking deactivate'); + } + if (txParams.type === 'enabletoken' && verificationOptions?.verifyTokenEnablement) { this.verifyTxType(txParams.type, explainedTx.type); const tokenEnablementsPrebuild = this.throwIfMissingTokenEnablementsOrReturn(explainedTx); diff --git a/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts b/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts index 24e46f5e2d5..4bf2951025d 100644 --- a/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts +++ b/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts @@ -3,8 +3,46 @@ */ import 'should'; import { explainSolTransaction } from '../../src/lib/explainTransactionWasm'; +import { KeyPair } from '../../src'; +import * as testData from '../resources/sol'; +import { getBuilderFactory } from './getBuilderFactory'; describe('explainTransactionWasm', function () { + describe('PrepareForRevoke memo', function () { + const sender = new KeyPair(testData.authAccount).getKeys().pub; + const recipient = 'CP5Dpaa42RtJmMuKqCQsLwma5Yh3knuvKsYDFX85F41S'; + const blockhash = 'GHtXQBsoZHVnNFa9YevAzFr17DJjgHXk3ycTKD5xD3Zi'; + + it('should not hide the transfer recipient and amount for a memo-tagged transfer', async function () { + const builder = getBuilderFactory('tsol').getTransferBuilder(); + builder.sender(sender).nonce(blockhash).send({ address: recipient, amount: '1000' }); + builder.memo('{"PrepareForRevoke":{"user":"attacker","amount":"1"}}'); + const tx = await builder.build(); + + const wasm = explainSolTransaction({ + txBase64: tx.toBroadcastFormat(), + feeInfo: { fee: '5000' }, + coinName: 'tsol', + }); + wasm.outputs.should.deepEqual([{ address: recipient, amount: '1000' }]); + wasm.outputAmount.should.equal('1000'); + + const legacy = tx.explainTransaction(); + legacy.outputs.should.deepEqual([{ address: recipient, amount: '1000' }]); + legacy.outputAmount.should.equal('1000'); + }); + + it('should still surface the transfer for a real Marinade deactivate', function () { + const explained = explainSolTransaction({ + txBase64: testData.MARINADE_STAKING_DEACTIVATE_SIGNED_TX, + feeInfo: { fee: '5000' }, + coinName: 'tsol', + }); + explained.type.should.equal('StakingDeactivate'); + explained.outputs.length.should.equal(1); + }); + }); + describe('deriveTransactionType', function () { it('should classify boilerplate-only transaction as CustomTx', function () { // Transaction with only NonceAdvance + Memo instructions (no Transfer or TokenTransfer). diff --git a/modules/sdk-coin-sol/test/unit/sol.ts b/modules/sdk-coin-sol/test/unit/sol.ts index c2fa5128254..ea7f29542e5 100644 --- a/modules/sdk-coin-sol/test/unit/sol.ts +++ b/modules/sdk-coin-sol/test/unit/sol.ts @@ -822,6 +822,29 @@ describe('SOL:', function () { .should.be.rejectedWith(/does not match the declared sponsored fee payer/); }); + it('should reject a transfer intent whose tx carries a PrepareForRevoke memo', async function () { + const recipientAddress = 'CP5Dpaa42RtJmMuKqCQsLwma5Yh3knuvKsYDFX85F41S'; + const prepareForRevokeMemo = '{"PrepareForRevoke":{"user":"attacker","amount":"1"}}'; + + const txBuilder = factory.getTransferBuilder(); + txBuilder.sender(wallet.pub); + txBuilder.nonce(blockHash); + txBuilder.send({ address: recipientAddress, amount: '1000' }); + txBuilder.memo(prepareForRevokeMemo); + const tx = await txBuilder.build(); + + await basecoin + .verifyTransaction({ + txParams: { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, + txPrebuild: { txBase64: tx.toBroadcastFormat() }, + memo: { value: prepareForRevokeMemo }, + feePayer: wallet.pub, + blockhash: blockHash, + wallet: walletObj, + } as any) + .should.be.rejectedWith('Invalid transaction type: transfer intent decoded as staking deactivate'); + }); + it('should verify a builder-produced transaction end-to-end with a sponsored fee payer', async function () { // Build a real transaction with the fee payer set to a different address, // serialize it, and verify it passes through verifyTransaction with the From 7bad8d43ab119f82ddc8703234854e9645ddab86 Mon Sep 17 00:00:00 2001 From: Hrishikesh Jain Date: Wed, 11 Nov 2026 21:55:54 +0530 Subject: [PATCH 2/2] fix(sdk-coin-sol): validate Marinade unstake by transfer, not memo The PrepareForRevoke memo is attacker-controlled yet was the only signal used to type a transaction as a Marinade deactivate, which also hid the transfer from explained outputs and let verifyTransaction pass a drain. Treat a transaction as a Marinade unstake only when it has exactly one SOL transfer to a Marinade order address for the pinned amount, one well-formed PrepareForRevoke memo, and nothing but nonce-advance/compute-budget; anything else is a Send and its output is verified normally. verifyTransaction also requires the Marinade transfer to come from the wallet root. The staking deactivate and withdraw parsers now throw on unsupported instructions. Ticket: CHALO-2127 Co-Authored-By: Claude --- modules/sdk-coin-sol/src/lib/constants.ts | 12 +++ .../src/lib/explainTransactionWasm.ts | 37 +++++++-- .../src/lib/instructionParamsFactory.ts | 17 ++++ modules/sdk-coin-sol/src/lib/transaction.ts | 11 --- modules/sdk-coin-sol/src/lib/utils.ts | 58 +++++++++++++- modules/sdk-coin-sol/src/sol.ts | 11 ++- modules/sdk-coin-sol/test/resources/sol.ts | 2 +- .../test/unit/explainTransactionWasm.ts | 67 ++++++++++------ .../unit/instructionParamsFactory.staking.ts | 56 ++++---------- modules/sdk-coin-sol/test/unit/sol.ts | 77 +++++++++++++++---- .../stakingDeactivateBuilder.ts | 2 +- 11 files changed, 244 insertions(+), 106 deletions(-) diff --git a/modules/sdk-coin-sol/src/lib/constants.ts b/modules/sdk-coin-sol/src/lib/constants.ts index 65a6d8d8cfa..cae85bb657d 100644 --- a/modules/sdk-coin-sol/src/lib/constants.ts +++ b/modules/sdk-coin-sol/src/lib/constants.ts @@ -268,6 +268,18 @@ export const marinadeStakingDeactivateInstructionsIndexes = { Memo: 1, } as const; +/** + * Marinade native-staking unstake has no on-chain program instruction: it is a single SystemProgram + * Transfer to one of Marinade's order addresses plus a PrepareForRevoke memo, acted on off-chain. + * The destination and amount are therefore the only server-independent signal that it is genuine. + */ +export const MARINADE_ORDER_ADDRESSES: readonly string[] = [ + 'opNS8ENpEMWdXcJUgJCsJTDp7arTXayoBEeBUg6UezP', // native + 'opiNSvKm1aPkbuSjnZfY5vyJUmaVxuQyTJt6jnNTW1L', // select +]; +/** Lamports BitGo sends per Marinade unstake order (marinade.unstakeTransferAmount in staking-service) */ +export const MARINADE_UNSTAKE_TRANSFER_LAMPORTS = BigInt(2300000); + /** Const to check the order of the Partial Staking Deactivate instructions when decoded */ export const stakingPartialDeactivateInstructionsIndexes = { Transfer: 0, diff --git a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts index 7d64261ea6e..481f4749f2b 100644 --- a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts +++ b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts @@ -9,6 +9,7 @@ import { } from './constants'; import { StakingAuthorizeParams, TransactionExplanation as SolLibTransactionExplanation } from './iface'; import { findTokenName } from './instructionParamsFactory'; +import { isMarinadeUnstakeTransfer, isPrepareForRevokeMemo } from './utils'; export interface ExplainTransactionWasmOptions { txBase64: string; @@ -129,15 +130,31 @@ function isWasmTokenAclThawInstruction(instr: InstructionParams): boolean { return instr.type === 'Unknown' && instr.programId === TOKEN_ACL_PROGRAM_ID; } -function deriveTransactionType( - instructions: InstructionParams[], - combined: CombinedPattern | null, - memo: string | undefined -): TransactionType { +/** + * True only for a genuine Marinade unstake: exactly one Transfer to a Marinade order address for the + * pinned amount, one well-formed PrepareForRevoke memo, and only nonce-advance / compute-budget besides. + * Mirrors getMarinadeUnstakeTransfer in utils.ts; the memo alone is attacker-controlled. + */ +function isWasmMarinadeUnstake(instructions: InstructionParams[]): boolean { + const transfers = instructions.filter((i) => i.type === 'Transfer'); + const memos = instructions.filter((i) => i.type === 'Memo'); + const others = instructions.filter((i) => !BOILERPLATE_TYPES.has(i.type) && i.type !== 'Transfer'); + if (transfers.length !== 1 || memos.length !== 1 || others.length > 0) return false; + const transfer = transfers[0]; + const memo = memos[0]; + return ( + transfer.type === 'Transfer' && + memo.type === 'Memo' && + isPrepareForRevokeMemo(memo.memo) && + isMarinadeUnstakeTransfer(transfer.toAddress, transfer.amount) + ); +} + +function deriveTransactionType(instructions: InstructionParams[], combined: CombinedPattern | null): TransactionType { if (combined) return TransactionType[combined.kind]; - // Marinade deactivate: Transfer + memo containing "PrepareForRevoke" - if (memo?.includes('PrepareForRevoke')) return TransactionType.StakingDeactivate; + // Marinade deactivate: validated by transfer destination/amount, never by the memo alone + if (isWasmMarinadeUnstake(instructions)) return TransactionType.StakingDeactivate; // Jito pool operations if (instructions.some((i) => i.type === 'StakePoolDepositSol')) return TransactionType.StakingActivate; @@ -255,7 +272,10 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So // --- Detect combined patterns and derive type --- const combined = detectCombinedPattern(parsed.instructionsData); - const txType = deriveTransactionType(parsed.instructionsData, combined, memo); + const txType = deriveTransactionType(parsed.instructionsData, combined); + + // Genuine Marinade deactivate: the Transfer is a contract interaction, not a real value transfer. + const isMarinadeDeactivate = isWasmMarinadeUnstake(parsed.instructionsData); // --- Extract outputs and inputs --- const outputs: { address: string; amount: bigint; tokenName?: string }[] = []; @@ -271,6 +291,7 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So for (const instr of parsed.instructionsData) { switch (instr.type) { case 'Transfer': + if (isMarinadeDeactivate) break; outputs.push({ address: instr.toAddress, amount: instr.amount }); inputs.push({ address: instr.fromAddress, value: instr.amount }); break; diff --git a/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts b/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts index ac0b4819712..9302f19ca88 100644 --- a/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts +++ b/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts @@ -788,6 +788,10 @@ function parseStakingDeactivateInstructions( ): Array { const instructionData: Array = []; const unstakingInstructions: UnstakingInstructions[] = []; + // Jito unstake legitimately carries Approve + Create alongside WithdrawStake + const isJitoUnstake = instructions.some( + (instruction) => getInstructionType(instruction) === ValidInstructionTypesEnum.WithdrawStake + ); for (const instruction of instructions) { const type = getInstructionType(instruction); switch (type) { @@ -896,6 +900,16 @@ function parseStakingDeactivateInstructions( }); } break; + + case ValidInstructionTypesEnum.Approve: + case ValidInstructionTypesEnum.Create: + if (!isJitoUnstake) { + throw new NotSupported('Invalid deactivate stake transaction, instruction type not supported: ' + type); + } + break; + + default: + throw new NotSupported('Invalid deactivate stake transaction, instruction type not supported: ' + type); } } @@ -1099,6 +1113,9 @@ function parseStakingWithdrawInstructions( }; instructionData.push(stakingWithdraw); break; + + default: + throw new NotSupported('Invalid withdraw stake transaction, instruction type not supported: ' + type); } } diff --git a/modules/sdk-coin-sol/src/lib/transaction.ts b/modules/sdk-coin-sol/src/lib/transaction.ts index 1b804d0a9a4..aead086ff1c 100644 --- a/modules/sdk-coin-sol/src/lib/transaction.ts +++ b/modules/sdk-coin-sol/src/lib/transaction.ts @@ -36,7 +36,6 @@ import { Nonce, StakingActivate, StakingAuthorizeParams, - StakingDeactivate, StakingWithdraw, TokenTransfer, TransactionExplanation, @@ -752,16 +751,6 @@ export class Transaction extends BaseTransaction { }); outputAmount = outputAmount.plus(stakingActivateInstruction.params.amount); break; - case InstructionBuilderTypes.StakingDeactivate: - // A Marinade deactivate is a real SOL transfer; never hide it from the explained outputs. - const stakingDeactivateInstruction = instruction as StakingDeactivate; - if (stakingDeactivateInstruction.params.stakingType === SolStakingTypeEnum.MARINADE) { - for (const recipient of stakingDeactivateInstruction.params.recipients ?? []) { - outputs.push({ address: recipient.address, amount: recipient.amount }); - outputAmount = outputAmount.plus(recipient.amount); - } - } - break; case InstructionBuilderTypes.StakingWithdraw: const stakingWithdrawInstruction = instruction as StakingWithdraw; outputs.push({ diff --git a/modules/sdk-coin-sol/src/lib/utils.ts b/modules/sdk-coin-sol/src/lib/utils.ts index 09e74e91f76..0a66bac1465 100644 --- a/modules/sdk-coin-sol/src/lib/utils.ts +++ b/modules/sdk-coin-sol/src/lib/utils.ts @@ -62,6 +62,8 @@ import { ZK_ELGAMAL_PROOF_PROGRAM_ID, CT_EXT_DISCRIMINATOR, CT_SUB_DISCRIMINATORS, + MARINADE_ORDER_ADDRESSES, + MARINADE_UNSTAKE_TRANSFER_LAMPORTS, } from './constants'; import { ValidInstructionTypes } from './iface'; import { STAKE_POOL_INSTRUCTION_LAYOUTS, STAKE_POOL_PROGRAM_ID } from '@solana/spl-stake-pool'; @@ -363,6 +365,57 @@ function isConfidentialTransferRawInstruction(instruction: TransactionInstructio return false; } +/** True if the memo is a well-formed Marinade unstake memo: {"PrepareForRevoke":{"user":..,"amount":..}} */ +export function isPrepareForRevokeMemo(memo: string | undefined): boolean { + if (!memo) return false; + try { + const body = JSON.parse(memo)?.PrepareForRevoke; + return ( + typeof body === 'object' && body !== null && typeof body.user === 'string' && /^\d+$/.test(body.amount ?? '') + ); + } catch { + return false; + } +} + +/** True if a SOL transfer matches the single transfer BitGo sends to Marinade's order address per unstake. */ +export function isMarinadeUnstakeTransfer(toAddress: string, lamports: bigint | number | string): boolean { + return MARINADE_ORDER_ADDRESSES.includes(toAddress) && BigInt(lamports) === MARINADE_UNSTAKE_TRANSFER_LAMPORTS; +} + +/** + * Returns the decoded transfer if the instructions are a genuine Marinade unstake, otherwise undefined. + * Genuine means exactly one SOL Transfer to a Marinade order address for the pinned amount, one well-formed + * PrepareForRevoke memo, and nothing else except nonce-advance / compute-budget. The memo alone is + * attacker-controlled, so it must never decide the type by itself. + */ +export function getMarinadeUnstakeTransfer( + instructions: TransactionInstruction[] +): ReturnType | undefined { + const transfers: TransactionInstruction[] = []; + const memos: TransactionInstruction[] = []; + for (const instruction of instructions) { + switch (getInstructionType(instruction)) { + case ValidInstructionTypesEnum.AdvanceNonceAccount: + case ValidInstructionTypesEnum.SetComputeUnitLimit: + case ValidInstructionTypesEnum.SetPriorityFee: + break; + case ValidInstructionTypesEnum.Memo: + memos.push(instruction); + break; + case ValidInstructionTypesEnum.Transfer: + transfers.push(instruction); + break; + default: + return undefined; + } + } + if (transfers.length !== 1 || memos.length !== 1) return undefined; + if (!isPrepareForRevokeMemo(memos[0].data.toString('utf-8'))) return undefined; + const transfer = SystemInstruction.decodeTransfer(transfers[0]); + return isMarinadeUnstakeTransfer(transfer.toPubkey.toString(), transfer.lamports) ? transfer : undefined; +} + /** * Returns the transaction Type based on the transaction instructions. * Wallet initialization, Transfer and Staking transactions are supported. @@ -395,11 +448,12 @@ export function getTransactionType(transaction: SolTransaction): TransactionType } if (instructions.filter((instruction) => getInstructionType(instruction) === 'Deactivate').length === 0) { + // A Transfer is a Send unless the whole transaction is a genuine Marinade unstake (see getMarinadeUnstakeTransfer) + const isMarinadeUnstake = getMarinadeUnstakeTransfer(instructions) !== undefined; for (const instruction of instructions) { const instructionType = getInstructionType(instruction); - // Check if memo instruction is there and if it contains 'PrepareForRevoke' because Marinade staking deactivate transaction will have this if ( - (instructionType === ValidInstructionTypesEnum.Transfer && !memoData?.includes('PrepareForRevoke')) || + (instructionType === ValidInstructionTypesEnum.Transfer && !isMarinadeUnstake) || instructionType === ValidInstructionTypesEnum.TokenTransfer ) { return TransactionType.Send; diff --git a/modules/sdk-coin-sol/src/sol.ts b/modules/sdk-coin-sol/src/sol.ts index 1c86000ee33..8f2150566be 100644 --- a/modules/sdk-coin-sol/src/sol.ts +++ b/modules/sdk-coin-sol/src/sol.ts @@ -95,6 +95,7 @@ import { } from './lib/token2022Resolve'; import { getAssociatedTokenAccountAddress, + getMarinadeUnstakeTransfer, getSolTokenFromAddress, getSolTokenFromTokenName, isValidAddress, @@ -610,9 +611,13 @@ export class Sol extends BaseCoin { const explainedTx = transaction.explainTransaction(); const isCloseAssociatedTokenAccountTx = transaction.type === TransactionType.CloseAssociatedTokenAccount; - // The memo is attacker-controlled, so a payment intent must never decode as a staking deactivate. - if (txParams.type === 'transfer' && transaction.type === TransactionType.StakingDeactivate) { - throw new Error('Invalid transaction type: transfer intent decoded as staking deactivate'); + // A Marinade unstake produces no explained outputs, so the only thing tying its transfer to this wallet + // is the source. The source is the wallet root, not the fee payer, since sponsored fee payers exist. + if (transaction.type === TransactionType.StakingDeactivate) { + const marinadeTransfer = getMarinadeUnstakeTransfer(transaction.solTransaction.instructions); + if (marinadeTransfer && marinadeTransfer.fromPubkey.toString() !== walletRootAddress) { + throw new Error('Marinade unstake transfer must be sent from the wallet root address'); + } } if (txParams.type === 'enabletoken' && verificationOptions?.verifyTokenEnablement) { diff --git a/modules/sdk-coin-sol/test/resources/sol.ts b/modules/sdk-coin-sol/test/resources/sol.ts index 527b6ff7a7e..d31cdb13961 100644 --- a/modules/sdk-coin-sol/test/resources/sol.ts +++ b/modules/sdk-coin-sol/test/resources/sol.ts @@ -205,7 +205,7 @@ export const STAKING_DEACTIVATE_SIGNED_TX = 'AUfyWtl4IUxhH21qX/H03hJZer1XxQaxL2r/uDTM/u1GzBIyePCHu78O2SkWGEYP6eDdiY3OLfJmUM1jiy8NCAoBAAIEReV5vPklPPaLR9/x+zo6XCwhusWyPAmuEqbgVWvwi0Fiey+6ASdh+bkZvPlMu0ydyAUdnwkymTFNOUkjMmi96Qah2BeRN1QqmDQ3vf4qerJVf1NcinhyK2ikncAAAAAABqfVFxjHdMkoVmOYaR1etoteuKObS21cc1VbIQAAAADjMtr5L6vs6LY/96RABeX9/Zr6FYdWthxalfkEs7jQgQECAwEDAAQFAAAA'; export const MARINADE_STAKING_DEACTIVATE_SIGNED_TX = - 'AaiHUOzzeJaUzpdkm2BmLJI3AVOhFHTD5BnVMwUH3lRv8hKpH1fnXiXNm6ghZgNwhggXBAqhL3t4XEl+H7T95gIBAAIEReV5vPklPPaLR9/x+zo6XCwhusWyPAmuEqbgVWvwi0EL/kczKamI94jNtLT/BR9nkfa/PR2IU6d7qaEV8VVC3AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABUpTWpkpIQZNJOhxYNo4fHw1td28kruB5B+oQEEFRI3jMtr5L6vs6LY/96RABeX9/Zr6FYdWthxalfkEs7jQgQICAgABDAIAAABgGCMAAAAAAAMAbntcIlByZXBhcmVGb3JSZXZva2VcIjp7XCJ1c2VyXCI6XCI1aHI1ZmlzUGk2RFhOdXVScG01WFVienBpRW5tZHl4WHVCRFR3endaajVQZX1cIixcImFtb3VudFwiOlwiNTAwMDAwMDAwMDAwXCJ9'; + 'AWmhrcrX9qzw++JsZqmjFwPT41NpnDqbSUe5P34yCMBYsJcNKgV6o1KWgZZc9qfimToM/uf/c5ihnnr+6adZiwcBAAIEReV5vPklPPaLR9/x+zo6XCwhusWyPAmuEqbgVWvwi0EL/kczKamI94jNtLT/BR9nkfa/PR2IU6d7qaEV8VVC3AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABUpTWpkpIQZNJOhxYNo4fHw1td28kruB5B+oQEEFRI3jMtr5L6vs6LY/96RABeX9/Zr6FYdWthxalfkEs7jQgQICAgABDAIAAABgGCMAAAAAAAMAZHsiUHJlcGFyZUZvclJldm9rZSI6eyJ1c2VyIjoiNWhyNWZpc1BpNkRYTnV1UnBtNVhVYnpwaUVubWR5eFh1QkRUd3p3Wmo1UGUiLCJhbW91bnQiOiI1MDAwMDAwMDAwMDAifX0='; export const JITO_STAKING_DEACTIVATE_SIGNED_TX = 'A7txZr55CtSJogfV1ihB1JOIuVbmhAh7BCl4hJeTBcbrq6KT+Jzbbjx4qEXDgRnMtY7cb9xnekOHUfKkW9D2RQpchl2oH4Np/+Oghy7QNjKrodZsFlqhiYoo+Zx0Bjf+Hwq35h/zVd1kHRTkaB1ebZwDeEejPrFgNCpkqxRh9ZgOMBethjkNPCrqzk50pOqx1ktJik5loScyp/81bggjQASE4jMdtET/a2jpFJeG34GZLIY6r+LNTXtGsK53qyR9CQMBBg9F5Xm8+SU89otH3/H7OjpcLCG6xbI8Ca4SpuBVa/CLQWJ7L7oBJ2H5uRm8+Uy7TJ3IBR2fCTKZMU05SSMyaL3p6Lfi7T4mzoclKbPedsv+JDs60KtRcBK6Y7CHyYejKikcg6pe+BOG2OETfAVS9ftz6va1oE4onLBolJ2N+ZOOhCPgdQm63e39tRapC5GXu1BHQyVdDjfF/13OiiQe7cQxl9rD5vZLBx1Aaz7SV4hmv2ZhGp4LQEU67b++EtDrIi8J5qP+7PmQMuHB32uXItyzY057jjRAk2vDSwzByOtSH/zRQemDLK8QrZF0lcoPJxtbKTzUcCfqc3AH7UDrOaC9BIo+CMO0lb4X9FQn2JvsW4DH4mlcGGTXZ0PbOb7TRtYAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFTlniTAUaihSnsel5fw4Szfp0kCFmUxlaalEqbxZmArBoFO1Mr2ihdGcv2shgMaY+hOoV76HUS3IpP229sAFlAGodgXkTdUKpg0N73+KnqyVX9TXIp4citopJ3AAAAAAAan1RcYx3TJKFZjmGkdXraLXrijm0ttXHNVWyEAAAAABt324ddloZPZy+FGzut5rBy0he1fWzeROoz1hX7/AKnjMtr5L6vs6LY/96RABeX9/Zr6FYdWthxalfkEs7jQgQQOAwMCAAkE6AMAAAAAAAAJAgABNAAAAACA1SIAAAAAAMgAAAAAAAAABqHYF5E3VCqYNDe9/ip6slV/U1yKeHIraKSdwAAAAAALDQgECgUBAAIDBgcNDgwJCugDAAAAAAAADAMBDQAEBQAAAA=='; diff --git a/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts b/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts index 4bf2951025d..6629e96463d 100644 --- a/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts +++ b/modules/sdk-coin-sol/test/unit/explainTransactionWasm.ts @@ -3,43 +3,60 @@ */ import 'should'; import { explainSolTransaction } from '../../src/lib/explainTransactionWasm'; +import { TransactionType } from '@bitgo/sdk-core'; import { KeyPair } from '../../src'; import * as testData from '../resources/sol'; import { getBuilderFactory } from './getBuilderFactory'; describe('explainTransactionWasm', function () { describe('PrepareForRevoke memo', function () { + const factory = getBuilderFactory('tsol'); const sender = new KeyPair(testData.authAccount).getKeys().pub; - const recipient = 'CP5Dpaa42RtJmMuKqCQsLwma5Yh3knuvKsYDFX85F41S'; + const attacker = 'CP5Dpaa42RtJmMuKqCQsLwma5Yh3knuvKsYDFX85F41S'; + const marinadeOrder = 'opNS8ENpEMWdXcJUgJCsJTDp7arTXayoBEeBUg6UezP'; const blockhash = 'GHtXQBsoZHVnNFa9YevAzFr17DJjgHXk3ycTKD5xD3Zi'; + const validMemo = `{"PrepareForRevoke":{"user":"${sender}","amount":"500000000000"}}`; + + const buildTransfer = async (to: string, amount: string, memo: string) => { + const builder = factory.getTransferBuilder(); + builder.sender(sender).nonce(blockhash).send({ address: to, amount }); + builder.memo(memo); + return (await builder.build()).toBroadcastFormat(); + }; + + // The legacy path derives the type when parsing a raw tx, so rebuild it from raw like verifyTransaction does. + const explainBoth = async (rawTx: string) => { + const wasm = explainSolTransaction({ txBase64: rawTx, feeInfo: { fee: '5000' }, coinName: 'tsol' }); + const legacyTx = await factory.from(rawTx).build(); + return { wasm, legacy: legacyTx.explainTransaction(), legacyType: legacyTx.type }; + }; + + it('should classify a memo-tagged transfer to an arbitrary address as Send and keep its output', async function () { + const { wasm, legacy, legacyType } = await explainBoth(await buildTransfer(attacker, '1000', validMemo)); + wasm.type.should.equal('Send'); + wasm.outputs.should.deepEqual([{ address: attacker, amount: '1000' }]); + legacyType.should.equal(TransactionType.Send); + legacy.outputs.should.deepEqual([{ address: attacker, amount: '1000' }]); + }); - it('should not hide the transfer recipient and amount for a memo-tagged transfer', async function () { - const builder = getBuilderFactory('tsol').getTransferBuilder(); - builder.sender(sender).nonce(blockhash).send({ address: recipient, amount: '1000' }); - builder.memo('{"PrepareForRevoke":{"user":"attacker","amount":"1"}}'); - const tx = await builder.build(); - - const wasm = explainSolTransaction({ - txBase64: tx.toBroadcastFormat(), - feeInfo: { fee: '5000' }, - coinName: 'tsol', - }); - wasm.outputs.should.deepEqual([{ address: recipient, amount: '1000' }]); - wasm.outputAmount.should.equal('1000'); + it('should classify a Marinade-address transfer with the wrong amount as Send', async function () { + const { wasm, legacyType } = await explainBoth(await buildTransfer(marinadeOrder, '3000000', validMemo)); + wasm.type.should.equal('Send'); + legacyType.should.equal(TransactionType.Send); + }); - const legacy = tx.explainTransaction(); - legacy.outputs.should.deepEqual([{ address: recipient, amount: '1000' }]); - legacy.outputAmount.should.equal('1000'); + it('should classify a Marinade-address transfer with a malformed PrepareForRevoke memo as Send', async function () { + const { wasm, legacyType } = await explainBoth(await buildTransfer(marinadeOrder, '2300000', 'PrepareForRevoke')); + wasm.type.should.equal('Send'); + legacyType.should.equal(TransactionType.Send); }); - it('should still surface the transfer for a real Marinade deactivate', function () { - const explained = explainSolTransaction({ - txBase64: testData.MARINADE_STAKING_DEACTIVATE_SIGNED_TX, - feeInfo: { fee: '5000' }, - coinName: 'tsol', - }); - explained.type.should.equal('StakingDeactivate'); - explained.outputs.length.should.equal(1); + it('should still classify a genuine Marinade unstake as StakingDeactivate with no outputs', async function () { + const { wasm, legacy, legacyType } = await explainBoth(testData.MARINADE_STAKING_DEACTIVATE_SIGNED_TX); + wasm.type.should.equal('StakingDeactivate'); + wasm.outputs.length.should.equal(0); + legacyType.should.equal(TransactionType.StakingDeactivate); + legacy.outputs.length.should.equal(0); }); }); diff --git a/modules/sdk-coin-sol/test/unit/instructionParamsFactory.staking.ts b/modules/sdk-coin-sol/test/unit/instructionParamsFactory.staking.ts index 49b8cc45b32..9d7e2c329e8 100644 --- a/modules/sdk-coin-sol/test/unit/instructionParamsFactory.staking.ts +++ b/modules/sdk-coin-sol/test/unit/instructionParamsFactory.staking.ts @@ -417,7 +417,7 @@ describe('Instruction Parser Staking Tests: ', function () { should.deepEqual(result, instructionsData); }); - it('Should parse deactivate stake tx instructions if there are unexpected instructions', () => { + it('Should reject deactivate stake tx instructions if there are unexpected instructions', () => { const fromAccount = new PublicKey(testData.authAccount.pub); const stakingAccount = new PublicKey(testData.stakeAccount.pub); const validator = new PublicKey(testData.validator.pub); @@ -446,26 +446,14 @@ describe('Instruction Parser Staking Tests: ', function () { stakePubkey: stakingAccount, }).instructions; - // Params - const stakingActivateParams: StakingDeactivate = { - type: InstructionBuilderTypes.StakingDeactivate, - params: { - fromAddress: fromAccount.toString(), - stakingAddress: stakingAccount.toString(), - amount: undefined, - unstakingAddress: undefined, - stakingType: SolStakingTypeEnum.NATIVE, - }, - }; - const instructions = [ ...stakingActivateInstructions, ...stakingDelegateInstructions, ...stakingDeactivateInstructions, ]; - const instructionsData = [stakingActivateParams]; - const result = instructionParamsFactory(TransactionType.StakingDeactivate, instructions); - should.deepEqual(result, instructionsData); + should(() => instructionParamsFactory(TransactionType.StakingDeactivate, instructions)).throw( + 'Invalid deactivate stake transaction, instruction type not supported: Create' + ); }); describe('Partially deactivate stake instructions', function () { @@ -512,7 +500,7 @@ describe('Instruction Parser Staking Tests: ', function () { const instructions = [ nonceAdvanceInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -563,7 +551,7 @@ describe('Instruction Parser Staking Tests: ', function () { const instructions = [ nonceAdvanceInstruction, allocateInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -664,7 +652,7 @@ describe('Instruction Parser Staking Tests: ', function () { nonceAdvanceInstruction, allocateInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -721,7 +709,7 @@ describe('Instruction Parser Staking Tests: ', function () { nonceAdvanceInstruction, allocateInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -779,7 +767,7 @@ describe('Instruction Parser Staking Tests: ', function () { nonceAdvanceInstruction, allocateInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -837,7 +825,7 @@ describe('Instruction Parser Staking Tests: ', function () { nonceAdvanceInstruction, allocateInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -895,7 +883,7 @@ describe('Instruction Parser Staking Tests: ', function () { nonceAdvanceInstruction, allocateInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -952,7 +940,7 @@ describe('Instruction Parser Staking Tests: ', function () { nonceAdvanceInstruction, allocateInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -1040,7 +1028,7 @@ describe('Instruction Parser Staking Tests: ', function () { transferInstruction, allocateInstruction, assignInstruction, - ...splitInstructions, + splitInstructions[1], ...stakingDeactivateInstructions, memoInstruction, ]; @@ -1185,7 +1173,7 @@ describe('Instruction Parser Staking Tests: ', function () { should.deepEqual(result, instructionsData); }); - it('Should parse withdraw stake tx instructions if there are unexpected instructions', () => { + it('Should reject withdraw stake tx instructions if there are unexpected instructions', () => { const fromAccount = new PublicKey(testData.authAccount.pub); const stakingAccount = new PublicKey(testData.stakeAccount.pub); const validator = new PublicKey(testData.validator.pub); @@ -1216,24 +1204,14 @@ describe('Instruction Parser Staking Tests: ', function () { lamports: new BigNumber(amount).toNumber(), }).instructions; - // Params - const withdrawStakeParams: StakingWithdraw = { - type: InstructionBuilderTypes.StakingWithdraw, - params: { - fromAddress: fromAccount.toString(), - stakingAddress: stakingAccount.toString(), - amount, - }, - }; - const instructions = [ ...stakingActivateInstructions, ...stakingDelegateInstructions, ...withdrawStakeInstructions, ]; - const instructionsData = [withdrawStakeParams]; - const result = instructionParamsFactory(TransactionType.StakingWithdraw, instructions); - should.deepEqual(result, instructionsData); + should(() => instructionParamsFactory(TransactionType.StakingWithdraw, instructions)).throw( + 'Invalid withdraw stake transaction, instruction type not supported: Create' + ); }); }); }); diff --git a/modules/sdk-coin-sol/test/unit/sol.ts b/modules/sdk-coin-sol/test/unit/sol.ts index ea7f29542e5..c306848714e 100644 --- a/modules/sdk-coin-sol/test/unit/sol.ts +++ b/modules/sdk-coin-sol/test/unit/sol.ts @@ -822,27 +822,72 @@ describe('SOL:', function () { .should.be.rejectedWith(/does not match the declared sponsored fee payer/); }); - it('should reject a transfer intent whose tx carries a PrepareForRevoke memo', async function () { - const recipientAddress = 'CP5Dpaa42RtJmMuKqCQsLwma5Yh3knuvKsYDFX85F41S'; - const prepareForRevokeMemo = '{"PrepareForRevoke":{"user":"attacker","amount":"1"}}'; + describe('PrepareForRevoke memo', () => { + const attacker = 'CP5Dpaa42RtJmMuKqCQsLwma5Yh3knuvKsYDFX85F41S'; + const marinadeOrder = 'opNS8ENpEMWdXcJUgJCsJTDp7arTXayoBEeBUg6UezP'; + const memoFor = (user: string) => `{"PrepareForRevoke":{"user":"${user}","amount":"500000000000"}}`; + + const buildRaw = async (from: string, to: string, amount: string, memoText: string) => { + const txBuilder = factory.getTransferBuilder(); + txBuilder.sender(from); + txBuilder.nonce(blockHash); + txBuilder.send({ address: to, amount }); + txBuilder.memo(memoText); + return (await txBuilder.build()).toBroadcastFormat(); + }; - const txBuilder = factory.getTransferBuilder(); - txBuilder.sender(wallet.pub); - txBuilder.nonce(blockHash); - txBuilder.send({ address: recipientAddress, amount: '1000' }); - txBuilder.memo(prepareForRevokeMemo); - const tx = await txBuilder.build(); + it('should reject a consolidation prebuild that drains to an attacker behind the memo', async function () { + const txBase64 = await buildRaw(wallet.pub, attacker, '1000000000', memoFor(wallet.pub)); + await basecoin + .verifyTransaction({ + txParams: {}, + txPrebuild: { txBase64, consolidateId: 'consolidateId' }, + verification: { consolidationToBaseAddress: true }, + feePayer: wallet.pub, + blockhash: blockHash, + wallet: walletObj, + } as any) + .should.be.rejectedWith('tx outputs does not match with expected address'); + }); - await basecoin - .verifyTransaction({ - txParams: { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, - txPrebuild: { txBase64: tx.toBroadcastFormat() }, - memo: { value: prepareForRevokeMemo }, + it('should reject a payment whose recipients do not match once the memo no longer hides the output', async function () { + const txBase64 = await buildRaw(wallet.pub, attacker, '1000', memoFor(wallet.pub)); + await basecoin + .verifyTransaction({ + txParams: { recipients: [] }, + txPrebuild: { txBase64 }, + feePayer: wallet.pub, + blockhash: blockHash, + wallet: walletObj, + } as any) + .should.be.rejectedWith('Number of tx outputs does not match with number of txParams recipients'); + }); + + it('should reject a Marinade unstake whose transfer is not sent from the wallet root', async function () { + const otherSender = stakeAccount.pub; + const txBase64 = await buildRaw(otherSender, marinadeOrder, '2300000', memoFor(otherSender)); + await basecoin + .verifyTransaction({ + txParams: {}, + txPrebuild: { txBase64 }, + feePayer: otherSender, + blockhash: blockHash, + wallet: walletObj, + } as any) + .should.be.rejectedWith('Marinade unstake transfer must be sent from the wallet root address'); + }); + + it('should accept a genuine Marinade unstake sent from the wallet root', async function () { + const txBase64 = await buildRaw(wallet.pub, marinadeOrder, '2300000', memoFor(wallet.pub)); + const result = await basecoin.verifyTransaction({ + txParams: {}, + txPrebuild: { txBase64 }, feePayer: wallet.pub, blockhash: blockHash, wallet: walletObj, - } as any) - .should.be.rejectedWith('Invalid transaction type: transfer intent decoded as staking deactivate'); + } as any); + result.should.equal(true); + }); }); it('should verify a builder-produced transaction end-to-end with a sponsored fee payer', async function () { diff --git a/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingDeactivateBuilder.ts b/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingDeactivateBuilder.ts index d381fca2a19..ad45c33e3af 100644 --- a/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingDeactivateBuilder.ts +++ b/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingDeactivateBuilder.ts @@ -244,7 +244,7 @@ describe('Sol Staking Deactivate Builder', () => { amount: '2300000', }); - const marinadeMemo = `{\\"PrepareForRevoke\\":{\\"user\\":\\"${wallet.pub}}\\",\\"amount\\":\\"500000000000\\"}`; + const marinadeMemo = `{"PrepareForRevoke":{"user":"${wallet.pub}","amount":"500000000000"}}`; it('Marinade: build and sign a staking deactivate tx', async () => { await performTest({