fix(bolt12): audit fixes — offer binding, verified-only counting, lower-amount dedup, codec hardening

From an adversarial audit of the BOLT12-zap feature. Security / correctness: - Offer↔invoice binding: `cryptoVerified=true` was asserted even when the offer had no `offer_issuer_id` or used blinded paths — cases where the invoice's node key is payer-chosen and can't be tied to the offer. An attacker could self-sign a "verified" proof having paid nothing. Now cryptoVerified requires the invoice to be provably the offer's (issuer_id present, no paths, invoice_node_id == issuer); unbindable proofs are accepted but flagged unverified, not verified. Definite contradictions still hard-reject. - Counting: `updateZapTotal` now counts ONLY crypto-verified BOLT12 zaps. An unverified (compressed / unbindable) proof carries a self-chosen preimage+amount with no settled- payment guarantee, so counting it let anyone inflate a note's total for free. Unverified entries stay stored + shown (dimmed), never summed. - Dedup: `innerAddBolt12Zap` now honors the NIP's "count the LOWER amount for the same payment hash" rule (was order-dependent last-writer-wins, inflatable by re-publishing a bigger amount tag). Keeps the stronger verification flag. - Precision: divide millisats in BigDecimal, so fractional sats survive and match the millisat-native lightning column (was integer `/1000`, flooring sub-sat zaps to 0). Codec hardening (quartz): - TLV length now range-checked (was a signed compare that let a high-bit BigSize length slip through and get truncated by toInt()). - BigSize enforces minimal encoding (also rejects >=2^63 values that read back negative). - bech32 alphabet membership is O(1) via a lookup table (was O(32n) indexOf per char). UI: - ReusableZapButton's "you zapped" gate now includes bolt12Zaps (and nutzaps/onchain), so a BOLT12-only zap correctly shows the zapped state. - The reactions gallery renders the blank/unknown author for anonymous zaps, matching the standalone card (was showing the throwaway ephemeral key's avatar). Tests: validator issuer-less-offer downgrade; TLV non-minimal-BigSize + oversized-length rejection; model lower-amount dedup (both orderings), verified-only counting, and fractional-sat survival. NoteBolt12ZapTest 6→8, Bolt12ZapValidatorTest 11→12, TlvTest 6→8. The audit also surfaced a NIP-level gap that is NOT fixable in code and is captured in the plan doc: there is no offer↔recipient-identity binding, so even a crypto-verified proof only proves payment to the *embedded* offer, not to the p-tagged recipient. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpgpWLKzgD7vS9Fs4CXTR3
2026-08-09 16:14:40 +00:00 · 2026-07-23 21:17:01 +00:00
parent 3e19b76343
commit 0b6a70ad26
10 changed files with 162 additions and 37 deletions
@@ -44,6 +44,7 @@ import com.vitorpamplona.amethyst.ui.theme.Size25dp
 import com.vitorpamplona.amethyst.ui.theme.Size35Modifier
 import com.vitorpamplona.amethyst.ui.theme.StdStartPadding
 import com.vitorpamplona.amethyst.ui.theme.WidthAuthorPictureModifier
+import com.vitorpamplona.quartz.nipXXBolt12Zaps.zap.Bolt12ZapEvent
 import kotlinx.collections.immutable.ImmutableList
 import kotlinx.collections.immutable.persistentListOf
 import kotlinx.collections.immutable.toImmutableList
@@ -121,7 +122,10 @@ private fun Bolt12ZapEntryRow(
    nav: INav,
    accountViewModel: AccountViewModel,
 ) {
-    val user = entry.source.author
+    // Anonymous zaps carry no `P` payer tag — show the blank/unknown author (as the
+    // standalone card does) instead of the throwaway ephemeral key's default avatar.
+    val isAnonymous = (entry.source.event as? Bolt12ZapEvent)?.isAnonymous() == true
+    val user = if (isAnonymous) null else entry.source.author

    // The amount is validated (checked against the proof's invoice_amount), so it
    // is safe to show for any sender. A not-yet-crypto-verified (compressed) proof
@@ -1551,7 +1551,12 @@ fun ObserveZapIcon(
        }

        LaunchedEffect(key1 = zapsState) {
-            if (zapsState?.note?.zapPayments?.isNotEmpty() == true || zapsState?.note?.zaps?.isNotEmpty() == true) {
+            if (zapsState?.note?.zapPayments?.isNotEmpty() == true ||
+                zapsState?.note?.zaps?.isNotEmpty() == true ||
+                zapsState?.note?.nutzaps?.isNotEmpty() == true ||
+                zapsState?.note?.onchainZaps?.isNotEmpty() == true ||
+                zapsState?.note?.bolt12Zaps?.isNotEmpty() == true
+            ) {
                val newWasZapped = accountViewModel.calculateIfNoteWasZappedByAccount(baseNote, afterTimeInSeconds)
                if (wasZappedByLoggedInUser.value != newWasZapped) {
                    wasZappedByLoggedInUser.value = newWasZapped
@@ -739,14 +739,20 @@ open class Note(
    ): Boolean =
        syncLock.withLock {
            val existing = bolt12Zaps[paymentHashHex]
-            if (existing != null) {
-                // Same settled payment (dedup by invoice_payment_hash) — a relay echo.
-                if (entry == existing) return@withLock false
-                // Prefer a fully crypto-verified entry; never let an unverified
-                // (compressed-proof) republish overwrite a verified one.
-                if (!entry.cryptoVerified && existing.cryptoVerified) return@withLock false
-            }
-            bolt12Zaps = bolt12Zaps + Pair(paymentHashHex, entry)
+            val merged =
+                if (existing == null) {
+                    entry
+                } else {
+                    // Same settled payment (dedup by invoice_payment_hash). NIP-XX: count
+                    // only one, and if amounts differ, keep the LOWER — so a re-publish
+                    // with a bigger amount tag can't inflate the total. Keep the stronger
+                    // verification flag, and the source of whichever entry we keep the
+                    // amount from.
+                    val keepEntry = if (entry.amountMillisats < existing.amountMillisats) entry else existing
+                    keepEntry.copy(cryptoVerified = entry.cryptoVerified || existing.cryptoVerified)
+                }
+            if (merged == existing) return@withLock false
+            bolt12Zaps = bolt12Zaps + Pair(paymentHashHex, merged)
            return@withLock true
        }

@@ -1102,11 +1108,16 @@ open class Note(
            sumOfAmounts += BigDecimal(entry.claimedSats)
        }

-        // NIP-XX BOLT12 zaps — validated synchronously at consume time (the `lnp`
-        // payer proof is a self-contained settlement proof), so every stored entry
-        // counts, converting its millisat amount to sats like the lightning path.
+        // NIP-XX BOLT12 zaps — count only the crypto-verified ones. An unverified
+        // (compressed, or offer-unbindable) proof carries a self-chosen preimage,
+        // amount, and payment hash with no settled-payment guarantee, so counting it
+        // would let anyone inflate a note's total for free. Unverified entries stay
+        // stored and shown (dimmed) but never sum. Divide in BigDecimal so fractional
+        // sats survive and match the millisat-native lightning column above.
        bolt12Zaps.values.forEach { entry ->
-            sumOfAmounts += BigDecimal(entry.amountMillisats / 1000)
+            if (entry.cryptoVerified) {
+                sumOfAmounts += BigDecimal(entry.amountMillisats).divide(BigDecimal(1000))
+            }
        }

        zapsAmount = sumOfAmounts
@@ -55,6 +55,21 @@ class NoteBolt12ZapTest {
        assertEquals(21_000L, target.zapsAmount.toLong())
    }

+    @Test
+    fun keepsTheLowerAmountWhenTwoProofsShareAPaymentHash() {
+        // NIP-XX: same proof identifier, differing amounts → count the LOWER, in either order.
+        val a = note("a".repeat(64))
+        a.addBolt12Zap(note("b".repeat(64)), "h", amountMillisats = 21_000_000L, cryptoVerified = true)
+        a.addBolt12Zap(note("c".repeat(64)), "h", amountMillisats = 5_000_000L, cryptoVerified = true)
+        assertEquals(1, a.bolt12Zaps.size)
+        assertEquals(5_000L, a.zapsAmount.toLong())
+
+        val b = note("a".repeat(64))
+        b.addBolt12Zap(note("b".repeat(64)), "h", amountMillisats = 5_000_000L, cryptoVerified = true)
+        b.addBolt12Zap(note("c".repeat(64)), "h", amountMillisats = 21_000_000L, cryptoVerified = true)
+        assertEquals(5_000L, b.zapsAmount.toLong(), "order must not change the counted amount")
+    }
+
    @Test
    fun aVerifiedEntryIsNotDowngradedByAnUnverifiedRepublish() {
        val target = note("a".repeat(64))
@@ -93,13 +108,23 @@ class NoteBolt12ZapTest {
    }

    @Test
-    fun bolt12ZapsCombineWithLightningTotalsIndependently() {
+    fun onlyCryptoVerifiedBolt12ZapsCountTowardTheTotal() {
        val target = note("a".repeat(64))
-        // Two BOLT12 zaps; no lightning receipts on this note.
        target.addBolt12Zap(note("b".repeat(64)), "h1", amountMillisats = 2_000_000L, cryptoVerified = true)
+        // An unverified (compressed / unbindable) proof is stored + shown but MUST NOT count —
+        // its amount is self-chosen with no settled-payment guarantee.
        target.addBolt12Zap(note("c".repeat(64)), "h2", amountMillisats = 500_000L, cryptoVerified = false)

-        // Both count (validated == counted), regardless of crypto-verification state.
-        assertEquals(2_500L, target.zapsAmount.toLong())
+        assertEquals(2, target.bolt12Zaps.size, "both are stored (the unverified one still renders, dimmed)")
+        assertEquals(2_000L, target.zapsAmount.toLong(), "only the verified 2000-sat zap counts")
+    }
+
+    @Test
+    fun fractionalSatAmountsSurviveInTheTotal() {
+        val target = note("a".repeat(64))
+        // 1500 msat = 1.5 sat; two of them = 3 sat. Integer-dividing each first drops to 2.
+        target.addBolt12Zap(note("b".repeat(64)), "h1", amountMillisats = 1_500L, cryptoVerified = true)
+        target.addBolt12Zap(note("c".repeat(64)), "h2", amountMillisats = 1_500L, cryptoVerified = true)
+        assertEquals(3L, target.zapsAmount.toLong(), "fractional sats must not be truncated per-entry")
    }
 }
@@ -61,6 +61,14 @@ object Bolt12Bech32 {
        return sb.toString().lowercase()
    }

+    // O(1) bech32 data-alphabet membership, indexed by char code (ASCII only).
+    private val IS_BECH32_CHAR =
+        BooleanArray(128).also { table ->
+            for (c in Bech32.ALPHABET) table[c.code] = true
+        }
+
+    private fun isBech32Char(c: Char): Boolean = c.code < 128 && IS_BECH32_CHAR[c.code]
+
    private fun hasHrp(
        canonical: String,
        hrp: String,
@@ -70,7 +78,7 @@ object Bolt12Bech32 {
        if (!canonical.startsWith(prefix)) return false
        // every data char must be in the bech32 alphabet
        for (i in prefix.length until canonical.length) {
-            if (Bech32.ALPHABET.indexOf(canonical[i]) < 0) return false
+            if (!isBech32Char(canonical[i])) return false
        }
        return true
    }
@@ -91,10 +91,13 @@ class TlvReader(

    fun readBigSize(): Long {
        val first = readByte()
+        // BOLT-1 requires minimal encoding. Rejecting non-minimal forms also rejects
+        // the multi-byte forms whose value would overflow a signed Long (≥ 2^63 reads
+        // back negative, failing the `>=` bound), so callers get a non-negative Long.
        return when (first) {
-            0xff -> readUInt(8)
-            0xfe -> readUInt(4)
-            0xfd -> readUInt(2)
+            0xff -> readUInt(8).also { require(it >= 0x100000000L) { "non-minimal or out-of-range BigSize" } }
+            0xfe -> readUInt(4).also { require(it >= 0x10000L) { "non-minimal BigSize" } }
+            0xfd -> readUInt(2).also { require(it >= 0xfdL) { "non-minimal BigSize" } }
            else -> first.toLong()
        }
    }
@@ -167,7 +170,7 @@ class TlvStream(
            while (reader.remaining() > 0) {
                val type = reader.readBigSize()
                val length = reader.readBigSize()
-                require(length <= reader.remaining()) { "TLV length $length exceeds the remaining stream" }
+                require(length in 0..reader.remaining().toLong()) { "TLV length $length out of range (remaining ${reader.remaining()})" }
                val value = reader.readBytes(length.toInt())
                require(type > lastType) { "TLV records must be strictly ascending (saw $type after $lastType)" }
                lastType = type
@@ -127,7 +127,7 @@ class Bolt12ZapValidator(
        val invoiceAmount = proof.invoiceAmount() ?: return invalid(Reason.MISSING_INVOICE_AMOUNT)
        if (invoiceAmount != amount) return invalid(Reason.PROOF_AMOUNT_MISMATCH)

-        offerBindingFailure(offerParsed, proof)?.let { return invalid(it) }
+        offerBindingHardFailure(offerParsed, proof)?.let { return invalid(it) }

        // --- Expensive checks (signatures + proof crypto) ------------------------

@@ -135,13 +135,26 @@ class Bolt12ZapValidator(
        if (!intent.verify()) return invalid(Reason.BAD_INTENT_SIGNATURE)

        val cryptoResult = proofVerifier.verify(proof)
-        val cryptoVerified =
+        val cryptoOk =
            when (cryptoResult) {
                is Bolt12ProofResult.Valid -> true
                is Bolt12ProofResult.Unsupported -> false
                is Bolt12ProofResult.Invalid -> return invalid(mapProofReason(cryptoResult.reason))
            }

+        // "Crypto verified" requires BOTH that the invoice signature was checked AND
+        // that the signed invoice is provably the offer's — i.e. signed by the offer's
+        // `offer_issuer_id`. When the offer hides its destination behind blinded paths,
+        // or publishes no issuer id, the invoice node key is one the payer chose and
+        // can't be tied to the offer here, so "paid this offer" is NOT proven; such a
+        // proof is downgraded to unverified rather than asserted verified.
+        //
+        // NB: even a bound proof only proves payment to the *embedded* offer. The NIP
+        // has no offer↔recipient-identity binding, so nothing here proves the offer
+        // belongs to the p-tagged recipient — see
+        // quartz/plans/2026-07-23-bolt12-zap-interop-vectors.md.
+        val cryptoVerified = cryptoOk && isInvoiceBoundToOffer(offerParsed, proof)
+
        val paymentHash = proof.invoicePaymentHash() ?: return invalid(Reason.PROOF_MISSING_REQUIRED_FIELDS)

        return Bolt12ZapValidation.Valid(
@@ -167,14 +180,13 @@ class Bolt12ZapValidator(
    }

    /**
-     * Soft binding of the proof to the offer without processing blinded paths:
-     * when the offer publishes an `offer_issuer_id` and no blinded paths, the
-     * invoice must be signed by that same node id, and any `offer_issuer_id`
-     * copied into the proof must match. Offers that route through blinded paths
-     * carry a per-path node id we can't check here, so they are left to the
-     * signature verification alone.
+     * A definite contradiction between the proof and the offer — the proof either
+     * copies a different `offer_issuer_id`, or (for a directly-addressed offer)
+     * carries an `invoice_node_id` that isn't the offer's issuer. These are hard
+     * rejects. Note the *absence* of a check is NOT a pass here — that only means
+     * we can't bind, which [isInvoiceBoundToOffer] reports separately.
     */
-    private fun offerBindingFailure(
+    private fun offerBindingHardFailure(
        offer: Bolt12Offer,
        proof: Bolt12PayerProof,
    ): Reason? {
@@ -191,6 +203,24 @@ class Bolt12ZapValidator(
        return null
    }

+    /**
+     * True only when the settled invoice can be cryptographically tied to the offer:
+     * the offer publishes an `offer_issuer_id`, uses no blinded paths, and the proof's
+     * `invoice_node_id` equals that issuer (so the invoice signature the verifier
+     * checks was made by the offer's own node). Blinded-path or issuer-less offers
+     * expose a payer-chosen node key that can't be bound to the offer here, so a
+     * self-consistent proof against such an offer proves nothing about paying it.
+     */
+    private fun isInvoiceBoundToOffer(
+        offer: Bolt12Offer,
+        proof: Bolt12PayerProof,
+    ): Boolean {
+        val issuerId = offer.issuerId() ?: return false
+        if (offer.hasPaths()) return false
+        val nodeId = proof.invoiceNodeId() ?: return false
+        return nodeId.contentEquals(issuerId)
+    }
+
    private fun mapProofReason(reason: Bolt12ProofResult.Reason): Reason =
        when (reason) {
            Bolt12ProofResult.Reason.MISSING_REQUIRED_FIELDS -> Reason.PROOF_MISSING_REQUIRED_FIELDS
@@ -87,6 +87,25 @@ class TlvTest {
        assertFailsWith<IllegalArgumentException> { TlvStream.read(outOfOrder) }
    }

+    @Test
+    fun bigSizeRejectsNonMinimalEncodings() {
+        // 5 encoded in the 3-byte (0xfd) form instead of a single byte.
+        assertFailsWith<IllegalArgumentException> { TlvReader(byteArrayOf(0xfd.toByte(), 0x00, 0x05)).readBigSize() }
+        // 0x100 encoded in the 5-byte (0xfe) form instead of 3.
+        assertFailsWith<IllegalArgumentException> {
+            TlvReader(byteArrayOf(0xfe.toByte(), 0x00, 0x00, 0x01, 0x00)).readBigSize()
+        }
+    }
+
+    @Test
+    fun tlvStreamRejectsAnOversizedOrHighBitLength() {
+        // type=1 (minimal), length=0xff FF FF FF FF 00 00 00 05 — reads back negative / huge.
+        val hostile =
+            byteArrayOf(0x01) +
+                byteArrayOf(0xff.toByte(), 0xff.toByte(), 0xff.toByte(), 0xff.toByte(), 0xff.toByte(), 0x00, 0x00, 0x00, 0x05)
+        assertFailsWith<IllegalArgumentException> { TlvStream.read(hostile) }
+    }
+
    @Test
    fun signatureElementRangeIsRecognized() {
        assertEquals(false, TlvRecord(176, byteArrayOf()).isSignatureElement())
@@ -44,13 +44,15 @@ object Bolt12ProofFixture {
    fun buildOffer(
        nodeKey: KeyPair,
        amountMillisats: Long,
+        withIssuerId: Boolean = true,
    ): String {
+        // TLV types must be strictly ascending: amount(8), description(10), issuer_id(22).
        val records =
-            listOf(
-                TlvRecord(Bolt12Offer.TYPE_AMOUNT, Bolt12Values.tu64ToBytes(amountMillisats)),
-                TlvRecord(Bolt12Offer.TYPE_DESCRIPTION, "zap".encodeToByteArray()),
-                TlvRecord(Bolt12Offer.TYPE_ISSUER_ID, point(nodeKey.pubKey)),
-            )
+            buildList {
+                add(TlvRecord(Bolt12Offer.TYPE_AMOUNT, Bolt12Values.tu64ToBytes(amountMillisats)))
+                add(TlvRecord(Bolt12Offer.TYPE_DESCRIPTION, "zap".encodeToByteArray()))
+                if (withIssuerId) add(TlvRecord(Bolt12Offer.TYPE_ISSUER_ID, point(nodeKey.pubKey)))
+            }
        return Bolt12Bech32.encode(Bolt12Bech32.OFFER_HRP, TlvStream(records).encode())
    }

@@ -199,6 +199,24 @@ class Bolt12ZapValidatorTest {
            assertEquals(Bolt12ZapValidation.Invalid(Bolt12ZapValidation.Reason.PAYER_TAG_MISMATCH), validator.validate(zap))
        }

+    @Test
+    fun anIssuerlessOfferCannotBindTheInvoiceSoTheZapIsAcceptedButUnverified() =
+        runTest {
+            // The offer publishes no offer_issuer_id, so the invoice's node key can't be
+            // tied to the offer — a self-consistent proof proves nothing about paying it.
+            val signer = NostrSignerInternal(KeyPair())
+            val nodeKey = KeyPair()
+            val preimage = ByteArray(32) { (it + 12).toByte() }
+            val offer = Bolt12ProofFixture.buildOffer(nodeKey, amount, withIssuerId = false)
+            val intent = signedIntent(signer, offer)
+            val note = Bolt12ZapValidator.NIP_URI_PREFIX + intent.id
+            val proof = Bolt12ProofFixture.buildProof(nodeKey, KeyPair(), preimage, amount, note)
+
+            val result = validator.validate(signedZap(signer, intent, proof))
+            assertIs<Bolt12ZapValidation.Valid>(result)
+            assertTrue(!result.proofCryptoVerified, "an offer with no issuer id cannot be crypto-verified")
+        }
+
    @Test
    fun rejectsWhenTheProofDoesNotMatchTheOffer() =
        runTest {