fix(wasm-utxo): reject duplicate wallet role keys - #414
Draft
ralph-bitgo[bot] wants to merge 1 commit into
Draft
ralph-bitgo[bot] wants to merge 1 commit into
ralph-bitgo[bot] wants to merge 1 commit into
Conversation
Reject duplicate root xpubs and derived keys before wallet scripts or PSBT metadata creation. Propagate typed Taproot aggregation errors. Prevent one authority from occupying multiple quorum positions and trapping WASM when aggregation fails. Ticket: WCN-1863 Session-Id: d899491b-4b54-442d-bfa3-99fc93352b00 Task-Id: 05aabeb7-a6fa-45bb-adb0-5b6323263435 Requested-By: David Kaplan <davidkaplan@bitgo.com>
This branch has not been deployed
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
RootWalletKeys::new/new_with_derivation_prefixesare now fallible and reject repeated role xpubs with a typedWalletKeyError::DuplicateRootKeys, surfaced to JS/WASM callers as a branchablecode: "WalletKeyError.DuplicateRootKeys"error instead of aStringError.require_distinct_pubkeysand enforced it at the shared sinkWalletScripts::newand in every script family:build_multisig_script_2_of_3(builder and parser),ScriptP2tr::new,ScriptP2mr::new,build_tap_tree_for_output,create_tap_bip32_derivation_for_output. The second, derived-key check protects custom derivation prefixes and any future constructor that bypasses the root check.expectcalls:BitGoMusigError(e.g.InvalidPubkeyCountfor identical user/BitGo keys) is now a typedWasmUtxoError::BitGoMusigpropagated throughScriptP2tr::newand the PSBT/BIP-322 callsites, instead of a panic that traps the whole WASM instance.Why
A wallet threshold counts distinct authorization principals, but nothing in this library enforced that (AnchorWatch finding
csf_be4ee1b1031cad5e945939bf2, WCN-1863). With the same xpub in two roles, the legacy builders emitted a nominal 2-of-3OP_CHECKMULTISIGscript[A,A,B]; Bitcoin consensus accepts one valid signature copied into two stack positions, so a single key holder could spend unilaterally once a victim funded an address derived from the malicious triple. On Taproot chains the duplicate user/BitGo key reachedexpect("valid aggregation")and aborted the entire WASM instance rather than returning a typed error. This closes the quorum-collapse path fail-closed at the root-key boundary and again on derived keys immediately before script assembly.Test plan
cargo test+cargo clippy+cargo fmt(wasm-utxo)npm run build:test && npm run test:mocha(wasm-utxo), includingtest/fixedScript/walletKeys.tsWalletKeyError.DuplicateRootKeys/DuplicateDerivedKeys, not string errorsBitGoMusigError.InvalidPubkeyCountinstead of panickingVerification note: this sandbox has no
cargo/rustc, no Nix dev shell, and nonode_modules/generated WASM bindings, so the suites could not be executed locally before opening this draft. The branch CI is the verification gate — do not mark ready for review until the above checks are green.Ticket: WCN-1863