Uh oh!
There was an error while loading. Please reload this page.
feat(sdk-coin-sol): add verifyTransaction validation for staking authorize intent - #9463
Open
bitgo-ai-agent-dev[bot] wants to merge 3 commits into
Open
feat(sdk-coin-sol): add verifyTransaction validation for staking authorize intent#9463bitgo-ai-agent-dev[bot] wants to merge 3 commits into
bitgo-ai-agent-dev[bot] wants to merge 3 commits into
Conversation
Contributor
ralph-bitgoBotforce-pushed
the
chalo-1294-sol-authorize-verify-transaction
branch
from
August 11, 2026 04:27
61816b4 to
b9f9be7Comparebitgo-ai-agent-devBotforce-pushed
the
chalo-1294-sol-authorize-verify-transaction
branch
from
August 11, 2026 04:27
b9f9be7 to
c0322c5Comparemaheshbitgo
marked this pull request as ready for review
August 11, 2026 08:18
…orize 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
maheshbitgoforce-pushed
the
chalo-1294-sol-authorize-verify-transaction
branch
from
August 11, 2026 08:56
c0322c5 to
f3d445eCompare
davidkaplanbitgo
left a comment
Contributor
There was a problem hiding this comment.
BTC related changes look fine
| // 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 |
Contributor
There was a problem hiding this comment.
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.
| authorizeParams.oldWithdrawAddress | ||
| ); | ||
| } | ||
| if (txParams.newWithdrawPublicKey && authorizeParams.newWithdrawAddress !== txParams.newWithdrawPublicKey) { |
Contributor
There was a problem hiding this comment.
Can you trace where SolAuthorizeIntent is constructed and confirm whether newWithdrawPublicKey and stakeAccount are guaranteed present?
| stakingAuthorize = { | ||
| stakingAddress: authorizeInstruction.params.stakingAddress, | ||
| oldWithdrawAddress: authorizeInstruction.params.oldAuthorizeAddress, | ||
| newWithdrawAddress: authorizeInstruction.params.newAuthorizeAddress, |
Contributor
There was a problem hiding this comment.
"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.
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
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 <cursoragent@cursor.com>
maheshbitgo
commented
Aug 17, 2026
Contributor
@claude Review the changes for security, correctness and adversarial effects/regression. |
Contributor
Claude finished @maheshbitgo's task —— View job Security and Correctness ReviewReviewing the SOL staking authorize intent validation changes for security vulnerabilities, correctness issues, and potential adversarial bypass scenarios. Todo List
Core Validation AnalysisThe ✅ Strengths:
Critical Security Issue: Field Naming Confusion |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
isStakingAuthorizeTxbranch insol.ts:verifyTransactionthat validates the decoded SOL staking authorize instruction against the caller's intent fields:oldWithdrawAddressmatches the wallet root address (Withdrawer authority only; skipped for staker-only txs where the field is'')newWithdrawAddressmatchestxParams.newWithdrawPublicKeyfrom the intent (when set)stakingAddressmatchestxParams.stakeAccountfrom the intent (when set)resolveEffectiveTxParamsinsdk-coreto propagatenewWithdrawPublicKeyandstakeAccountfromtxRequest.intentintotxParamsso the fields reachverifyTransactionthrough the TSS signing flownewWithdrawPublicKeyandstakeAccounttoPopulatedIntent(baseTypes.ts) andTransactionParams(iBaseCoin.ts)explainTransactionfor both the WASM (tsol) and non-WASM (sol) paths to populatestakingAuthorizewith the Withdrawer instruction's fields (the security-critical one), not the Staker instruction's, when a standard two-instruction authorize tx is presentnewWithdrawPublicKeysubstitution, wrongstakeAccount, wrongoldWithdrawAddress, non-WASM explainTransaction path, andrecipientUtilspropagationWhy
'authorize'was added toNO_RECIPIENT_TX_TYPES(WCI-1111 / PR fix(sdk-core): wire resolveEffectiveTxParams into EddsaMPCv2Utils (WCI-1111) #9394) becauseSolAuthorizeIntenthas emptyrecipientsby design. Without coin-layer validation, a compromised BitGo server could present aStakingAuthorizetxHex that rotates the stake account's withdraw authority to an attacker-controlled key, and the client would sign it with no checks.Test plan
yarn unit-test --scope @bitgo/sdk-coin-sol— 646 tests passyarn unit-test --scope @bitgo/sdk-core(recipientUtils tests) — 32 tests passsdk-coin-solandsdk-corenewWithdrawPublicKeyandstakeAccountpassesnewWithdrawPublicKeycorrectly throwsstakeAccountcorrectly throwsoldWithdrawAddress(wallet root mismatch) correctly throwsoldWithdrawAddress) does not block legitimate staker-authority changesTicket: CHALO-1294