fix(onchain-zaps): address audit findings — fund-safety, interop, robustness

CRITICAL - PsbtSignatureVerifier: independently verifies every key-path signature in a PSBT (BIP-340 sig over the BIP-341 sighash, against the tweaked output key). - OnchainZapSender now rejects a signer that returns a different transaction than it was asked to sign (byte-compares the unsigned tx) and verifies all signatures before broadcasting — closes a substitution attack where a malicious/buggy external signer could redirect funds. HIGH — break test circularity - Pin the full BIP-341 keyPathSpending input-0 witness: signing the vector sighash with the vector tweaked key reproduces the vector signature byte-for-byte (also pins BIP-340 nonce determinism). - Pin TaprootAddress.fromPubKey + SegwitAddress.encodeP2TR against all seven BIP-341 wallet-test-vector P2TR mainnet addresses. - EsploraBackendTest: JSON-parsing coverage for /tx, /address/{}/utxo, the mempool.space and blockstream fee formats, and schema-fallback. MEDIUM - OnchainZapBuilder filters to confirmed UTXOs by default (allowUnconfirmed opt-in) and signals BIP-125 RBF (nSequence 0xFFFFFFFD). - CachingOnchainBackend: TTL-caching decorator (confirmed tx forever, unconfirmed/tip/fees short TTL) so a feed of onchain zaps doesn't fan out into one HTTP request per event. Wired in AppModules. - OnchainZapVerifier computes real confirmation depth from the chain tip and asserts the backend echoed the requested txid. - EsploraBackend falls back to the standard Esplora /fee-estimates endpoint (blockstream.info) when /v1/fees/recommended 404s. LOW - BitcoinTransaction.parse caps input/output/witness-item counts to stop a hostile varint from triggering a giant pre-allocation. - OnchainZapEventTest: asserts kind:8333 on-the-wire tag structure against the NIP-BC spec. - alt tag now includes the amount ("Onchain zap: N sats"), matching the spec example. All quartz / commons / androidHostTest suites pass; amethyst compiles.
2026-05-14 13:29:48 +00:00
parent 5f16ebc060
commit 1d7be1d444
16 changed files with 844 additions and 39 deletions
@@ -29,6 +29,7 @@ import com.vitorpamplona.quartz.nipBCOnchainZaps.build.OnchainZapBuilder
 import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.OnchainBackend
 import com.vitorpamplona.quartz.nipBCOnchainZaps.psbt.Psbt
 import com.vitorpamplona.quartz.nipBCOnchainZaps.psbt.PsbtFinalizer
+import com.vitorpamplona.quartz.nipBCOnchainZaps.psbt.PsbtSignatureVerifier
 import com.vitorpamplona.quartz.nipBCOnchainZaps.taproot.TaprootAddress
 import com.vitorpamplona.quartz.nipBCOnchainZaps.zap.OnchainZapEvent
 import kotlin.coroutines.cancellation.CancellationException
@@ -142,17 +143,38 @@ object OnchainZapSender {
                return fail(OnchainZapSendStage.BUILDING, e.message ?: "Could not build the transaction", e)
            }

