diff --git a/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts b/modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts index 90e2733291..19e98e119a 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,13 +240,21 @@ export function explainSolTransaction(params: ExplainTransactionWasmOptions): So } // --- Staking authorize --- - let stakingAuthorize: StakingAuthorizeParams | undefined; - for (const instr of parsed.instructionsData) { - if (instr.type === 'StakingAuthorize') { - stakingAuthorize = mapStakingAuthorize(instr); - break; - } - } + // 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( + (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..51652a3b1e 100644 --- a/modules/sdk-coin-sol/src/lib/iface.ts +++ b/modules/sdk-coin-sol/src/lib/iface.ts @@ -181,14 +181,38 @@ 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. 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. */ 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..a9fa491472 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,22 @@ 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] 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(), + 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..c7113346eb --- /dev/null +++ b/modules/sdk-coin-sol/src/lib/stakingAuthorizeSummary.ts @@ -0,0 +1,79 @@ +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 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. 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 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 + */ +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 e91bfcb1d3..2c944169b5 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, @@ -33,6 +34,7 @@ import { Memo, Nonce, StakingActivate, + StakingAuthorize, StakingAuthorizeParams, StakingWithdraw, TokenTransfer, @@ -42,6 +44,7 @@ import { VersionedTransactionData, WalletInit, } from './iface'; +import { AuthorizeInstructionView, summarizeStakingAuthorize } from './stakingAuthorizeSummary'; import { instructionParamsFactory } from './instructionParamsFactory'; import { getInstructionType, @@ -540,6 +543,7 @@ export class Transaction extends BaseTransaction { const outputs: TransactionRecipient[] = []; // Create a separate array for token enablements const tokenEnablements: ITokenEnablement[] = []; + const authorizeInstructions: AuthorizeInstructionView[] = []; for (const instruction of decodedInstructions) { switch (instruction.type) { @@ -598,6 +602,22 @@ export class Transaction extends BaseTransaction { tokenAddress: ataInit.params.mintAddress, }); break; + case InstructionBuilderTypes.StakingAuthorize: { + 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: // Custom instructions are arbitrary and cannot be explained break; @@ -617,7 +637,14 @@ export class Transaction extends BaseTransaction { } } - return this.getExplainedTransaction(outputAmount, outputs, memo, durableNonce, tokenEnablements); + return this.getExplainedTransaction( + outputAmount, + outputs, + memo, + durableNonce, + tokenEnablements, + summarizeStakingAuthorize(authorizeInstructions) + ); } private calculateFee(): string { @@ -638,7 +665,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 +702,7 @@ export class Transaction extends BaseTransaction { blockhash: this.getNonce(), durableNonce: durableNonce, tokenEnablements: tokenEnablements, + ...(stakingAuthorize && { stakingAuthorize }), }; return explanation; @@ -686,22 +715,27 @@ 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. + 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 c426ebe05c..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 = {}; @@ -571,6 +663,14 @@ export class Sol extends BaseCoin { } } + const isStakingAuthorizeTx = + transaction.type === TransactionType.StakingAuthorize || + transaction.type === TransactionType.StakingAuthorizeRaw || + txParams.type === 'authorize'; + if (isStakingAuthorizeTx) { + this.verifyStakingAuthorizeInstructions(transaction, txParams, walletRootAddress); + } + 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..59af771622 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 { @@ -1000,6 +1001,296 @@ 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(); + + /** + * 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 rejectOnBothChains = 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() + .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); + }); + + // 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(); + txPrebuild.txBase64 = txBase64; + txPrebuild.txInfo.nonce = blockHash; + txParams.recipients = []; + txParams.type = 'authorize'; + 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 () { + 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 newAuthorizeAddress 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 oldAuthorizeAddress 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 oldAuthorizeAddress 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'); + }); + + 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 via toJson() instruction parsing. + await rejectOnBothChains( + solTx, + /StakingAuthorize newAuthorizeAddress 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 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\)/); + }); + }); }); describe('getAmountBasedOnEndianness', () => { 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..33411175ff --- /dev/null +++ b/modules/sdk-coin-sol/test/unit/stakingAuthorizeSummary.ts @@ -0,0 +1,68 @@ +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. + 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 a5f72c69d0..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', () => { @@ -1122,4 +1129,157 @@ 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 + // 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-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', }); }); 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..c222c1c0ab 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 every decoded Authorize 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'); + }); }); }); });