fix(sdk-coin-avaxp): prevent credential wipe and expose incomplete signing state

[nvrakesh06]

Summary

• hasCredentials now checks != null only (was !== undefined && length > 0). credentials=[] previously bypassed the guard in buildAvaxTransaction(), causing it to regenerate fresh placeholder credentials and wipe any signature already placed by a prior signer.
• sign() adds an explicit length === 0 guard to compensate — empty credentials still throw an appropriate error.
• signature getter now computes the intersection of real ECDSAs across all credentials. A signer's ECDSA is counted only if it appears in every credential. Since RFC6979 is deterministic (same key + same tx hash = same bytes), a missing signature in any one credential is now detectable via toJson().signatures before the tx reaches the sendq.
• toBroadcastFormat() throws if a real ECDSA coexists with an address placeholder (r=0) in the same credential set — the exact shape produced by the guard bypass. This fires regardless of which upstream path produced the corruption.
• toBroadcastFormat() also throws if credentials=[] — a bug state where hasCredentials returns true but no signer has touched the tx, which would otherwise serialize to a zero-credential tx.

---

Changes Explained

Fix 1 — hasCredentials getter (core bug)

- return this.credentials !== undefined && this.credentials.length > 0;
+ // Must use != null (not length check) so credentials=[] still blocks buildAvaxTransaction()
+ // from wiping a partially-signed state.
+ return this.credentials != null;

The bug: credentials = [] (empty array) returned false because [].length > 0 is false. buildAvaxTransaction() in the builder checks if (this.transaction.hasCredentials) return; before regenerating credentials. When hasCredentials was false on a halfSigned tx (Signer 1's ECDSA in slot[0], Signer 2's placeholder in slot[1]), the guard failed to fire — buildAvaxTransaction() ran again and regenerated fresh placeholder credentials, wiping Signer 1's ECDSA from the in-memory transaction.

The fix: != null is true for [] — everything except null and undefined. The semantics change from "credentials exist AND are non-empty" to "credentials field has been set from a parsed tx". If the avalanche SDK sets credentials = [] during deserialization of the halfSigned tx, we still block the regeneration path.

Note: The root cause of why credentials=[] arose on Jul 16 is not yet confirmed — PVMTx.fromBuffer() producing an empty array is unproven. This is hardening that blocks the guard bypass if that edge case occurs.

---

Fix 2 — sign() guard extended