-        // 3. Sign and finalize.
+        // 3. Sign, verify the signer didn't tamper, and finalize.
        val rawTxHex =
            try {
                val signedHex = signer.signPsbt(built.psbt.toHex())
                val signedPsbt = Psbt.parse(signedHex)
+
+                // Fund-safety: the signer must ONLY add signatures. If it returns a
+                // different transaction (with valid signatures over IT), finalizing
+                // and broadcasting would send funds wherever that tx says. Reject
+                // anything whose unsigned transaction isn't byte-identical to ours.
+                val expectedTx = built.psbt.global.get(Psbt.PSBT_GLOBAL_UNSIGNED_TX)
+                val returnedTx = signedPsbt.global.get(Psbt.PSBT_GLOBAL_UNSIGNED_TX)
+                if (expectedTx == null || returnedTx == null || !expectedTx.contentEquals(returnedTx)) {
+                    return fail(
+                        OnchainZapSendStage.SIGNING,
+                        "The signer returned a different transaction than the one it was asked to sign",
+                    )
+                }
                if (!PsbtFinalizer.isFullySigned(signedPsbt)) {
                    return fail(
                        OnchainZapSendStage.SIGNING,
                        "The signer did not sign every input",
                    )
                }
+                // Verify every signature is actually valid before money moves —
+                // catches a broken signer up front instead of a doomed broadcast.
+                if (!PsbtSignatureVerifier.verifyAllKeyPathInputs(signedPsbt)) {
+                    return fail(
+                        OnchainZapSendStage.SIGNING,
+                        "The signed transaction has invalid signatures",
+                    )
+                }
                PsbtFinalizer.finalizeToHex(signedPsbt)
            } catch (e: CancellationException) {
                throw e
@@ -20,15 +20,22 @@
 */
 package com.vitorpamplona.amethyst.commons.onchain

+import com.vitorpamplona.quartz.nip01Core.core.Event
+import com.vitorpamplona.quartz.nip01Core.core.HexKey
 import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray
 import com.vitorpamplona.quartz.nip01Core.core.toHexKey
 import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair
+import com.vitorpamplona.quartz.nip01Core.signers.NostrSigner
 import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal
+import com.vitorpamplona.quartz.nip57Zaps.LnZapPrivateEvent
+import com.vitorpamplona.quartz.nip57Zaps.LnZapRequestEvent
+import com.vitorpamplona.quartz.nipBCOnchainZaps.build.OnchainZapBuilder
 import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.BitcoinTx
 import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.FeeEstimates
 import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.OnchainBackend
 import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.Utxo
 import com.vitorpamplona.quartz.nipBCOnchainZaps.psbt.BitcoinTransaction
+import com.vitorpamplona.quartz.nipBCOnchainZaps.psbt.PsbtSigner
 import com.vitorpamplona.quartz.nipBCOnchainZaps.zap.OnchainZapEvent
 import com.vitorpamplona.quartz.utils.Secp256k1Instance
 import kotlinx.coroutines.test.runTest
@@ -175,4 +182,88 @@ class OnchainZapSenderTest {
            val broadcastTxid = BitcoinTransaction.parse(backend.broadcastedHex!!).txid()
            assertEquals(broadcastTxid, result.broadcastTxid)
        }
+
+    /**
+     * A signer that ignores the PSBT it was handed and instead signs a
+     * completely different transaction of its own — the substitution attack.
+     */
+    private class TamperingSigner(
+        private val inner: NostrSignerInternal,
+        private val attackerControlledPubKey: HexKey,
+    ) : NostrSigner(inner.pubKey) {
+        override fun isWriteable() = inner.isWriteable()
+
+        override fun hasForegroundSupport() = inner.hasForegroundSupport()
+
+        override suspend fun <T : Event> sign(
+            createdAt: Long,
+            kind: Int,
+            tags: Array<Array<String>>,
+            content: String,
+        ): T = inner.sign(createdAt, kind, tags, content)
+
+        override suspend fun nip04Encrypt(
+            plaintext: String,
+            toPublicKey: HexKey,
+        ) = inner.nip04Encrypt(plaintext, toPublicKey)
+
+        override suspend fun nip04Decrypt(
+            ciphertext: String,
+            fromPublicKey: HexKey,
+        ) = inner.nip04Decrypt(ciphertext, fromPublicKey)
+
+        override suspend fun nip44Encrypt(
+            plaintext: String,
+            toPublicKey: HexKey,
+        ) = inner.nip44Encrypt(plaintext, toPublicKey)
+
+        override suspend fun nip44Decrypt(
+            ciphertext: String,
+            fromPublicKey: HexKey,
+        ) = inner.nip44Decrypt(ciphertext, fromPublicKey)
+
+        override suspend fun decryptZapEvent(event: LnZapRequestEvent): LnZapPrivateEvent = inner.decryptZapEvent(event)
+
+        override suspend fun deriveKey(nonce: HexKey): HexKey = inner.deriveKey(nonce)
+
+        override suspend fun signPsbt(psbtHex: String): String {
+            // Discard the requested PSBT entirely; build and sign one that pays
+            // the attacker instead, then hand it back as if it were the answer.
+            val malicious =
+                OnchainZapBuilder.build(
+                    senderPubKey = inner.pubKey,
+                    recipientPubKey = attackerControlledPubKey,
+                    amountSats = 200_000L,
+                    feeRateSatPerVByte = 1.0,
+                    availableUtxos = listOf(Utxo("9".repeat(64), 0, 250_000L, 6)),
+                )
+            PsbtSigner.signKeyPathInputs(malicious.psbt, inner.keyPair.privKey!!)
+            return malicious.psbt.toHex()
+        }
+    }
+
+    @Test
+    fun rejectsASignerThatReturnsADifferentTransaction() =
+        runTest {
+            val backend = FakeBackend(listOf(Utxo("5".repeat(64), 0, 250_000L, 6)))
+            val tamperingSigner = TamperingSigner(senderSigner, attackerControlledPubKey = recipientPubKey)
+
+            val result =
+                OnchainZapSender.send(
+                    backend = backend,
+                    signer = tamperingSigner,
+                    senderPubKey = senderPubKey,
+                    recipientPubKey = recipientPubKey,
+                    amountSats = 50_000L,
+                    feeRateSatPerVByte = 5.0,
+                    comment = "",
+                    zappedEvent = null,
+                ) { senderSigner.sign(it) }
+
+            // The substituted transaction must be rejected at the signing stage,
+            // and nothing must have been broadcast.
+            assertIs<OnchainZapSendResult.Failure>(result)
+            assertEquals(OnchainZapSendStage.SIGNING, result.stage)
+            assertEquals(null, backend.broadcastedHex)
+        }
 }