Set is_change only after each branch's own checks
What the developer wrote
Authored by kdmukai
Set is_change only after each branch's own checks
In _parse_outputs the flag was set True as soon as the rebuilt
scriptPubKey matched, and two multisig branches then turned it back
off. That made "change" the default inside the matched arm, so a branch
that forgot the flag would record change with nothing verified behind
it. Now each branch sets the flag only once its own checks have passed,
and an output that satisfies none of them stays an external spend.
The two sites where the flag is set split by signature type, and each
carries the certainty it actually has: single sig is settled, because
our own key rebuilt the scriptPubKey; multisig is a presumption, because
our key in the committed script does not say which wallet that script
belongs to, and only the user's descriptor can. The reminder that
"change" means any output coming back to our seed moves to the line
where change_data is built, which is where the word takes that meaning.
No behavior change: the same outputs land in the same lists on every
fixture shape, and the suite is unchanged.
What changed, and why it matters
This commit fixes a logic bug in how SeedSigner decides whether a Bitcoin transaction output is 'change' coming back to the user's wallet. Previously, the code marked an output as change as soon as it looked like it paid to the user's seed, before fully checking the details. Two special cases (multisig wallets) then had to manually undo that mark. The danger was that any future code path that forgot to undo the mark could wrongly label an external payment as the user's own change, potentially tricking a user into approving a transaction that sends funds to an attacker. The patch moves the 'is_change = True' assignment so it only happens after each branch's specific safety checks have passed. The commit itself says there is no behavior change on current test fixtures, but it removes a risky default.
Treat this as a security-hardening fix with latent bug-fix value. Review whether any prior release shipped with the default-true behavior and assess if a user could construct a PSBT that reaches the matched arm but fails the multisig checks in a way that left is_change True before this patch. If such a path existed, consider a security advisory and patch release. Regardless, the change should be included in the next release because it removes a footgun for future code changes.
Security signals we found
Logic bug: default-true flag inside a conditional arm with branches responsible for undoing it
Potential UI deception: unverified output could be displayed as user change
Defensive refactor: flag set only after branch-specific verification
Explicit comment noting multisig change is a presumption pending descriptor verification
No new tests or fixtures added; commit claims no behavior change on existing fixtures
Evidence from the diff
In src/seedsigner/models/psbt_parser.py’s _parse_outputs(), the flag is_change was previously set to True immediately when the rebuilt scriptPubKey matched the output’s scriptPubKey. The singlesig and multisig branches then either confirmed or, in two multisig error paths, reset it to False. This created a default-true state inside the matched arm, meaning a missing or incomplete branch could leave is_change True without verification. The patch removes the early assignment and instead sets is_change = True only after the singlesig branch has verified the derivation path, and after the multisig branch has confirmed the seed’s key is in the committed script. The two multisig failure cases now simply fall through (pass) rather than explicitly setting is_change = False, because the default is now False. Comments are updated to clarify that ‘change’ here means any output returning to the seed, and that true multisig change verification still depends on the user’s known-good descriptor.
Changed components
src/seedsigner/models/psbt_parser.py_parse_outputs() methodSingle-signature output verification branchMultisig output verification branchchange_data constructionInspect captured patch +23 / −7
### src/seedsigner/models/psbt_parser.py
@@ -478,9 +478,6 @@ def _parse_outputs(self, child_key_derivation_cache: dict):
# The scriptPubKey we created using our own seed matched what this
# output is actually committing to.
- # Remember that "change" is ANY output coming back to our seed
- is_change = True
-
if singlesig_derivation_path is not None:
if verified_derivation_path is None:
# The output pays this seed but the psbt claimed a different
@@ -494,6 +491,11 @@ def _parse_outputs(self, child_key_derivation_cache: dict):
# only have verified this same path.
raise RuntimeError(f"Output {i} verified at a path it does not pay")
+ # We've now verified that the key we derived from our seed at the
+ # claimed path is the key this output pays. This output is
+ # provably ours.
+ is_change = True
+
elif multisig_script is not None:
if verified_derivation_path is None:
# No entry claimed this seed's fingerprint, but we already
@@ -512,10 +514,11 @@ def _parse_outputs(self, child_key_derivation_cache: dict):
# We treat this deception as an attack.
raise PSBTOutputOwnershipContradictionError(f"Output's committed script holds this seed's key at {bip32.path_to_str(derivation_path_obj.derivation)} but the psbt claims another fingerprint and/or public key there")
- # Every path the psbt supplied has been checked and none puts
- # this seed in the committed script, so the output is an
- # external spend.
- is_change = False
+ # We have derived a key from our seed for every derivation
+ # path this output supplies, but none of our keys match any
+ # of the keys in this output's script. So we consider this
+ # output an external spend.
+ pass
else:
# This output claimed that our seed is part of the receiving
@@ -536,6 +539,15 @@ def _parse_outputs(self, child_key_derivation_cache: dict):
# mistake. We just abort the parse.
raise PSBTSurplusDerivationPathsError("Multisig output claims more derivation paths than its script has keys")
+ # We now know that our key is in the committed script; this
+ # output does pay to a multisig that our seed is part of. But
+ # note that we do not know yet if this is truly change coming
+ # back to our wallet or if it is paying out to a different
+ # multisig that happens to include our seed. Final change
+ # verification can only happen if and when the user loads
+ # their "known-good" multisig descriptor.
+ is_change = True
+
else:
# No handler claimed a matching output, which the branches above
# should make impossible. Raise rather than leave is_change True;
@@ -569,6 +581,10 @@ def _parse_outputs(self, child_key_derivation_cache: dict):
self.op_return_data = vout[i].script_pubkey.data[3:]
elif is_change:
+ # Remember that "change" in this function is ANY output coming back to our
+ # seed, receive addresses included. It is up to the View layer to use the
+ # derivation path to determine if it should be displayed as change or
+ # receive.
addr = vout[i].script_pubkey.address(NETWORKS[SettingsConstants.map_network_to_embit(self.network)])
self.change_data.append({
"output_index": i,Why this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.