From f3d445e45402f7a1c7f53e34d0596e180310380f Mon Sep 17 00:00:00 2001 From: maheshbitgo Date: Tue, 11 Aug 2026 14:18:34 +0530 Subject: [PATCH 1/3] feat(sdk-coin-sol): add verifyTransaction validation for staking authorize SOL authorize transactions carry no recipients by design, so 'authorize' was added to NO_RECIPIENT_TX_TYPES in WCI-1111 to keep the signing flow working. That left sol.ts:verifyTransaction with no checks at all for these transactions: a compromised server could present a txHex that rotates a stake account's withdraw authority to an attacker key and the client would sign it without noticing. Thread the authorize intent fields through to the coin layer and validate the decoded instruction against them: - sdk-core baseTypes.ts / iBaseCoin.ts: add newWithdrawPublicKey and stakeAccount to PopulatedIntent and TransactionParams - sdk-core recipientUtils.ts: propagate both fields from the intent in resolveEffectiveTxParams, so they reach verifyTransaction via the existing txParams argument without new plumbing in signRequestBase - sdk-coin-sol explainTransactionWasm.ts / transaction.ts: populate explainedTx.stakingAuthorize, preferring the Withdrawer instruction over Staker so the security-critical newWithdrawAddress is not dropped when a tx changes both authorities - sdk-coin-sol sol.ts: validate oldWithdrawAddress against the wallet root address, newWithdrawAddress against the intended newWithdrawPublicKey, and stakingAddress against the intended stakeAccount, whenever those intent fields are present The staker/withdrawer distinction matters because verifyTransaction always explains via the legacy Transaction.explainTransaction path, never the WASM one. Neither instruction parser surfaces Solana's stakeAuthorizationType, so a Withdrawer-type instruction is identified by its custodian key; a staker-only authorize populates the staking authority fields and leaves the withdraw fields empty rather than reporting staker addresses as withdraw addresses. The authorize checks deliberately fall through to the rest of verifyTransaction rather than returning early, so authorize transactions remain subject to the fee payer, durable nonce, memo and recipient checks. TICKET: CHALO-1294 --- .../src/lib/explainTransactionWasm.ts | 10 +- modules/sdk-coin-sol/src/lib/transaction.ts | 38 ++++- modules/sdk-coin-sol/src/sol.ts | 42 ++++++ modules/sdk-coin-sol/test/unit/sol.ts | 137 ++++++++++++++++++ modules/sdk-coin-sol/test/unit/transaction.ts | 65 +++++++++ .../sdk-core/src/bitgo/baseCoin/iBaseCoin.ts | 4 + .../sdk-core/src/bitgo/utils/tss/baseTypes.ts | 4 + .../src/bitgo/utils/tss/recipientUtils.ts | 11 ++ .../unit/bitgo/utils/tss/recipientUtils.ts | 32 ++++ 9 files changed, 339 insertions(+), 4 deletions(-) diff --git a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts index 90e2733291..43a7a59ef6 100644 --- a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts +++ b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts @@ -272,11 +272,17 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So } // --- Staking authorize --- + // A standard authorize tx contains two instructions: one for Staker and one + // for Withdrawer authority. Prefer the Withdrawer instruction for the + // stakingAuthorize summary because newWithdrawAddress is the security-critical + // field validated in verifyTransaction. Fall back to the first instruction + // if no Withdrawer instruction is present. let stakingAuthorize: StakingAuthorizeParams | undefined; for (const instr of parsed.instructionsData) { if (instr.type === 'StakingAuthorize') { - stakingAuthorize = mapStakingAuthorize(instr); - break; + if (!stakingAuthorize || instr.authorizeType === 'Withdrawer') { + stakingAuthorize = mapStakingAuthorize(instr); + } } } diff --git a/modules/sdk-coin-sol/src/lib/transaction.ts b/modules/sdk-coin-sol/src/lib/transaction.ts index e91bfcb1d3..83df2b5fdb 100644 --- a/modules/sdk-coin-sol/src/lib/transaction.ts +++ b/modules/sdk-coin-sol/src/lib/transaction.ts @@ -33,6 +33,7 @@ import { Memo, Nonce, StakingActivate, + StakingAuthorize, StakingAuthorizeParams, StakingWithdraw, TokenTransfer, @@ -540,6 +541,7 @@ export class Transaction extends BaseTransaction { const outputs: TransactionRecipient[] = []; // Create a separate array for token enablements const tokenEnablements: ITokenEnablement[] = []; + let stakingAuthorize: StakingAuthorizeParams | undefined = undefined; for (const instruction of decodedInstructions) { switch (instruction.type) { @@ -598,6 +600,36 @@ export class Transaction extends BaseTransaction { tokenAddress: ataInit.params.mintAddress, }); break; + case InstructionBuilderTypes.StakingAuthorize: { + const authorizeInstruction = instruction as StakingAuthorize; + // Neither instruction parser surfaces Solana's stakeAuthorizationType, so a + // Withdrawer-type authorize is identified by its custodian key: the standard + // parser surfaces it as newWithdrawAddress, the raw parser as custodianAddress. + // A standard authorize tx carries both a Staker and a Withdrawer instruction; + // the Withdrawer one wins because newWithdrawAddress is what verifyTransaction + // validates. Staker-only instructions must not populate the withdraw fields, + // otherwise a staker address would be compared against an intended withdraw key. + const isWithdrawerAuthorize = !!( + authorizeInstruction.params.newWithdrawAddress || authorizeInstruction.params.custodianAddress + ); + if (isWithdrawerAuthorize) { + stakingAuthorize = { + stakingAddress: authorizeInstruction.params.stakingAddress, + oldWithdrawAddress: authorizeInstruction.params.oldAuthorizeAddress, + newWithdrawAddress: authorizeInstruction.params.newAuthorizeAddress, + custodianAddress: authorizeInstruction.params.custodianAddress, + }; + } else if (!stakingAuthorize) { + stakingAuthorize = { + stakingAddress: authorizeInstruction.params.stakingAddress, + oldWithdrawAddress: '', + newWithdrawAddress: '', + oldStakingAuthorityAddress: authorizeInstruction.params.oldAuthorizeAddress, + newStakingAuthorityAddress: authorizeInstruction.params.newAuthorizeAddress, + }; + } + break; + } case InstructionBuilderTypes.CustomInstruction: // Custom instructions are arbitrary and cannot be explained break; @@ -617,7 +649,7 @@ export class Transaction extends BaseTransaction { } } - return this.getExplainedTransaction(outputAmount, outputs, memo, durableNonce, tokenEnablements); + return this.getExplainedTransaction(outputAmount, outputs, memo, durableNonce, tokenEnablements, stakingAuthorize); } private calculateFee(): string { @@ -638,7 +670,8 @@ export class Transaction extends BaseTransaction { outputs: TransactionRecipient[], memo: undefined | string = undefined, durableNonce: undefined | DurableNonceParams = undefined, - tokenEnablements: ITokenEnablement[] = [] + tokenEnablements: ITokenEnablement[] = [], + stakingAuthorize: StakingAuthorizeParams | undefined = undefined ): TransactionExplanation { const feeString = this.calculateFee(); @@ -674,6 +707,7 @@ export class Transaction extends BaseTransaction { blockhash: this.getNonce(), durableNonce: durableNonce, tokenEnablements: tokenEnablements, + ...(stakingAuthorize && { stakingAuthorize }), }; return explanation; diff --git a/modules/sdk-coin-sol/src/sol.ts b/modules/sdk-coin-sol/src/sol.ts index c426ebe05c..14a0b04fec 100644 --- a/modules/sdk-coin-sol/src/sol.ts +++ b/modules/sdk-coin-sol/src/sol.ts @@ -571,6 +571,48 @@ export class Sol extends BaseCoin { } } + const isStakingAuthorizeTx = + transaction.type === TransactionType.StakingAuthorize || + transaction.type === TransactionType.StakingAuthorizeRaw || + txParams.type === 'authorize'; + if (isStakingAuthorizeTx) { + const authorizeParams = explainedTx.stakingAuthorize; + if (!authorizeParams) { + throw new Error('StakingAuthorize transaction is missing stakingAuthorize explanation fields'); + } + // oldWithdrawAddress is '' for staker-only instructions (no Withdrawer authority change). + // Only validate when it is a non-empty string — an empty string indicates the instruction + // changes staker authority only, not withdrawer, so the wallet root check does not apply. + if ( + walletRootAddress && + authorizeParams.oldWithdrawAddress && + authorizeParams.oldWithdrawAddress !== walletRootAddress + ) { + throw new Error( + 'StakingAuthorize oldWithdrawAddress does not match wallet root address: expected ' + + walletRootAddress + + ' but got ' + + authorizeParams.oldWithdrawAddress + ); + } + if (txParams.newWithdrawPublicKey && authorizeParams.newWithdrawAddress !== txParams.newWithdrawPublicKey) { + throw new Error( + 'StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey: expected ' + + txParams.newWithdrawPublicKey + + ' but got ' + + authorizeParams.newWithdrawAddress + ); + } + if (txParams.stakeAccount && authorizeParams.stakingAddress !== txParams.stakeAccount) { + throw new Error( + 'StakingAuthorize stakingAddress does not match intended stakeAccount: expected ' + + txParams.stakeAccount + + ' but got ' + + authorizeParams.stakingAddress + ); + } + } + const isTokenEnablementTx = txParams.type === 'enabletoken'; // users do not input recipients for consolidation requests as they are generated by the server // Close-ATA txs do not populate explainedTx.outputs; recipients carry ATA addresses for intent only. diff --git a/modules/sdk-coin-sol/test/unit/sol.ts b/modules/sdk-coin-sol/test/unit/sol.ts index 9d8b4e4196..f15c95e7f0 100644 --- a/modules/sdk-coin-sol/test/unit/sol.ts +++ b/modules/sdk-coin-sol/test/unit/sol.ts @@ -1000,6 +1000,143 @@ describe('SOL:', function () { } as any); validTransaction.should.equal(true); }); + + describe('staking authorize transaction verification', function () { + const newWithdrawKey = new KeyPair(resources.authAccount2).getKeys(); + // a key that is not involved in the authorize tx — used to simulate malicious substitution + const differentKey = new KeyPair(resources.splitStakeAccount).getKeys(); + + const buildAuthorizeTx = async (newAuthorizedAddress: string) => { + const tx = await factory + .getStakingAuthorizeBuilder() + .stakingAddress(stakeAccount.pub) + .sender(wallet.pub) + .nonce(blockHash) + .newAuthorizedAddress(newAuthorizedAddress) + .oldAuthorizedAddress(wallet.pub) + .fee({ amount: 5000 }) + .build(); + return tx.toBroadcastFormat(); + }; + + it('should verify a valid staking authorize transaction with all intent fields', async function () { + const txBase64 = await buildAuthorizeTx(newWithdrawKey.pub); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = txBase64; + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + txParams.newWithdrawPublicKey = newWithdrawKey.pub; + txParams.stakeAccount = stakeAccount.pub; + const result = await basecoin.verifyTransaction({ + txParams, + txPrebuild, + wallet: walletObj, + } as any); + result.should.equal(true); + }); + + it('should verify a valid staking authorize transaction without optional intent fields', async function () { + const txBase64 = await buildAuthorizeTx(newWithdrawKey.pub); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = txBase64; + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + // newWithdrawPublicKey and stakeAccount not set — skips those checks + const result = await basecoin.verifyTransaction({ + txParams, + txPrebuild, + wallet: walletObj, + } as any); + result.should.equal(true); + }); + + it('should reject a staking authorize transaction where newWithdrawAddress was swapped to attacker key', async function () { + const txBase64 = await buildAuthorizeTx(differentKey.pub); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = txBase64; + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + // Intent says newWithdrawPublicKey should be newWithdrawKey, but tx has attacker key + txParams.newWithdrawPublicKey = newWithdrawKey.pub; + txParams.stakeAccount = stakeAccount.pub; + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) + .should.rejectedWith(/StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey/); + }); + + it('should reject a staking authorize transaction where stakeAccount does not match', async function () { + const txBase64 = await buildAuthorizeTx(newWithdrawKey.pub); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = txBase64; + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + txParams.newWithdrawPublicKey = newWithdrawKey.pub; + // Pass a different stakeAccount (attacker has replaced it) + txParams.stakeAccount = differentKey.pub; + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) + .should.rejectedWith(/StakingAuthorize stakingAddress does not match intended stakeAccount/); + }); + + it('should reject a staking authorize transaction where oldWithdrawAddress does not match wallet root', async function () { + // Build tx where oldAuthorizedAddress is NOT wallet.pub + const tx = await factory + .getStakingAuthorizeBuilder() + .stakingAddress(stakeAccount.pub) + .sender(newWithdrawKey.pub) + .nonce(blockHash) + .newAuthorizedAddress(newWithdrawKey.pub) + .oldAuthorizedAddress(newWithdrawKey.pub) // different from walletObj root + .fee({ amount: 5000 }) + .build(); + const txBase64 = tx.toBroadcastFormat(); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = txBase64; + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + txParams.newWithdrawPublicKey = newWithdrawKey.pub; + txParams.stakeAccount = stakeAccount.pub; + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) + .should.rejectedWith(/StakingAuthorize oldWithdrawAddress does not match wallet root address/); + }); + + it('should still enforce the fee payer check on a staking authorize transaction', async function () { + // oldAuthorizedAddress stays the wallet root so the authorize checks pass, but the + // fee payer is someone else — the authorize branch must not short-circuit that check. + const tx = await factory + .getStakingAuthorizeBuilder() + .stakingAddress(stakeAccount.pub) + .sender(wallet.pub) + .nonce(blockHash) + .newAuthorizedAddress(newWithdrawKey.pub) + .oldAuthorizedAddress(wallet.pub) + .feePayer(differentKey.pub) + .fee({ amount: 5000 }) + .build(); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = tx.toBroadcastFormat(); + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + txParams.newWithdrawPublicKey = newWithdrawKey.pub; + txParams.stakeAccount = stakeAccount.pub; + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) + .should.rejectedWith('Tx fee payer is not the wallet root address'); + }); + }); }); describe('getAmountBasedOnEndianness', () => { diff --git a/modules/sdk-coin-sol/test/unit/transaction.ts b/modules/sdk-coin-sol/test/unit/transaction.ts index a5f72c69d0..7fa561d871 100644 --- a/modules/sdk-coin-sol/test/unit/transaction.ts +++ b/modules/sdk-coin-sol/test/unit/transaction.ts @@ -1122,4 +1122,69 @@ describe('Sol Transaction', () => { }); }); }); + + describe('StakingAuthorize explainTransaction (non-WASM path)', () => { + // The 'sol' coin (mainnet) uses the legacy non-WASM explainTransaction path, + // unlike 'tsol' which routes through the WASM explainer. This ensures the new + // case InstructionBuilderTypes.StakingAuthorize block in transaction.ts is covered. + const solCoin = coins.get('sol'); + const factory = getBuilderFactory('sol'); + const wallet = new KeyPair(testData.authAccount).getKeys(); + const stakeAccount = new KeyPair(testData.stakeAccount).getKeys(); + const newWithdrawKey = new KeyPair(testData.authAccount2).getKeys(); + const blockHash = testData.blockHashes.validBlockHashes[0]; + + it('should populate stakingAuthorize with Withdrawer fields from a standard two-instruction authorize tx', async () => { + const tx = await factory + .getStakingAuthorizeBuilder() + .stakingAddress(stakeAccount.pub) + .sender(wallet.pub) + .nonce(blockHash) + .newAuthorizedAddress(newWithdrawKey.pub) + .oldAuthorizedAddress(wallet.pub) + .fee({ amount: 5000 }) + .build(); + + const rawTxBase64 = tx.toBroadcastFormat(); + const solTx = new Transaction(solCoin); + solTx.fromRawTransaction(rawTxBase64); + const explained = solTx.explainTransaction(); + + should.exist(explained.stakingAuthorize); + explained.stakingAuthorize!.stakingAddress.should.equal(stakeAccount.pub); + explained.stakingAuthorize!.oldWithdrawAddress.should.equal(wallet.pub); + explained.stakingAuthorize!.newWithdrawAddress.should.equal(newWithdrawKey.pub); + }); + + it('should leave withdraw fields empty for a staker-only authorize tx', async () => { + const tx = await factory + .getStakingAuthorizeBuilder() + .stakingAddress(stakeAccount.pub) + .sender(wallet.pub) + .nonce(blockHash) + .newAuthorizedAddress(newWithdrawKey.pub) + .oldAuthorizedAddress(wallet.pub) + .fee({ amount: 5000 }) + .build(); + + // Drop the Withdrawer instruction, keeping only the Staker authorize instruction. + const full = SolTransaction.from(Buffer.from(tx.toBroadcastFormat(), 'base64')); + const stakerOnly = new SolTransaction(); + stakerOnly.recentBlockhash = full.recentBlockhash; + stakerOnly.feePayer = full.feePayer; + stakerOnly.add(full.instructions[0]); + + const solTx = new Transaction(solCoin); + solTx.fromRawTransaction( + stakerOnly.serialize({ requireAllSignatures: false, verifySignatures: false }).toString('base64') + ); + const explained = solTx.explainTransaction(); + + should.exist(explained.stakingAuthorize); + explained.stakingAuthorize!.oldWithdrawAddress.should.equal(''); + explained.stakingAuthorize!.newWithdrawAddress.should.equal(''); + explained.stakingAuthorize!.oldStakingAuthorityAddress!.should.equal(wallet.pub); + explained.stakingAuthorize!.newStakingAuthorityAddress!.should.equal(newWithdrawKey.pub); + }); + }); }); diff --git a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts index ccd5a4adf4..5a38227edc 100644 --- a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts +++ b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts @@ -287,6 +287,10 @@ export interface TransactionParams { memo?: Memo; enableTokens?: TokenEnablement[]; stakingRequestId?: string; + /** SOL authorize: new withdraw authority public key from the intent. */ + newWithdrawPublicKey?: string; + /** SOL authorize: stake account address from the intent. */ + stakeAccount?: string; } export interface AddressVerificationData { diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts index 96c4d14d88..98c1b63517 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts @@ -491,6 +491,10 @@ export interface PopulatedIntent extends PopulatedIntentBase, DefiIntentFields { clientOnboarder?: string; /** Optional ISO 8601 expiration timestamp (cantonParticipantOnboardingRequest intent). */ expirationIso?: string; + /** SOL authorize intent: new withdraw authority public key. */ + newWithdrawPublicKey?: string; + /** SOL authorize intent: stake account address being re-authorized. */ + stakeAccount?: string; } export type TxRequestState = diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index d7116dea0d..994b3a714c 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -138,6 +138,17 @@ export function resolveEffectiveTxParams( effectiveTxParams.stakingRequestId = intentStakingRequestId; } + // Propagate SOL authorize-specific fields from the intent so sol.ts:verifyTransaction + // can validate the decoded instruction against what the user intended. + const intentNewWithdrawPublicKey = (txRequest.intent as PopulatedIntent)?.newWithdrawPublicKey; + if (intentNewWithdrawPublicKey && !effectiveTxParams.newWithdrawPublicKey) { + effectiveTxParams.newWithdrawPublicKey = intentNewWithdrawPublicKey; + } + const intentStakeAccount = (txRequest.intent as PopulatedIntent)?.stakeAccount; + if (intentStakeAccount && !effectiveTxParams.stakeAccount) { + effectiveTxParams.stakeAccount = intentStakeAccount; + } + // All staking intents (BSC delegate/undelegate, CELO stake/unstake, etc.) carry // stakingRequestId as a required field on BaseStakeIntent (@bitgo/public-types). // Use its presence as a generic staking signal — no need to enumerate every intentType. diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts index 72c452681f..829bdac2d6 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts @@ -371,6 +371,38 @@ describe('recipientUtils', function () { const txRequest = makeTxRequest({ intent: { intentType: 'stakingAuthorize' } as any }); assert.throws(() => resolveEffectiveTxParams(txRequest, {}), InvalidTransactionError); }); + + it('propagates newWithdrawPublicKey from authorize intent into effectiveTxParams', function () { + const txRequest = makeTxRequest({ + intent: { intentType: 'authorize', newWithdrawPublicKey: 'SomePubkey123' } as any, + }); + const result = resolveEffectiveTxParams(txRequest, {}); + assert.strictEqual(result.newWithdrawPublicKey, 'SomePubkey123'); + }); + + it('propagates stakeAccount from authorize intent into effectiveTxParams', function () { + const txRequest = makeTxRequest({ + intent: { intentType: 'authorize', stakeAccount: 'StakeAcct456' } as any, + }); + const result = resolveEffectiveTxParams(txRequest, {}); + assert.strictEqual(result.stakeAccount, 'StakeAcct456'); + }); + + it('does not overwrite existing newWithdrawPublicKey in txParams', function () { + const txRequest = makeTxRequest({ + intent: { intentType: 'authorize', newWithdrawPublicKey: 'IntentKey' } as any, + }); + const result = resolveEffectiveTxParams(txRequest, { newWithdrawPublicKey: 'CallerKey' } as any); + assert.strictEqual(result.newWithdrawPublicKey, 'CallerKey'); + }); + + it('does not overwrite existing stakeAccount in txParams', function () { + const txRequest = makeTxRequest({ + intent: { intentType: 'authorize', stakeAccount: 'IntentAcct' } as any, + }); + const result = resolveEffectiveTxParams(txRequest, { stakeAccount: 'CallerAcct' } as any); + assert.strictEqual(result.stakeAccount, 'CallerAcct'); + }); }); }); }); From 035159d7742ef9ed5aa2b60aa291e46923552898 Mon Sep 17 00:00:00 2001 From: maheshbitgo Date: Fri, 14 Aug 2026 14:00:42 +0530 Subject: [PATCH 2/3] fix(sdk-coin-sol): decode stake authority type when verifying authorize Select the authorize instruction that defines the withdraw authority from Solana's stakeAuthorizationType rather than from the presence of a lockup custodian. A custodian is orthogonal to StakeAuthorize: Solana permits a Staker change to carry one and a Withdrawer change to omit one. Inferring the authority type from it let a crafted transaction pair a real Withdrawer change to an attacker key with a later Staker change to the expected key; both matched the custodian heuristic, the decoy overwrote the real change, and verifyTransaction compared the decoy and passed. This affected the legacy web3.js parse path used by mainnet. The WASM path already read authorizeType and was not vulnerable. Both instruction parsers now surface the decoded authority type, and all three explain paths share one summarizer: a Withdrawer change outranks a Staker change, the last instruction of a type wins to match Solana's sequential execution, and withdraw fields stay empty when no Withdrawer change is present so a staker address is never compared against an intended withdraw key. An unrecognised authority type is rejected rather than defaulted. Also fixes the raw AuthorizeChecked path, which read only instructions[1] and asserted exactly two instructions, so a legitimate message carrying both a Staker and a Withdrawer change was mis-explained or rejected outright. verifyTransaction now fails closed: a missing newWithdrawPublicKey or stakeAccount aborts instead of skipping the comparison, since a server able to omit a field could otherwise disable the check meant to constrain it. SolAuthorizeIntent declares both as required. TICKET: CHALO-1294 --- .../src/lib/explainTransactionWasm.ts | 64 +++------- modules/sdk-coin-sol/src/lib/iface.ts | 25 +++- .../src/lib/instructionParamsFactory.ts | 63 ++++++++-- .../src/lib/stakingAuthorizeSummary.ts | 74 ++++++++++++ modules/sdk-coin-sol/src/lib/transaction.ts | 91 +++++++------- modules/sdk-coin-sol/src/sol.ts | 44 ++++--- modules/sdk-coin-sol/test/unit/sol.ts | 111 ++++++++++++++++-- .../test/unit/stakingAuthorizeSummary.ts | 69 +++++++++++ modules/sdk-coin-sol/test/unit/transaction.ts | 97 ++++++++++++++- .../stakingRawMsgAuthorizeBuilder.ts | 2 + 10 files changed, 510 insertions(+), 130 deletions(-) create mode 100644 modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts create mode 100644 modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts diff --git a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts index 43a7a59ef6..632409f5b1 100644 --- a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts +++ b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts @@ -1,8 +1,9 @@ import { ITokenEnablement } from '@bitgo/sdk-core'; import { Transaction, parseTransaction, type ParsedTransaction, type InstructionParams } from '@bitgo/wasm-solana'; import { UNAVAILABLE_TEXT } from './constants'; -import { StakingAuthorizeParams, TransactionExplanation as SolLibTransactionExplanation } from './iface'; +import { TransactionExplanation as SolLibTransactionExplanation } from './iface'; import { findTokenName } from './instructionParamsFactory'; +import { summarizeStakingAuthorize } from './stakingAuthorizeSummary'; export interface ExplainTransactionWasmOptions { txBase64: string; @@ -132,39 +133,6 @@ function extractTransactionId(signatures: string[]): string | undefined { return sig; } -// ============================================================================= -// Staking authorize mapping -// ============================================================================= - -/** - * Map WASM StakingAuthorize instruction to the legacy BitGoJS shape. - * BitGoJS uses different field names for Staker vs Withdrawer authority changes. - */ -function mapStakingAuthorize(instr: { - stakingAddress: string; - oldAuthorizeAddress: string; - newAuthorizeAddress: string; - authorizeType: 'Staker' | 'Withdrawer'; - custodianAddress?: string; -}): StakingAuthorizeParams { - if (instr.authorizeType === 'Withdrawer') { - return { - stakingAddress: instr.stakingAddress, - oldWithdrawAddress: instr.oldAuthorizeAddress, - newWithdrawAddress: instr.newAuthorizeAddress, - custodianAddress: instr.custodianAddress, - }; - } - // Staker authority change - return { - stakingAddress: instr.stakingAddress, - oldWithdrawAddress: '', - newWithdrawAddress: '', - oldStakingAuthorityAddress: instr.oldAuthorizeAddress, - newStakingAuthorityAddress: instr.newAuthorizeAddress, - }; -} - // ============================================================================= // Main explain function // ============================================================================= @@ -272,19 +240,21 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So } // --- Staking authorize --- - // A standard authorize tx contains two instructions: one for Staker and one - // for Withdrawer authority. Prefer the Withdrawer instruction for the - // stakingAuthorize summary because newWithdrawAddress is the security-critical - // field validated in verifyTransaction. Fall back to the first instruction - // if no Withdrawer instruction is present. - let stakingAuthorize: StakingAuthorizeParams | undefined; - for (const instr of parsed.instructionsData) { - if (instr.type === 'StakingAuthorize') { - if (!stakingAuthorize || instr.authorizeType === 'Withdrawer') { - stakingAuthorize = mapStakingAuthorize(instr); - } - } - } + // Summarised by the same rules as the legacy and raw parse paths, so all three agree on which + // instruction defines the withdraw authority that verifyTransaction validates. + const stakingAuthorize = summarizeStakingAuthorize( + parsed.instructionsData + .filter( + (instr): instr is Extract => instr.type === 'StakingAuthorize' + ) + .map((instr) => ({ + stakingAddress: instr.stakingAddress, + oldAuthorizeAddress: instr.oldAuthorizeAddress, + newAuthorizeAddress: instr.newAuthorizeAddress, + authorizeType: instr.authorizeType, + custodianAddress: instr.custodianAddress, + })) + ); // --- Resolve token names and convert bigint to string at serialization boundary --- const resolvedOutputs = outputs.map((o) => ({ diff --git a/modules/sdk-coin-sol/src/lib/iface.ts b/modules/sdk-coin-sol/src/lib/iface.ts index 44de38ffb2..5012083fe8 100644 --- a/modules/sdk-coin-sol/src/lib/iface.ts +++ b/modules/sdk-coin-sol/src/lib/iface.ts @@ -181,14 +181,37 @@ export interface StakingWithdraw { params: { fromAddress: string; stakingAddress: string; amount: string }; } +/** + * Which stake authority an Authorize instruction changes. Mirrors Solana's `StakeAuthorize` + * enum (`StakeAuthorizationLayout`: `Staker` = 0, `Withdrawer` = 1) and the `authorizeType` + * emitted by the WASM parser, so all parse paths share one vocabulary. + */ +export type StakeAuthorizeType = 'Staker' | 'Withdrawer'; + export interface StakingAuthorize { type: InstructionBuilderTypes.StakingAuthorize; params: { stakingAddress: string; - oldAuthorizeAddress; + /** The authority signing the change — the staker or the withdrawer, per `authorizeType`. */ + oldAuthorizeAddress: string; + /** The new authority — the new staker or the new withdrawer, per `authorizeType`. */ newAuthorizeAddress: string; + /** + * The lockup custodian account, NOT a withdraw authority. Named `newWithdrawAddress` for + * historical reasons: {@link stakingAuthorizeInstruction} passes it as `custodianPubkey`, and + * the decoder reads that account back into this field. Its presence is orthogonal to whether + * the instruction changes the staker or the withdrawer — use `authorizeType` for that. + */ newWithdrawAddress?: string; + /** The lockup custodian account, as decoded by the raw (AuthorizeChecked) parser. */ custodianAddress?: string; + /** + * Which authority this instruction changes, decoded from Solana's `stakeAuthorizationType`. + * This is the only authoritative discriminator; do not infer it from custodian presence. + * Optional because transaction *builders* construct these params before encoding, where the + * type is implied by the builder. Both instruction parsers always populate it. + */ + authorizeType?: StakeAuthorizeType; }; } diff --git a/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts b/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts index 446a4d66ac..8ab9f173e6 100644 --- a/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts +++ b/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts @@ -20,6 +20,7 @@ import { DelegateStakeParams, InitializeStakeParams, SplitStakeParams, + StakeAuthorizationLayout, StakeInstruction, StakeProgram, SystemInstruction, @@ -41,6 +42,7 @@ import { Memo, MintTo, Nonce, + StakeAuthorizeType, StakingActivate, StakingAuthorize, StakingDeactivate, @@ -1171,6 +1173,35 @@ function parseAtaCloseInstructions(instructions: TransactionInstruction[]): Arra return instructionData; } +/** + * Map Solana's numeric `stakeAuthorizationType` onto our discriminator. + * + * `StakeAuthorizationLayout` defines `Staker = 0` and `Withdrawer = 1`. Any other value is + * rejected rather than defaulted, so an unrecognised authority type can never be silently + * treated as a staker change (which would suppress the withdraw-authority validation). + */ +function toStakeAuthorizeType(index: number): StakeAuthorizeType { + if (index === StakeAuthorizationLayout.Withdrawer.index) { + return 'Withdrawer'; + } + if (index === StakeAuthorizationLayout.Staker.index) { + return 'Staker'; + } + throw new NotSupported(`Invalid transaction, unknown stake authorization type: ${index}`); +} + +/** + * Decode the authority type of a raw `AuthorizeChecked` instruction. + * + * The raw path exists because web3.js cannot decode `AuthorizeChecked`, so the type is read + * straight from the instruction data: a little-endian u32 opcode (10) followed by a + * little-endian u32 `StakeAuthorize` discriminant. + */ +function decodeRawAuthorizeType(instruction: TransactionInstruction): StakeAuthorizeType { + assert(instruction.data.length >= 8, 'Invalid authorize instruction data'); + return toStakeAuthorizeType(instruction.data.readUInt32LE(4)); +} + /** * Parses Solana instructions to authorized staking account params * Only supports Nonce, Authorize instructions @@ -1210,7 +1241,9 @@ function parseStakingAuthorizeInstructions( stakingAddress: authorize.stakePubkey.toString(), oldAuthorizeAddress: authorize.authorizedPubkey.toString(), newAuthorizeAddress: authorize.newAuthorizedPubkey.toString(), + // The custodian (lockup authority) account, not a withdraw authority — see iface. newWithdrawAddress: authorize.custodianPubkey?.toString() || '', + authorizeType: toStakeAuthorizeType(authorize.stakeAuthorizationType.index), }, }); break; @@ -1229,7 +1262,9 @@ function parseStakingAuthorizeInstructions( */ function parseStakingAuthorizeRawInstructions(instructions: TransactionInstruction[]): Array { const instructionData: Array = []; - assert(instructions.length === 2, 'Invalid number of instructions'); + // validateRawMsgInstruction accepts a nonce advance followed by one or two authorize + // instructions (a Staker change, a Withdrawer change, or both). + assert(instructions.length === 2 || instructions.length === 3, 'Invalid number of instructions'); const advanceNonceInstruction = SystemInstruction.decodeNonceAdvance(instructions[0]); const nonce: Nonce = { type: InstructionBuilderTypes.NonceAdvance, @@ -1239,17 +1274,21 @@ function parseStakingAuthorizeRawInstructions(instructions: TransactionInstructi }, }; instructionData.push(nonce); - const authorize = instructions[1]; - assert(authorize.keys.length === 5, 'Invalid number of keys in authorize instruction'); - instructionData.push({ - type: InstructionBuilderTypes.StakingAuthorize, - params: { - stakingAddress: authorize.keys[0].pubkey.toString(), - oldAuthorizeAddress: authorize.keys[2].pubkey.toString(), - newAuthorizeAddress: authorize.keys[3].pubkey.toString(), - custodianAddress: authorize.keys[4].pubkey.toString(), - }, - }); + for (const authorize of instructions.slice(1)) { + // AuthorizeChecked accounts: [0] stake, [1] clock sysvar, [2] current authority, + // [3] new authority, [4] optional lockup custodian. + assert(authorize.keys.length === 5, 'Invalid number of keys in authorize instruction'); + instructionData.push({ + type: InstructionBuilderTypes.StakingAuthorize, + params: { + stakingAddress: authorize.keys[0].pubkey.toString(), + oldAuthorizeAddress: authorize.keys[2].pubkey.toString(), + newAuthorizeAddress: authorize.keys[3].pubkey.toString(), + custodianAddress: authorize.keys[4].pubkey.toString(), + authorizeType: decodeRawAuthorizeType(authorize), + }, + }); + } return instructionData; } diff --git a/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts b/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts new file mode 100644 index 0000000000..fe43f07f84 --- /dev/null +++ b/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts @@ -0,0 +1,74 @@ +import { StakeAuthorizeType, StakingAuthorizeParams } from './iface'; + +/** + * A single decoded Authorize instruction, normalised across the three parse paths + * (legacy web3.js `Authorize`, raw `AuthorizeChecked`, and the WASM parser). + */ +export interface AuthorizeInstructionView { + stakingAddress: string; + /** The authority signing this change — staker or withdrawer, per `authorizeType`. */ + oldAuthorizeAddress: string; + /** The new authority — new staker or new withdrawer, per `authorizeType`. */ + newAuthorizeAddress: string; + /** Decoded from Solana's `stakeAuthorizationType`. Undefined only if decoding failed. */ + authorizeType?: StakeAuthorizeType; + /** The lockup custodian account, if the instruction carries one. */ + custodianAddress?: string; +} + +/** + * Reduce the Authorize instructions of a transaction to the single summary that + * `verifyTransaction` validates against the signing intent. + * + * Selection rules, and why: + * + * - The authority being changed is taken **only** from `authorizeType`, never inferred from the + * presence of a custodian. A lockup custodian is orthogonal to `StakeAuthorize` — Solana allows + * a Staker change to carry one and a Withdrawer change to omit one — so inferring the type from + * it lets a crafted transaction pair a real Withdrawer change to an attacker key with a decoy + * Staker change to the expected key, and have the decoy reported as the withdraw authority. + * - A Withdrawer change always outranks a Staker change, because `newWithdrawAddress` is the + * security-critical field and must not be masked by a staker-only instruction. + * - Among instructions of the same authority type the **last** one wins, matching Solana's + * sequential execution: the final on-chain authority is the one set by the last instruction. + * - Withdraw fields are left empty for a staker-only transaction so a staker address is never + * compared against an intended withdraw key. `verifyTransaction` rejects the empty value when + * the intent expects a withdraw-authority change. + * + * @param instructions decoded Authorize instructions, in transaction order + * @returns the summary, or undefined when the transaction has no Authorize instruction + */ +export function summarizeStakingAuthorize( + instructions: AuthorizeInstructionView[] +): StakingAuthorizeParams | undefined { + if (instructions.length === 0) { + return undefined; + } + + const lastOfType = (type: StakeAuthorizeType): AuthorizeInstructionView | undefined => + [...instructions].reverse().find((instruction) => instruction.authorizeType === type); + + const withdrawer = lastOfType('Withdrawer'); + if (withdrawer) { + return { + stakingAddress: withdrawer.stakingAddress, + oldWithdrawAddress: withdrawer.oldAuthorizeAddress, + newWithdrawAddress: withdrawer.newAuthorizeAddress, + custodianAddress: withdrawer.custodianAddress, + }; + } + + // No withdraw-authority change in this transaction. Report the staker change (if the type was + // decodable at all) and leave the withdraw fields empty. + const staker = lastOfType('Staker'); + const fallback = staker ?? instructions[instructions.length - 1]; + return { + stakingAddress: fallback.stakingAddress, + oldWithdrawAddress: '', + newWithdrawAddress: '', + ...(staker && { + oldStakingAuthorityAddress: staker.oldAuthorizeAddress, + newStakingAuthorityAddress: staker.newAuthorizeAddress, + }), + }; +} diff --git a/modules/sdk-coin-sol/src/lib/transaction.ts b/modules/sdk-coin-sol/src/lib/transaction.ts index 83df2b5fdb..a878960527 100644 --- a/modules/sdk-coin-sol/src/lib/transaction.ts +++ b/modules/sdk-coin-sol/src/lib/transaction.ts @@ -1,3 +1,4 @@ +import assert from 'assert'; import { BaseTransaction, Entry, @@ -43,6 +44,7 @@ import { VersionedTransactionData, WalletInit, } from './iface'; +import { AuthorizeInstructionView, summarizeStakingAuthorize } from './stakingAuthorizeSummary'; import { instructionParamsFactory } from './instructionParamsFactory'; import { getInstructionType, @@ -541,7 +543,7 @@ export class Transaction extends BaseTransaction { const outputs: TransactionRecipient[] = []; // Create a separate array for token enablements const tokenEnablements: ITokenEnablement[] = []; - let stakingAuthorize: StakingAuthorizeParams | undefined = undefined; + const authorizeInstructions: AuthorizeInstructionView[] = []; for (const instruction of decodedInstructions) { switch (instruction.type) { @@ -601,33 +603,19 @@ export class Transaction extends BaseTransaction { }); break; case InstructionBuilderTypes.StakingAuthorize: { - const authorizeInstruction = instruction as StakingAuthorize; - // Neither instruction parser surfaces Solana's stakeAuthorizationType, so a - // Withdrawer-type authorize is identified by its custodian key: the standard - // parser surfaces it as newWithdrawAddress, the raw parser as custodianAddress. - // A standard authorize tx carries both a Staker and a Withdrawer instruction; - // the Withdrawer one wins because newWithdrawAddress is what verifyTransaction - // validates. Staker-only instructions must not populate the withdraw fields, - // otherwise a staker address would be compared against an intended withdraw key. - const isWithdrawerAuthorize = !!( - authorizeInstruction.params.newWithdrawAddress || authorizeInstruction.params.custodianAddress - ); - if (isWithdrawerAuthorize) { - stakingAuthorize = { - stakingAddress: authorizeInstruction.params.stakingAddress, - oldWithdrawAddress: authorizeInstruction.params.oldAuthorizeAddress, - newWithdrawAddress: authorizeInstruction.params.newAuthorizeAddress, - custodianAddress: authorizeInstruction.params.custodianAddress, - }; - } else if (!stakingAuthorize) { - stakingAuthorize = { - stakingAddress: authorizeInstruction.params.stakingAddress, - oldWithdrawAddress: '', - newWithdrawAddress: '', - oldStakingAuthorityAddress: authorizeInstruction.params.oldAuthorizeAddress, - newStakingAuthorityAddress: authorizeInstruction.params.newAuthorizeAddress, - }; - } + const { params } = instruction as StakingAuthorize; + // Collect every Authorize instruction and summarise them once, after the loop. + // Selecting per-instruction here is what allowed a decoy instruction to overwrite + // the real withdraw-authority change — see summarizeStakingAuthorize. + authorizeInstructions.push({ + stakingAddress: params.stakingAddress, + oldAuthorizeAddress: params.oldAuthorizeAddress, + newAuthorizeAddress: params.newAuthorizeAddress, + authorizeType: params.authorizeType, + // The legacy parser reports the custodian in newWithdrawAddress; the raw parser + // in custodianAddress. Neither indicates which authority is being changed. + custodianAddress: params.custodianAddress || params.newWithdrawAddress || undefined, + }); break; } case InstructionBuilderTypes.CustomInstruction: @@ -649,7 +637,14 @@ export class Transaction extends BaseTransaction { } } - return this.getExplainedTransaction(outputAmount, outputs, memo, durableNonce, tokenEnablements, stakingAuthorize); + return this.getExplainedTransaction( + outputAmount, + outputs, + memo, + durableNonce, + tokenEnablements, + summarizeStakingAuthorize(authorizeInstructions) + ); } private calculateFee(): string { @@ -720,22 +715,28 @@ export class Transaction extends BaseTransaction { walletNonceAddress: nonceInstruction.noncePubkey.toString(), authWalletAddress: nonceInstruction.authorizedPubkey.toString(), }; - const data = instructions[1].data.toString('hex'); - const stakingAuthorizeParams: StakingAuthorizeParams = - data === validInstructionData - ? { - stakingAddress: instructions[1].keys[0].pubkey.toString(), - oldWithdrawAddress: instructions[1].keys[2].pubkey.toString(), - newWithdrawAddress: instructions[1].keys[3].pubkey.toString(), - custodianAddress: instructions[1].keys[4].pubkey.toString(), - } - : { - stakingAddress: instructions[1].keys[0].pubkey.toString(), - oldWithdrawAddress: '', - newWithdrawAddress: '', - oldStakingAuthorityAddress: instructions[1].keys[2].pubkey.toString(), - newStakingAuthorityAddress: instructions[1].keys[3].pubkey.toString(), - }; + // validateRawMsgInstruction accepts a nonce advance followed by one or two AuthorizeChecked + // instructions, so summarise all of them rather than only instructions[1]. Reading just the + // first one reported empty withdraw fields whenever the Staker change came first, which made + // verifyTransaction reject a legitimate two-instruction authorize. + const stakingAuthorizeParams = summarizeStakingAuthorize( + instructions.slice(1).map((instruction) => { + // AuthorizeChecked requires the stake account, clock sysvar, current authority and new + // authority; the lockup custodian is optional. Fail explicitly rather than reading past + // the end of a malformed key list. + assert(instruction.keys.length >= 4, 'Invalid number of keys in authorize instruction'); + return { + stakingAddress: instruction.keys[0].pubkey.toString(), + oldAuthorizeAddress: instruction.keys[2].pubkey.toString(), + newAuthorizeAddress: instruction.keys[3].pubkey.toString(), + custodianAddress: instruction.keys[4]?.pubkey.toString(), + // The authority type is the trailing u32 of the instruction data; validateRawMsgInstruction + // has already constrained it to exactly these two encodings. + authorizeType: + instruction.data.toString('hex') === validInstructionData ? ('Withdrawer' as const) : ('Staker' as const), + }; + }) + ); const feeString = this.calculateFee(); return { displayOrder: [ diff --git a/modules/sdk-coin-sol/src/sol.ts b/modules/sdk-coin-sol/src/sol.ts index 14a0b04fec..7e09128659 100644 --- a/modules/sdk-coin-sol/src/sol.ts +++ b/modules/sdk-coin-sol/src/sol.ts @@ -576,34 +576,46 @@ export class Sol extends BaseCoin { transaction.type === TransactionType.StakingAuthorizeRaw || txParams.type === 'authorize'; if (isStakingAuthorizeTx) { + // This check exists to stop a compromised server from substituting a txHex that rotates the + // withdraw authority to an attacker key, so every branch below fails closed: a missing + // intent field or a missing explanation aborts the signature rather than skipping the + // comparison. SolAuthorizeIntent declares stakeAccount and newWithdrawPublicKey as required, + // so their absence means the intent did not survive intact and must not be signed against. const authorizeParams = explainedTx.stakingAuthorize; if (!authorizeParams) { throw new Error('StakingAuthorize transaction is missing stakingAuthorize explanation fields'); } - // oldWithdrawAddress is '' for staker-only instructions (no Withdrawer authority change). - // Only validate when it is a non-empty string — an empty string indicates the instruction - // changes staker authority only, not withdrawer, so the wallet root check does not apply. - if ( - walletRootAddress && - authorizeParams.oldWithdrawAddress && - authorizeParams.oldWithdrawAddress !== walletRootAddress - ) { + if (!txParams.newWithdrawPublicKey) { + throw new Error('StakingAuthorize intent is missing newWithdrawPublicKey, cannot verify withdraw authority'); + } + if (!txParams.stakeAccount) { + throw new Error('StakingAuthorize intent is missing stakeAccount, cannot verify the stake account'); + } + if (!walletRootAddress) { + throw new Error('StakingAuthorize verification requires the wallet root address'); + } + // Empty when the transaction changes only the staker authority. Comparing it against the + // intended withdraw key then fails here, which is correct: an authorize intent always + // transfers the withdraw authority, so a staker-only transaction does not fulfil it. + if (authorizeParams.newWithdrawAddress !== txParams.newWithdrawPublicKey) { throw new Error( - 'StakingAuthorize oldWithdrawAddress does not match wallet root address: expected ' + - walletRootAddress + + 'StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey: expected ' + + txParams.newWithdrawPublicKey + ' but got ' + - authorizeParams.oldWithdrawAddress + (authorizeParams.newWithdrawAddress || '') ); } - if (txParams.newWithdrawPublicKey && authorizeParams.newWithdrawAddress !== txParams.newWithdrawPublicKey) { + // The wallet must be the authority it is signing away, otherwise the transaction is + // re-authorising some other account's stake. + if (authorizeParams.oldWithdrawAddress !== walletRootAddress) { throw new Error( - 'StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey: expected ' + - txParams.newWithdrawPublicKey + + 'StakingAuthorize oldWithdrawAddress does not match wallet root address: expected ' + + walletRootAddress + ' but got ' + - authorizeParams.newWithdrawAddress + authorizeParams.oldWithdrawAddress ); } - if (txParams.stakeAccount && authorizeParams.stakingAddress !== txParams.stakeAccount) { + if (authorizeParams.stakingAddress !== txParams.stakeAccount) { throw new Error( 'StakingAuthorize stakingAddress does not match intended stakeAccount: expected ' + txParams.stakeAccount + diff --git a/modules/sdk-coin-sol/test/unit/sol.ts b/modules/sdk-coin-sol/test/unit/sol.ts index f15c95e7f0..de8660e7a7 100644 --- a/modules/sdk-coin-sol/test/unit/sol.ts +++ b/modules/sdk-coin-sol/test/unit/sol.ts @@ -5,6 +5,7 @@ import * as should from 'should'; import * as sinon from 'sinon'; import { TOKEN_2022_PROGRAM_ID, TOKEN_PROGRAM_ID } from '@solana/spl-token'; +import { PublicKey, StakeAuthorizationLayout, StakeProgram, Transaction as SolTransaction } from '@solana/web3.js'; import { BitGoAPI, encrypt } from '@bitgo/sdk-api'; import { @@ -1006,6 +1007,31 @@ describe('SOL:', function () { // a key that is not involved in the authorize tx — used to simulate malicious substitution const differentKey = new KeyPair(resources.splitStakeAccount).getKeys(); + /** + * Verify a hand-crafted transaction against an authorize intent naming `newWithdrawKey`, + * on both explain implementations: 'tsol' routes through the WASM parser and 'sol' through + * the legacy web3.js parser. A crafted transaction must be rejected by both. + */ + const rejectOnBothExplainPaths = async (solTx: SolTransaction, matcher: RegExp) => { + for (const chain of ['tsol', 'sol']) { + const coin = bitgo.coin(chain) as Sol; + coin.getChain().should.equal(chain); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = solTx + .serialize({ verifySignatures: false, requireAllSignatures: false }) + .toString('base64'); + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + txParams.newWithdrawPublicKey = newWithdrawKey.pub; + txParams.stakeAccount = stakeAccount.pub; + await coin + .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) + .should.be.rejectedWith(matcher, `expected ${chain} to reject the crafted transaction`); + } + }; + const buildAuthorizeTx = async (newAuthorizedAddress: string) => { const tx = await factory .getStakingAuthorizeBuilder() @@ -1037,7 +1063,10 @@ describe('SOL:', function () { result.should.equal(true); }); - it('should verify a valid staking authorize transaction without optional intent fields', async function () { + // The intent fields are what the decoded transaction is judged against, so a missing one + // must abort rather than skip the comparison — otherwise a server that simply omits the + // field disables the very check that is meant to constrain it. + it('should reject a staking authorize transaction when newWithdrawPublicKey is absent from the intent', async function () { const txBase64 = await buildAuthorizeTx(newWithdrawKey.pub); const txParams = newTxParams(); const txPrebuild = newTxPrebuild(); @@ -1045,13 +1074,26 @@ describe('SOL:', function () { txPrebuild.txInfo.nonce = blockHash; txParams.recipients = []; txParams.type = 'authorize'; - // newWithdrawPublicKey and stakeAccount not set — skips those checks - const result = await basecoin.verifyTransaction({ - txParams, - txPrebuild, - wallet: walletObj, - } as any); - result.should.equal(true); + txParams.stakeAccount = stakeAccount.pub; + // newWithdrawPublicKey deliberately not set + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) + .should.rejectedWith(/StakingAuthorize intent is missing newWithdrawPublicKey/); + }); + + it('should reject a staking authorize transaction when stakeAccount is absent from the intent', async function () { + const txBase64 = await buildAuthorizeTx(newWithdrawKey.pub); + const txParams = newTxParams(); + const txPrebuild = newTxPrebuild(); + txPrebuild.txBase64 = txBase64; + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + txParams.newWithdrawPublicKey = newWithdrawKey.pub; + // stakeAccount deliberately not set + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) + .should.rejectedWith(/StakingAuthorize intent is missing stakeAccount/); }); it('should reject a staking authorize transaction where newWithdrawAddress was swapped to attacker key', async function () { @@ -1136,6 +1178,59 @@ describe('SOL:', function () { .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) .should.rejectedWith('Tx fee payer is not the wallet root address'); }); + + it('should reject a crafted transaction that hides a Withdrawer change behind a decoy Staker change', async function () { + // A lockup custodian is orthogonal to StakeAuthorize: Solana permits a Staker change to + // carry one. A transaction that pairs a real Withdrawer change to an attacker key with a + // later Staker change to the expected key therefore looks, to any custodian-presence + // heuristic, like two Withdrawer changes — and the decoy would be reported as the withdraw + // authority. Selection must come from stakeAuthorizationType so the real change is seen. + const solTx = new SolTransaction(); + solTx.recentBlockhash = blockHash; + solTx.feePayer = new PublicKey(wallet.pub); + solTx.add( + StakeProgram.authorize({ + stakePubkey: new PublicKey(stakeAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + newAuthorizedPubkey: new PublicKey(differentKey.pub), + stakeAuthorizationType: StakeAuthorizationLayout.Withdrawer, + custodianPubkey: new PublicKey(differentKey.pub), + }) + ); + solTx.add( + StakeProgram.authorize({ + stakePubkey: new PublicKey(stakeAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + newAuthorizedPubkey: new PublicKey(newWithdrawKey.pub), + stakeAuthorizationType: StakeAuthorizationLayout.Staker, + custodianPubkey: new PublicKey(newWithdrawKey.pub), + }) + ); + + // Asserted on both chains: 'tsol' explains via the WASM parser, 'sol' via the legacy + // web3.js parser, and each must reach the same verdict. + await rejectOnBothExplainPaths( + solTx, + /StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey/ + ); + }); + + it('should reject a staker-only authorize transaction against a withdraw-authority intent', async function () { + const solTx = new SolTransaction(); + solTx.recentBlockhash = blockHash; + solTx.feePayer = new PublicKey(wallet.pub); + solTx.add( + StakeProgram.authorize({ + stakePubkey: new PublicKey(stakeAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + newAuthorizedPubkey: new PublicKey(newWithdrawKey.pub), + stakeAuthorizationType: StakeAuthorizationLayout.Staker, + custodianPubkey: new PublicKey(newWithdrawKey.pub), + }) + ); + + await rejectOnBothExplainPaths(solTx, //); + }); }); }); diff --git a/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts b/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts new file mode 100644 index 0000000000..9098a28ea9 --- /dev/null +++ b/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts @@ -0,0 +1,69 @@ +import should from 'should'; +import { AuthorizeInstructionView, summarizeStakingAuthorize } from '../../src/lib/stakingAuthorizeSummary'; + +describe('summarizeStakingAuthorize', () => { + const stakingAddress = 'stake-account'; + const walletRoot = 'wallet-root'; + + const view = ( + authorizeType: AuthorizeInstructionView['authorizeType'], + newAuthorizeAddress: string, + custodianAddress?: string + ): AuthorizeInstructionView => ({ + stakingAddress, + oldAuthorizeAddress: walletRoot, + newAuthorizeAddress, + authorizeType, + custodianAddress, + }); + + it('returns undefined when there are no authorize instructions', () => { + should.not.exist(summarizeStakingAuthorize([])); + }); + + it('reports a Withdrawer change', () => { + const summary = summarizeStakingAuthorize([view('Withdrawer', 'new-withdrawer', 'custodian')]); + summary!.newWithdrawAddress.should.equal('new-withdrawer'); + summary!.oldWithdrawAddress.should.equal(walletRoot); + summary!.custodianAddress!.should.equal('custodian'); + }); + + it('leaves withdraw fields empty for a staker-only transaction', () => { + const summary = summarizeStakingAuthorize([view('Staker', 'new-staker')]); + summary!.newWithdrawAddress.should.equal(''); + summary!.oldWithdrawAddress.should.equal(''); + summary!.newStakingAuthorityAddress!.should.equal('new-staker'); + }); + + // A Staker change may legally carry a custodian, so it must never be mistaken for a Withdrawer + // change regardless of where it sits in the transaction. + for (const [name, instructions] of [ + ['staker decoy last', [view('Withdrawer', 'attacker', 'c'), view('Staker', 'expected', 'c')]], + ['staker decoy first', [view('Staker', 'expected', 'c'), view('Withdrawer', 'attacker', 'c')]], + ] as [string, AuthorizeInstructionView[]][]) { + it(`prefers the real Withdrawer change over a custodian-bearing Staker change (${name})`, () => { + summarizeStakingAuthorize(instructions)!.newWithdrawAddress.should.equal('attacker'); + }); + } + + // Solana executes instructions in order, so the last change of a given type is the one that + // determines the final on-chain authority. + it('takes the last Withdrawer change when several are present', () => { + const summary = summarizeStakingAuthorize([view('Withdrawer', 'first'), view('Withdrawer', 'last')]); + summary!.newWithdrawAddress.should.equal('last'); + }); + + it('takes the last Staker change when several are present', () => { + const summary = summarizeStakingAuthorize([view('Staker', 'first'), view('Staker', 'last')]); + summary!.newStakingAuthorityAddress!.should.equal('last'); + }); + + // An undecodable authority type must not be reported as a withdraw-authority change, otherwise + // verifyTransaction would compare an unknown address against the intended withdraw key. + it('leaves withdraw fields empty when the authority type is unknown', () => { + const summary = summarizeStakingAuthorize([view(undefined, 'unknown', 'custodian')]); + summary!.newWithdrawAddress.should.equal(''); + summary!.oldWithdrawAddress.should.equal(''); + should.not.exist(summary!.newStakingAuthorityAddress); + }); +}); diff --git a/modules/sdk-coin-sol/test/unit/transaction.ts b/modules/sdk-coin-sol/test/unit/transaction.ts index 7fa561d871..a1c64b7389 100644 --- a/modules/sdk-coin-sol/test/unit/transaction.ts +++ b/modules/sdk-coin-sol/test/unit/transaction.ts @@ -3,7 +3,14 @@ import should from 'should'; import { coins } from '@bitgo/statics'; import { KeyPair, Transaction } from '../../src/lib'; import * as testData from '../resources/sol'; -import { PublicKey, Transaction as SolTransaction } from '@solana/web3.js'; +import { + PublicKey, + StakeProgram, + SystemProgram, + SYSVAR_CLOCK_PUBKEY, + Transaction as SolTransaction, + TransactionInstruction, +} from '@solana/web3.js'; import { getBuilderFactory } from './getBuilderFactory'; describe('Sol Transaction', () => { @@ -1123,6 +1130,94 @@ describe('Sol Transaction', () => { }); }); + describe('StakingAuthorize explainTransaction (raw AuthorizeChecked message path)', () => { + // validateRawMsgInstruction accepts a nonce advance followed by one OR two AuthorizeChecked + // instructions. explainRawMsgAuthorizeTransaction used to read only instructions[1], so a + // two-instruction message whose Staker change came first reported empty withdraw fields. + const solCoin = coins.get('sol'); + const wallet = new KeyPair(testData.authAccount).getKeys(); + const stakeAccount = new KeyPair(testData.stakeAccount).getKeys(); + const newWithdrawKey = new KeyPair(testData.authAccount2).getKeys(); + const custodian = new KeyPair(testData.splitStakeAccount).getKeys(); + const nonceAccount = new KeyPair(testData.nonceAccount).getKeys(); + const blockHash = testData.blockHashes.validBlockHashes[0]; + + /** Build an AuthorizeChecked instruction; web3.js has no encoder for this opcode. */ + const authorizeChecked = (newAuthority: string, type: 'Staker' | 'Withdrawer') => + new TransactionInstruction({ + programId: StakeProgram.programId, + // [0] stake, [1] clock sysvar, [2] current authority, [3] new authority, [4] custodian + keys: [ + { pubkey: new PublicKey(stakeAccount.pub), isSigner: false, isWritable: true }, + { pubkey: SYSVAR_CLOCK_PUBKEY, isSigner: false, isWritable: false }, + { pubkey: new PublicKey(wallet.pub), isSigner: true, isWritable: false }, + { pubkey: new PublicKey(newAuthority), isSigner: true, isWritable: false }, + { pubkey: new PublicKey(custodian.pub), isSigner: true, isWritable: false }, + ], + // u32 opcode 10 (AuthorizeChecked) then u32 StakeAuthorize (0 Staker / 1 Withdrawer) + data: Buffer.from(type === 'Withdrawer' ? '0a00000001000000' : '0a00000000000000', 'hex'), + }); + + const buildRawMsg = (instructions: TransactionInstruction[]) => { + const solTx = new SolTransaction(); + solTx.recentBlockhash = blockHash; + solTx.feePayer = new PublicKey(wallet.pub); + solTx.add( + SystemProgram.nonceAdvance({ + noncePubkey: new PublicKey(nonceAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + }) + ); + instructions.forEach((instruction) => solTx.add(instruction)); + const tx = new Transaction(solCoin); + tx.fromRawTransaction( + solTx.serialize({ verifySignatures: false, requireAllSignatures: false }).toString('base64') + ); + return tx; + }; + + it('should report the Withdrawer change when the Staker change comes first', () => { + const explained = buildRawMsg([ + authorizeChecked(custodian.pub, 'Staker'), + authorizeChecked(newWithdrawKey.pub, 'Withdrawer'), + ]).explainTransaction(); + + should.exist(explained.stakingAuthorize); + explained.stakingAuthorize!.stakingAddress.should.equal(stakeAccount.pub); + explained.stakingAuthorize!.oldWithdrawAddress.should.equal(wallet.pub); + explained.stakingAuthorize!.newWithdrawAddress.should.equal(newWithdrawKey.pub); + }); + + it('should report the Withdrawer change when the Staker change comes second', () => { + const explained = buildRawMsg([ + authorizeChecked(newWithdrawKey.pub, 'Withdrawer'), + authorizeChecked(custodian.pub, 'Staker'), + ]).explainTransaction(); + + explained.stakingAuthorize!.newWithdrawAddress.should.equal(newWithdrawKey.pub); + }); + + it('should leave withdraw fields empty for a staker-only raw message', () => { + const explained = buildRawMsg([authorizeChecked(newWithdrawKey.pub, 'Staker')]).explainTransaction(); + + explained.stakingAuthorize!.newWithdrawAddress.should.equal(''); + explained.stakingAuthorize!.oldWithdrawAddress.should.equal(''); + explained.stakingAuthorize!.newStakingAuthorityAddress!.should.equal(newWithdrawKey.pub); + }); + + it('should decode the authority type into toJson for a two-instruction raw message', () => { + const txJson = buildRawMsg([ + authorizeChecked(custodian.pub, 'Staker'), + authorizeChecked(newWithdrawKey.pub, 'Withdrawer'), + ]).toJson(); + + // Previously asserted instructions.length === 2 and dropped the second authorize entirely. + txJson.instructionsData.length.should.equal(3); + (txJson.instructionsData[1].params as { authorizeType: string }).authorizeType.should.equal('Staker'); + (txJson.instructionsData[2].params as { authorizeType: string }).authorizeType.should.equal('Withdrawer'); + }); + }); + describe('StakingAuthorize explainTransaction (non-WASM path)', () => { // The 'sol' coin (mainnet) uses the legacy non-WASM explainTransaction path, // unlike 'tsol' which routes through the WASM explainer. This ensures the new diff --git a/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingRawMsgAuthorizeBuilder.ts b/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingRawMsgAuthorizeBuilder.ts index b4409a4a47..252a835bde 100644 --- a/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingRawMsgAuthorizeBuilder.ts +++ b/modules/sdk-coin-sol/test/unit/transactionBuilder/stakingRawMsgAuthorizeBuilder.ts @@ -219,6 +219,8 @@ describe('Sol Staking Raw Message Authorize Builder', () => { oldAuthorizeAddress: '6xgesG4vajCYfAQpknodrarD49ZCnXGvYA4H1DLuGV7Y', newAuthorizeAddress: '4p1VdN6BngTAbWR7Q5JPpbB6dc4k4y8wn1knmmWEjc9i', custodianAddress: 'DHCVjKy7kN6D6vM69nHcEeEeS685qtonFbiFNBW5bGiq', + // Decoded from the instruction data (0a00000001000000): AuthorizeChecked, StakeAuthorize 1. + authorizeType: 'Withdrawer', }); }); From 36a498df61b27be56501239ab01de0c27d2c534c Mon Sep 17 00:00:00 2001 From: maheshbitgo Date: Mon, 17 Aug 2026 13:43:09 +0530 Subject: [PATCH 3/3] fix(sdk-coin-sol): verify every authorize instruction against the intent Validating a single summarised authority change left the rest of the transaction unconstrained. Solana executes every instruction, so a crafted txHex could satisfy the check with one instruction while a second did something the user never asked for: - a Withdrawer change on a different stake account the same wallet controls, smuggled alongside a correct change on the intended account - a Staker change to an attacker key, paired with a correct Withdrawer change; its holder can delegate, deactivate and split the stake Verification now walks every Authorize instruction and requires each to target the intended stakeAccount, to be signed away by the wallet root, and to move the authority to the intended newWithdrawPublicKey. The staker authority is held to the same key because the intent carries one new authority and the builder points both authorities at it. At least one Withdrawer change is required, since an authorize intent always transfers the withdraw authority. The instruction list comes from toJson rather than the explanation: summarising many instructions into one cannot describe a transaction that touches several stake accounts, so the summary is display-only and is documented as such. Also aligns the raw AuthorizeChecked parser with the explain path by requiring the four mandatory accounts rather than five, since Solana makes the lockup custodian optional. Both attack cases are covered by tests on the legacy and WASM explain paths, and both fail against the previous commit. TICKET: CHALO-1294 Co-authored-by: Cursor --- .../src/lib/explainTransactionWasm.ts | 4 +- modules/sdk-coin-sol/src/lib/iface.ts | 5 +- .../src/lib/instructionParamsFactory.ts | 7 +- .../src/lib/stakingAuthorizeSummary.ts | 29 ++-- modules/sdk-coin-sol/src/lib/transaction.ts | 3 +- modules/sdk-coin-sol/src/sol.ts | 142 ++++++++++++------ modules/sdk-coin-sol/test/unit/sol.ts | 83 ++++++++-- .../test/unit/stakingAuthorizeSummary.ts | 3 +- .../src/bitgo/utils/tss/recipientUtils.ts | 2 +- 9 files changed, 194 insertions(+), 84 deletions(-) diff --git a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts index 632409f5b1..19e98e119a 100644 --- a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts +++ b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts @@ -240,8 +240,8 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So } // --- Staking authorize --- - // Summarised by the same rules as the legacy and raw parse paths, so all three agree on which - // instruction defines the withdraw authority that verifyTransaction validates. + // Display summary only — same rules as the legacy and raw explain paths. Signature validation + // walks every Authorize instruction in Sol.verifyStakingAuthorizeInstructions instead. const stakingAuthorize = summarizeStakingAuthorize( parsed.instructionsData .filter( diff --git a/modules/sdk-coin-sol/src/lib/iface.ts b/modules/sdk-coin-sol/src/lib/iface.ts index 5012083fe8..51652a3b1e 100644 --- a/modules/sdk-coin-sol/src/lib/iface.ts +++ b/modules/sdk-coin-sol/src/lib/iface.ts @@ -199,8 +199,9 @@ export interface StakingAuthorize { /** * The lockup custodian account, NOT a withdraw authority. Named `newWithdrawAddress` for * historical reasons: {@link stakingAuthorizeInstruction} passes it as `custodianPubkey`, and - * the decoder reads that account back into this field. Its presence is orthogonal to whether - * the instruction changes the staker or the withdrawer — use `authorizeType` for that. + * the decoder reads that account back into this field. Solana documents the custodian as + * optional for Withdrawer changes under lockup; the encoding can still attach or omit it + * independently of `authorizeType` — use `authorizeType` for the authority being changed. */ newWithdrawAddress?: string; /** The lockup custodian account, as decoded by the raw (AuthorizeChecked) parser. */ diff --git a/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts b/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts index 8ab9f173e6..a9fa491472 100644 --- a/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts +++ b/modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts @@ -1276,15 +1276,16 @@ function parseStakingAuthorizeRawInstructions(instructions: TransactionInstructi instructionData.push(nonce); for (const authorize of instructions.slice(1)) { // AuthorizeChecked accounts: [0] stake, [1] clock sysvar, [2] current authority, - // [3] new authority, [4] optional lockup custodian. - assert(authorize.keys.length === 5, 'Invalid number of keys in authorize instruction'); + // [3] new authority, [4] lockup custodian. The custodian is optional in Solana, so require + // only the four mandatory accounts — matching the explain path, which accepts the same shape. + assert(authorize.keys.length >= 4, 'Invalid number of keys in authorize instruction'); instructionData.push({ type: InstructionBuilderTypes.StakingAuthorize, params: { stakingAddress: authorize.keys[0].pubkey.toString(), oldAuthorizeAddress: authorize.keys[2].pubkey.toString(), newAuthorizeAddress: authorize.keys[3].pubkey.toString(), - custodianAddress: authorize.keys[4].pubkey.toString(), + custodianAddress: authorize.keys[4]?.pubkey.toString(), authorizeType: decodeRawAuthorizeType(authorize), }, }); diff --git a/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts b/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts index fe43f07f84..c7113346eb 100644 --- a/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts +++ b/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts @@ -17,23 +17,28 @@ export interface AuthorizeInstructionView { } /** - * Reduce the Authorize instructions of a transaction to the single summary that - * `verifyTransaction` validates against the signing intent. + * Reduce the Authorize instructions of a transaction to a single human-readable summary for + * `explainTransaction`. + * + * This is a **display** summary, not a security boundary. Because it collapses many instructions + * into one it cannot describe a transaction that re-authorises several stake accounts, so nothing + * may authorise a signature on the strength of it. `Sol.verifyStakingAuthorizeInstructions` + * validates every instruction individually against the intent instead. * * Selection rules, and why: * * - The authority being changed is taken **only** from `authorizeType`, never inferred from the - * presence of a custodian. A lockup custodian is orthogonal to `StakeAuthorize` — Solana allows - * a Staker change to carry one and a Withdrawer change to omit one — so inferring the type from - * it lets a crafted transaction pair a real Withdrawer change to an attacker key with a decoy - * Staker change to the expected key, and have the decoy reported as the withdraw authority. - * - A Withdrawer change always outranks a Staker change, because `newWithdrawAddress` is the - * security-critical field and must not be masked by a staker-only instruction. + * presence of a custodian. Solana documents the lockup custodian as an optional account for + * Withdrawer changes under lockup, and the instruction encoding can still attach one on a + * Staker change (or omit it on a Withdrawer change), so custodian presence is not a type + * discriminator — inferring from it would let a decoy Staker change be reported as the + * withdraw authority. + * - A Withdrawer change outranks a Staker change, so the withdraw authority is never masked by a + * staker-only instruction. * - Among instructions of the same authority type the **last** one wins, matching Solana's - * sequential execution: the final on-chain authority is the one set by the last instruction. - * - Withdraw fields are left empty for a staker-only transaction so a staker address is never - * compared against an intended withdraw key. `verifyTransaction` rejects the empty value when - * the intent expects a withdraw-authority change. + * sequential execution for a single stake account. + * - Withdraw fields are left empty for a staker-only transaction, so a staker address is never + * presented as a withdraw address. * * @param instructions decoded Authorize instructions, in transaction order * @returns the summary, or undefined when the transaction has no Authorize instruction diff --git a/modules/sdk-coin-sol/src/lib/transaction.ts b/modules/sdk-coin-sol/src/lib/transaction.ts index a878960527..2c944169b5 100644 --- a/modules/sdk-coin-sol/src/lib/transaction.ts +++ b/modules/sdk-coin-sol/src/lib/transaction.ts @@ -717,8 +717,7 @@ export class Transaction extends BaseTransaction { }; // validateRawMsgInstruction accepts a nonce advance followed by one or two AuthorizeChecked // instructions, so summarise all of them rather than only instructions[1]. Reading just the - // first one reported empty withdraw fields whenever the Staker change came first, which made - // verifyTransaction reject a legitimate two-instruction authorize. + // first one reported empty withdraw fields whenever the Staker change came first. const stakingAuthorizeParams = summarizeStakingAuthorize( instructions.slice(1).map((instruction) => { // AuthorizeChecked requires the stake account, clock sysvar, current authority and new diff --git a/modules/sdk-coin-sol/src/sol.ts b/modules/sdk-coin-sol/src/sol.ts index 7e09128659..d05995d9c3 100644 --- a/modules/sdk-coin-sol/src/sol.ts +++ b/modules/sdk-coin-sol/src/sol.ts @@ -70,7 +70,12 @@ import { TransactionBuilderFactory, explainSolTransaction, } from './lib'; -import { AtaClose, AtaRecoverNested, TransactionExplanation as SolLibTransactionExplanation } from './lib/iface'; +import { + AtaClose, + AtaRecoverNested, + StakingAuthorize, + TransactionExplanation as SolLibTransactionExplanation, +} from './lib/iface'; import { InstructionBuilderTypes } from './lib/constants'; import { getAssociatedTokenAccountAddress, @@ -519,6 +524,93 @@ export class Sol extends BaseCoin { return true; } + /** + * Validate every Authorize instruction in a staking authorize transaction against the intent. + * + * This exists to stop a compromised server substituting a txHex that rotates a stake account's + * authorities to keys the user never asked for, so it fails closed throughout: a missing intent + * field aborts the signature rather than skipping a comparison. `SolAuthorizeIntent` declares + * `stakeAccount` and `newWithdrawPublicKey` as required, so their absence means the intent did + * not reach us intact and must not be signed against. + * + * Every instruction is checked, not just a representative one. Solana executes all of them, so + * summarising the transaction down to a single authority change would leave the rest + * unconstrained — a second instruction could re-authorise a different stake account, or hand the + * staker authority to an attacker, while the summarised one still matched the intent. + */ + private verifyStakingAuthorizeInstructions( + transaction: Transaction, + txParams: TransactionParams, + walletRootAddress: string | undefined + ): void { + if (!txParams.newWithdrawPublicKey) { + throw new Error('StakingAuthorize intent is missing newWithdrawPublicKey, cannot verify withdraw authority'); + } + if (!txParams.stakeAccount) { + throw new Error('StakingAuthorize intent is missing stakeAccount, cannot verify the stake account'); + } + if (!walletRootAddress) { + throw new Error('StakingAuthorize verification requires the wallet root address'); + } + + const authorizeInstructions = transaction + .toJson() + .instructionsData.filter((instruction) => instruction.type === InstructionBuilderTypes.StakingAuthorize) + .map((instruction) => (instruction as StakingAuthorize).params); + + if (authorizeInstructions.length === 0) { + throw new Error('StakingAuthorize transaction contains no authorize instructions'); + } + + let withdrawerChanges = 0; + for (const params of authorizeInstructions) { + // Confines the transaction to the one stake account the intent names, so a second + // instruction cannot re-authorise an unrelated account the wallet also controls. + if (params.stakingAddress !== txParams.stakeAccount) { + throw new Error( + 'StakingAuthorize stakingAddress does not match intended stakeAccount: expected ' + + txParams.stakeAccount + + ' but got ' + + params.stakingAddress + ); + } + // The wallet must be the authority it is signing away. + if (params.oldAuthorizeAddress !== walletRootAddress) { + throw new Error( + 'StakingAuthorize oldAuthorizeAddress does not match wallet root address: expected ' + + walletRootAddress + + ' but got ' + + params.oldAuthorizeAddress + ); + } + // Applied to staker changes as well as withdrawer changes: the intent carries a single new + // authority key, and the builder points both authorities at it, so a staker change to any + // other key is not something the user asked for. + if (params.newAuthorizeAddress !== txParams.newWithdrawPublicKey) { + throw new Error( + 'StakingAuthorize newAuthorizeAddress does not match intended newWithdrawPublicKey: expected ' + + txParams.newWithdrawPublicKey + + ' but got ' + + params.newAuthorizeAddress + + ' (' + + (params.authorizeType ?? 'unknown') + + ' authority)' + ); + } + if (params.authorizeType === 'Withdrawer') { + withdrawerChanges++; + } + } + + // An authorize intent always transfers the withdraw authority, so a transaction that only + // touches the staker authority does not fulfil it. + if (withdrawerChanges === 0) { + throw new Error( + 'StakingAuthorize transaction does not transfer the withdraw authority: ' + ); + } + } + async verifyTransaction(params: SolVerifyTransactionOptions): Promise { // asset name to transfer amount map const totalAmount: Record = {}; @@ -576,53 +668,7 @@ export class Sol extends BaseCoin { transaction.type === TransactionType.StakingAuthorizeRaw || txParams.type === 'authorize'; if (isStakingAuthorizeTx) { - // This check exists to stop a compromised server from substituting a txHex that rotates the - // withdraw authority to an attacker key, so every branch below fails closed: a missing - // intent field or a missing explanation aborts the signature rather than skipping the - // comparison. SolAuthorizeIntent declares stakeAccount and newWithdrawPublicKey as required, - // so their absence means the intent did not survive intact and must not be signed against. - const authorizeParams = explainedTx.stakingAuthorize; - if (!authorizeParams) { - throw new Error('StakingAuthorize transaction is missing stakingAuthorize explanation fields'); - } - if (!txParams.newWithdrawPublicKey) { - throw new Error('StakingAuthorize intent is missing newWithdrawPublicKey, cannot verify withdraw authority'); - } - if (!txParams.stakeAccount) { - throw new Error('StakingAuthorize intent is missing stakeAccount, cannot verify the stake account'); - } - if (!walletRootAddress) { - throw new Error('StakingAuthorize verification requires the wallet root address'); - } - // Empty when the transaction changes only the staker authority. Comparing it against the - // intended withdraw key then fails here, which is correct: an authorize intent always - // transfers the withdraw authority, so a staker-only transaction does not fulfil it. - if (authorizeParams.newWithdrawAddress !== txParams.newWithdrawPublicKey) { - throw new Error( - 'StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey: expected ' + - txParams.newWithdrawPublicKey + - ' but got ' + - (authorizeParams.newWithdrawAddress || '') - ); - } - // The wallet must be the authority it is signing away, otherwise the transaction is - // re-authorising some other account's stake. - if (authorizeParams.oldWithdrawAddress !== walletRootAddress) { - throw new Error( - 'StakingAuthorize oldWithdrawAddress does not match wallet root address: expected ' + - walletRootAddress + - ' but got ' + - authorizeParams.oldWithdrawAddress - ); - } - if (authorizeParams.stakingAddress !== txParams.stakeAccount) { - throw new Error( - 'StakingAuthorize stakingAddress does not match intended stakeAccount: expected ' + - txParams.stakeAccount + - ' but got ' + - authorizeParams.stakingAddress - ); - } + this.verifyStakingAuthorizeInstructions(transaction, txParams, walletRootAddress); } const isTokenEnablementTx = txParams.type === 'enabletoken'; diff --git a/modules/sdk-coin-sol/test/unit/sol.ts b/modules/sdk-coin-sol/test/unit/sol.ts index de8660e7a7..59af771622 100644 --- a/modules/sdk-coin-sol/test/unit/sol.ts +++ b/modules/sdk-coin-sol/test/unit/sol.ts @@ -1008,11 +1008,11 @@ describe('SOL:', function () { const differentKey = new KeyPair(resources.splitStakeAccount).getKeys(); /** - * Verify a hand-crafted transaction against an authorize intent naming `newWithdrawKey`, - * on both explain implementations: 'tsol' routes through the WASM parser and 'sol' through - * the legacy web3.js parser. A crafted transaction must be rejected by both. + * Verify a hand-crafted transaction against an authorize intent naming `newWithdrawKey` + * on both `tsol` and `sol`. Verification reads Authorize instructions via `toJson()` / + * `instructionParamsFactory` on both chains (not the explain / WASM path). */ - const rejectOnBothExplainPaths = async (solTx: SolTransaction, matcher: RegExp) => { + const rejectOnBothChains = async (solTx: SolTransaction, matcher: RegExp) => { for (const chain of ['tsol', 'sol']) { const coin = bitgo.coin(chain) as Sol; coin.getChain().should.equal(chain); @@ -1109,7 +1109,7 @@ describe('SOL:', function () { txParams.stakeAccount = stakeAccount.pub; await basecoin .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) - .should.rejectedWith(/StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey/); + .should.rejectedWith(/StakingAuthorize newAuthorizeAddress does not match intended newWithdrawPublicKey/); }); it('should reject a staking authorize transaction where stakeAccount does not match', async function () { @@ -1128,7 +1128,7 @@ describe('SOL:', function () { .should.rejectedWith(/StakingAuthorize stakingAddress does not match intended stakeAccount/); }); - it('should reject a staking authorize transaction where oldWithdrawAddress does not match wallet root', async function () { + it('should reject a staking authorize transaction where oldAuthorizeAddress does not match wallet root', async function () { // Build tx where oldAuthorizedAddress is NOT wallet.pub const tx = await factory .getStakingAuthorizeBuilder() @@ -1150,7 +1150,7 @@ describe('SOL:', function () { txParams.stakeAccount = stakeAccount.pub; await basecoin .verifyTransaction({ txParams, txPrebuild, wallet: walletObj } as any) - .should.rejectedWith(/StakingAuthorize oldWithdrawAddress does not match wallet root address/); + .should.rejectedWith(/StakingAuthorize oldAuthorizeAddress does not match wallet root address/); }); it('should still enforce the fee payer check on a staking authorize transaction', async function () { @@ -1207,11 +1207,10 @@ describe('SOL:', function () { }) ); - // Asserted on both chains: 'tsol' explains via the WASM parser, 'sol' via the legacy - // web3.js parser, and each must reach the same verdict. - await rejectOnBothExplainPaths( + // Asserted on both chains via toJson() instruction parsing. + await rejectOnBothChains( solTx, - /StakingAuthorize newWithdrawAddress does not match intended newWithdrawPublicKey/ + /StakingAuthorize newAuthorizeAddress does not match intended newWithdrawPublicKey/ ); }); @@ -1229,7 +1228,67 @@ describe('SOL:', function () { }) ); - await rejectOnBothExplainPaths(solTx, //); + await rejectOnBothChains(solTx, //); + }); + + it('should reject a second Withdrawer change smuggled onto a different stake account', async function () { + // Solana executes every instruction, so validating only the instruction that matches the + // intent leaves the rest unconstrained. Here the intent names stakeAccount, and the + // matching instruction is entirely correct — but an earlier instruction re-authorises a + // different stake account the same wallet controls, to an attacker key. + const otherStakeAccount = new KeyPair(resources.nonceAccount).getKeys(); + const solTx = new SolTransaction(); + solTx.recentBlockhash = blockHash; + solTx.feePayer = new PublicKey(wallet.pub); + solTx.add( + StakeProgram.authorize({ + stakePubkey: new PublicKey(otherStakeAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + newAuthorizedPubkey: new PublicKey(differentKey.pub), + stakeAuthorizationType: StakeAuthorizationLayout.Withdrawer, + custodianPubkey: new PublicKey(differentKey.pub), + }) + ); + solTx.add( + StakeProgram.authorize({ + stakePubkey: new PublicKey(stakeAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + newAuthorizedPubkey: new PublicKey(newWithdrawKey.pub), + stakeAuthorizationType: StakeAuthorizationLayout.Withdrawer, + custodianPubkey: new PublicKey(newWithdrawKey.pub), + }) + ); + + await rejectOnBothChains(solTx, /StakingAuthorize stakingAddress does not match intended stakeAccount/); + }); + + it('should reject a Staker change to a key other than the intended new authority', async function () { + // The intent carries one new authority key and the builder points both authorities at it, + // so a staker change to some other key was never requested. Its holder can delegate, + // deactivate and split the stake even though the withdraw authority is correct. + const solTx = new SolTransaction(); + solTx.recentBlockhash = blockHash; + solTx.feePayer = new PublicKey(wallet.pub); + solTx.add( + StakeProgram.authorize({ + stakePubkey: new PublicKey(stakeAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + newAuthorizedPubkey: new PublicKey(differentKey.pub), + stakeAuthorizationType: StakeAuthorizationLayout.Staker, + custodianPubkey: new PublicKey(differentKey.pub), + }) + ); + solTx.add( + StakeProgram.authorize({ + stakePubkey: new PublicKey(stakeAccount.pub), + authorizedPubkey: new PublicKey(wallet.pub), + newAuthorizedPubkey: new PublicKey(newWithdrawKey.pub), + stakeAuthorizationType: StakeAuthorizationLayout.Withdrawer, + custodianPubkey: new PublicKey(newWithdrawKey.pub), + }) + ); + + await rejectOnBothChains(solTx, /Staker authority\)/); }); }); }); diff --git a/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts b/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts index 9098a28ea9..33411175ff 100644 --- a/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts +++ b/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts @@ -58,8 +58,7 @@ describe('summarizeStakingAuthorize', () => { summary!.newStakingAuthorityAddress!.should.equal('last'); }); - // An undecodable authority type must not be reported as a withdraw-authority change, otherwise - // verifyTransaction would compare an unknown address against the intended withdraw key. + // An undecodable authority type must not be reported as a withdraw-authority change. it('leaves withdraw fields empty when the authority type is unknown', () => { const summary = summarizeStakingAuthorize([view(undefined, 'unknown', 'custodian')]); summary!.newWithdrawAddress.should.equal(''); diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index 994b3a714c..c222c1c0ab 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -139,7 +139,7 @@ export function resolveEffectiveTxParams( } // Propagate SOL authorize-specific fields from the intent so sol.ts:verifyTransaction - // can validate the decoded instruction against what the user intended. + // can validate every decoded Authorize instruction against what the user intended. const intentNewWithdrawPublicKey = (txRequest.intent as PopulatedIntent)?.newWithdrawPublicKey; if (intentNewWithdrawPublicKey && !effectiveTxParams.newWithdrawPublicKey) { effectiveTxParams.newWithdrawPublicKey = intentNewWithdrawPublicKey;