bitcoindevkit / bitcoindevkit/bdk-tx

Finalizer doesn't enforce BIP-174 requirements (sighash type, unknown fields)

Open
#75 1 comment 0 reactions 1 assignee Claimed by @evanlinjin View on GitHub
bug
Dominant language
Rust
Stars
13
Forks
17
Avg merge
17h 22m
Merged PRs (30d)
1

Description

The local [`bdk_tx::Finalizer::finalize_input`](https://github.com/bitcoindevkit/bdk-tx/blob/master/src/finalizer.rs) misses two BIP-174 finalizer requirements.

## 1. `PSBT_IN_SIGHASH_TYPE` enforcement (MUST)

BIP-174 states (under `PSBT_IN_SIGHASH_TYPE`):

> Signatures for this input must use the sighash type, finalizers **must** fail to finalize inputs which have signatures that do not match the specified sighash type.

The current implementation calls `plan.satisfy(&PsbtInputSatisfier)` to build the witness from whatever signatures are present in the PSBT input, with no cross-check against `psbt_input.sighash_type`. If a buggy or non-conforming signer produces signatures whose sighash byte doesn't match the declared `PSBT_IN_SIGHASH_TYPE`, finalization still succeeds and the resulting transaction broadcasts.

This is the load-bearing channel for `PSBT_IN_SIGHASH_TYPE` being a useful defense — without finalizer enforcement, the only protection comes from the signer voluntarily honoring the declared value. The spec requires the check; we don't do it.

**Proposed fix:** before `plan.satisfy()`, if `psbt_input.sighash_type` is set, iterate the input's signature fields and compare each signature's effective sighash type against the declared value:

- **ECDSA** (`partial_sigs`): each `bitcoin::ecdsa::Signature` carries an explicit `sighash_type` field — compare directly to the declared `PsbtSighashType` (as `EcdsaSighashType`).
- **Taproot key-path** (`tap_key_sig`): a 64-byte signature implies `SIGHASH_DEFAULT`; a 65-byte signature has the sighash byte as its trailing byte. Compare to the declared `PsbtSighashType` (as `TapSighashType`).
- **Taproot script-path** (`tap_script_sigs`): same encoding rule per signature; same comparison.

Return an error on any mismatch.

## 2. Unknown fields not preserved (SHOULD/MUST)

BIP-174 states (under the Input Finalizer section):

> All other data except the UTXO and **unknown fields** in the input key-value map should be cleared from the PSBT.

The current implementation uses `core::mem::take` to clear the entire `psbt::Input`, then writes back only `non_witness_utxo` and `witness_utxo`. The `Input::unknown` field (and likely `Input::proprietary` by extension) is dropped, contrary to the BIP. Callers who attach metadata expecting it to survive finalization currently lose it.

**Proposed fix:** preserve `input.unknown` (and likely `input.proprietary`) when reconstructing the finalized input.

## Impact

(1) is the more critical of the two — it's the only channel that makes a declared `PSBT_IN_SIGHASH_TYPE` actually binding. Without it, callers relying on declared sighash for defense-in-depth (e.g. against signers that may pick non-`ALL` defaults when given license to do so via an unset field) get no protection from this Finalizer.

(2) is a hygiene/conformance issue but matters for callers using `Input::unknown` or `Input::proprietary` to thread metadata through their PSBT pipeline.

## Related

- Discussed in #74 review (PsbtParams::declare_sighash).
- Upstream rust-miniscript's [`psbt::finalizer`](https://docs.rs/miniscript/latest/miniscript/psbt/index.html) has the same `sighash_type` enforcement gap. A separate upstream issue should be filed against rust-bitcoin/rust-miniscript so callers using that finalizer (instead of bdk-tx's) get the same defense.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.