- if (!this.hasCredentials) {
+ if (!this.credentials || this.credentials.length === 0) {
     throw new InvalidTransactionError('empty credentials to sign');
   }

Why: After Fix 1, hasCredentials returns true for credentials = []. Without this extra guard, sign() would call this.credentials.forEach(...) on an empty array — silently doing nothing and producing no signature.

---

Fix 3 — signature getter uses intersection across all credentials

- if (this.credentials.length === 0) {
+ if (!this.credentials || this.credentials.length === 0) {
     return [];
   }
- const obj: any = this.credentials[0].serialize();
- return obj.sigArray.map((s) => s.bytes).filter((s) => !isEmptySignature(s));
+ // Intersection: a signer's ECDSA is counted only if present in every credential.
+ let intersection: Set<string> | null = null;
+ for (const c of this.credentials) {
+   const cs: any = c.serialize();
+   const credSigs = new Set<string>(
+     cs.sigArray.map((s: any) => s.bytes as string).filter((b: string) => !isEmptySignature(b))
+   );
+   if (intersection === null) {
+     intersection = credSigs;
+   } else {
+     for (const sig of intersection) {
+       if (!credSigs.has(sig)) intersection.delete(sig);
+     }
+   }
+ }
+ return intersection ? [...intersection] : [];

The bug: the old getter only inspected credentials[0]. For a multi-UTXO tx (N inputs → N credentials), cred[0] = {ECDSA1, ECDSA2} while cred[1] = {ECDSA1, placeholder} would return length 2 — falsely showing a fully-signed state.

The fix: intersection semantics. RFC6979 guarantees the same key produces identical bytes for the same tx across all credentials, so the intersection correctly reflects how many signers have signed every input. length === 1 means halfSigned; length === 2 means ready to broadcast. The corrupt state above now returns length 1.

---

Fix 4 — toBroadcastFormat() rejects mixed ECDSA + addr placeholder

+ if (this.credentials && this.credentials.length > 0) {
+   let hasRealSig = false, hasAddrPlaceholder = false;
+   for (const c of this.credentials) {
+     const cs: any = c.serialize();
+     for (const s of cs.sigArray) {
+       if (!isEmptySignature(s.bytes)) hasRealSig = true;
+       else if (isAddrPlaceholder(s.bytes)) hasAddrPlaceholder = true;
+     }
+   }
+   if (hasRealSig && hasAddrPlaceholder) {
+     throw new InvalidTransactionError(
+       'transaction has a real ECDSA alongside an address placeholder (r=0): incomplete signing detected, refusing broadcast'
+     );
+   }
+ }

This is the catch-all guard suggested in review. It fires regardless of which path produced the corrupt credential shape — the production Jul 16 tx would have been blocked here even without the hasCredentials fix. isAddrPlaceholder() distinguishes a 90-zero-hex-char prefix followed by non-zero address bytes from a purely empty slot (all zeros), so half-signed txs with unfilled empty slots pass safely.

---

Fix 5 — toBroadcastFormat() rejects credentials=[]

+ if (this.credentials != null && this.credentials.length === 0) {
+   throw new InvalidTransactionError('transaction has no credentials — cannot broadcast');
+ }

credentials=[] is always a bug state: hasCredentials returns true (correct — the field was initialized from a parsed tx), but there are no credentials to include in the serialized tx. Unsigned txs have credentials = null (field not yet set), so this check does not affect the normal unsigned-tx flow.

---

Why all five fixes

Caller	Before	After
buildAvaxTransaction() guard	hasCredentials([]) = false → guard misses → credentials wiped	hasCredentials([]) = true → guard fires → credentials preserved
sign()	credentials=[] → .forEach no-op, silent failure	credentials=[] → throws "empty credentials to sign"
signature getter	reads credentials[0] only → corrupt credentials[1..N] invisible	intersection across all credentials → incomplete signing detectable
toBroadcastFormat()	ECDSA + addr placeholder silently serialized → node rejects	throws "incomplete signing detected"
toBroadcastFormat()	credentials=[] → zero-credential tx serialized silently	throws "no credentials — cannot broadcast"

---

Background

Production incident CECHO-1697 (2026-07-16): a custodial 2-of-3 AVAXP ExportTx P→C was broadcast with sig[1] still containing an address placeholder (r=0) in every credential, causing AvalancheGo to reject with:

failed verifySpend: failed to verify transfer: invalid signature

Tx: 8xbiLpsKDKkJrN3YUqcZpFcxApLCwmQkpKUuvmHU1GL9RmAyu

Root cause hypothesis: if credentials=[] arises during BitGo Signer 2's deserialization of the half-signed tx, hasCredentials returned false, buildAvaxTransaction() regenerated fresh placeholders (wiping Signer 1's ECDSA), and Signer 2 signed into the wrong slot — leaving slot[1] as address placeholder with r=0. The exact trigger for credentials=[] from PVMTx.fromBuffer() is not yet confirmed.

A retry the next day with a different signer succeeded on the same unsigned tx, confirming the bug is in the credential guard path, not the UTXOs or policy.

Note: The definitive fix for the production failure path requires a wallet-platform change to validate all credential slots have r≠0 before inserting into the sendq. These SDK changes are a hardening layer that catches the bug earlier.

---

Code Architecture — Which Classes Are Affected

sdk-coin-avaxp has two parallel transaction stacks. The bug lives in DeprecatedTransaction, the base for all six old-SDK tx types:

TransactionBuilderFactory.from(raw)
  │
  ├─ PVMTx (old "avalanche" npm package, P-chain blockchainID)
  │   ├─ ExportTxBuilder        ← P→C export  ★ production failure (typeID 18)
  │   ├─ ExportInCTxBuilder     ← C→P export    (also affected, typeID 1)
  │   ├─ ImportTxBuilder        ← P-side import (also affected)
  │   ├─ ImportInCTxBuilder     ← C-side import (also affected)
  │   ├─ ValidatorTxBuilder                     (also affected)
  │   └─ DelegatorTxBuilder                     (also affected)
  │       All extend: AtomicTransactionBuilder
  │                → DeprecatedTransactionBuilder
  │                       → DeprecatedTransaction  ← BUG IS HERE
  │
  └─ @bitgo-forks/avalanchejs (new SDK)
      └─ PermissionlessValidatorTxBuilder        (NOT affected — separate stack)

"Deprecated" refers to the old avalanche npm package, NOT the tx types. ExportTx P→C (typeID 18) is live production code and routes entirely through DeprecatedTransaction. All six builders above are affected by the hasCredentials guard bug.

---

Flow Diagrams

Happy Path (correct signing flow)

factory.from(halfSignedHex)
    │
    ▼
DeprecatedTransactionBuilder.fromImplementation()
    │
    ▼
PVMTx.fromBuffer(halfSignedHex)     ← credentials populated from bytes
initBuilder(tx)
    │
    ▼
builder.sign(key2)                  ← stored in this._signer (not called yet)
    │
    ▼
builder.build() → buildImplementation()
    │
    ▼
buildAvaxTransaction()
    │
    ▼
transaction.hasCredentials?         ← credentials != null  → TRUE
    │
    YES ──────────────────────────► return early (no rebuild)
                                        │
                                        ▼
                                    transaction.sign(key2)
                                        │
                                    generateSelectorSignature MODE 2
                                    finds first empty slot
                                        │
                                        ▼
                                    slot[1] = realECDSA2  ✓
                                        │
                                        ▼
                                    toBroadcastFormat()
                                    credentials = [ECDSA1, ECDSA2]
                                        │
                                        ▼
                                    Node accepts ✓

Broken Path (production failure — credentials=[])

factory.from(halfSignedHex)
    │
    ▼
DeprecatedTransactionBuilder.fromImplementation()
    │
    ▼
PVMTx.fromBuffer(halfSignedHex)     ← credentials becomes [] (edge case)
initBuilder(tx)
    │
    ▼
builder.sign(key2)                  ← stored in this._signer
    │
    ▼
builder.build() → buildImplementation()
    │
    ▼
buildAvaxTransaction()
    │
    ▼
transaction.hasCredentials?         ← credentials=[] → FALSE (BUG)
    │
    NO ──────────────────────────►  createInputOutput(...)
                                    generates FRESH credentials
                                    [hsm1AddrPlaceholder, emptyZeros]
                                        │
                                    realECDSA1 WIPED  ✗
                                        │
                                        ▼
                                    transaction.sign(key2)
                                        │
                                    generateSelectorSignature MODE 1
                                    all slots empty → match by address
                                    slot[0] = realECDSA2
                                    slot[1] = hsm1AddrPlaceholder (r=0)  ✗
                                        │
                                        ▼
                                    toBroadcastFormat()
                                    ← Fix 4 throws here ✓

Credential Slot States

Unsigned Tx:
  slot[0] = hsm1AddrPlaceholder (r=0)
  slot[1] = emptyZeros (r=0)
       │
       ▼ HSM Signer 1 signs
Half-signed:
  slot[0] = realECDSA1 (r≠0)  ✓
  slot[1] = emptyZeros (r=0)
       │
       ├─── hasCredentials=true (FIXED) ──►  slot[1] = realECDSA2  → Node accepts ✓
       │
       └─── hasCredentials=false (BUG) ─►   slot[0] = realECDSA2
                                             slot[1] = placeholder(r=0)
                                             → Fix 4 throws at toBroadcastFormat ✓

---

Test plan

• Run test/diag/diag3-guard-bypass.ts — confirm hasCredentials=true for credentials=[] after fix
• Run test/diag/diag4-reproduce-invalid-signature.ts — confirm Phase 3 (correct path) still produces 2 valid ECDSAs
• Run existing sdk-coin-avaxp unit tests — all regression tests in exportP2CTxBuilder.ts pass
• Enable and pass the skipped test TODO(BG-56700) in test/unit/lib/exportC2PTxBuilder.ts

🤖 Generated with Claude Code

[nayandas190]

lgtm

[Copilot]

Pull request overview

This PR hardens the legacy sdk-coin-avaxp (deprecated avalanche SDK stack) signing flow to prevent credential regeneration from wiping partially signed state, and aims to expose incomplete signing earlier via transaction serialization.

Changes:

• Update DeprecatedTransaction.hasCredentials semantics to treat credentials=[] as “initialized” (blocks rebuild/regeneration).
• Add an explicit credentials.length === 0 guard in DeprecatedTransaction.sign() to avoid silent no-op signing.
• Expand regression coverage in exportP2CTxBuilder for the credential-guard bypass incident (CECHO-1697) and signature exposure.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
modules/sdk-coin-avaxp/src/lib/deprecatedTransaction.ts	Adjusts credential guard logic, signing guard, and signature aggregation behavior in the deprecated AVAXP transaction stack.
modules/sdk-coin-avaxp/test/unit/lib/exportP2CTxBuilder.ts	Adds regression tests around credential guard bypass and signature visibility for partially signed transactions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mohd-kashif]

Review against CECHO-1697 — four inline findings on the relevant lines.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

modules/sdk-coin-avaxp/test/unit/lib/exportP2CTxBuilder.ts:235

• The intersection-regression test can silently pass without exercising the new behavior: if creds is missing/has <2 elements or targetIdx is -1, the test does no corruption and makes no assertion about the intersection result. This risks a false-positive where the test passes even if the signature getter regresses.

      // Read the real ECDSA2 bytes from credentials[0] slot 1 and remove it from credentials[1].
      const creds = (fullTx as any).credentials;
      if (creds && creds.length >= 2) {
        // Corrupt credentials[1]: zero out the second signer's slot so it looks empty.
        const cred1: any = creds[1];

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[mohd-kashif]

LGTM