Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions modules/sdk-coin-sol/src/lib/explainTransactionWasm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

Expand Down
38 changes: 36 additions & 2 deletions modules/sdk-coin-sol/src/lib/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
Memo,
Nonce,
StakingActivate,
StakingAuthorize,
StakingAuthorizeParams,
StakingWithdraw,
TokenTransfer,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm whether the non-WASM instruction decoder guarantees newWithdrawAddress is empty/unset for Staker-only authorize instructions? If not, we may need an explicit authorizeType field on the parsed instruction (like the WASM path has) rather than inferring from field presence.

);
if (isWithdrawerAuthorize) {
stakingAuthorize = {
stakingAddress: authorizeInstruction.params.stakingAddress,
oldWithdrawAddress: authorizeInstruction.params.oldAuthorizeAddress,
newWithdrawAddress: authorizeInstruction.params.newAuthorizeAddress,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Your detection says 'this is a Withdrawer instruction because newWithdrawAddress is set,' but then you validate newAuthorizeAddress — what if they diverge? You'd be checking the wrong field and approving a malicious tx.

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;
Expand All @@ -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 {
Expand All @@ -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();

Expand Down Expand Up @@ -674,6 +707,7 @@ export class Transaction extends BaseTransaction {
blockhash: this.getNonce(),
durableNonce: durableNonce,
tokenEnablements: tokenEnablements,
...(stakingAuthorize && { stakingAuthorize }),
};

return explanation;
Expand Down
42 changes: 42 additions & 0 deletions modules/sdk-coin-sol/src/sol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you trace where SolAuthorizeIntent is constructed and confirm whether newWithdrawPublicKey and stakeAccount are guaranteed present?

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.
Expand Down
137 changes: 137 additions & 0 deletions modules/sdk-coin-sol/test/unit/sol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
65 changes: 65 additions & 0 deletions modules/sdk-coin-sol/test/unit/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
4 changes: 4 additions & 0 deletions modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
Loading
Loading