remove unused signature verification results in satochip and keycard signers, note where signatures are verified
What the developer wrote
Authored by Craig Raw
remove unused signature verification results in satochip and keycard signers, note where signatures are verified
What changed, and why it matters
This commit removes leftover code that checked whether signatures from hardware card signers were valid, but then threw away the result. The signatures are still verified later by a different part of the wallet when the signed transaction is combined. The change is essentially a cleanup with no known security flaw, though it slightly reduces defense-in-depth by removing an early, unused sanity check.
No immediate action required. Reviewers may verify that PSBT.verifyCombinedSignatures() is indeed invoked for all signed PSBTs returned by these signers, preserving the post-signing verification guarantee. Consider whether an active early-verification failure path is desirable for defense-in-depth, but the current change is consistent cleanup.
Security signals we found
Removal of local signature verification calls in hardware signer code paths
Added comments documenting that signature verification occurs later via PSBT.verifyCombinedSignatures()
Unused boolean results indicate the removed checks were dead code rather than active security controls
Evidence from the diff
In KeycardApi.java and SatoCardApi.java, the code previously created a TransactionSignature, called pubkey.verify(hash, txSig) to check it, and discarded the boolean result. The commit removes these unused verification calls and adds comments noting that PSBT.verifyCombinedSignatures() performs verification when the signed PSBT is combined. The functional behavior of returned signatures is unchanged; only an unused local verification step is removed.
Changed components
com.sparrowwallet.sparrow.io.keycard.KeycardApicom.sparrowwallet.sparrow.io.satochip.SatoCardApiHardware wallet signing paths for ECDSA and Schnorr signaturesInspect captured patch +5 / −13
diff --git a/src/main/java/com/sparrowwallet/sparrow/io/keycard/KeycardApi.java b/src/main/java/com/sparrowwallet/sparrow/io/keycard/KeycardApi.java
index 59c7be4..7b7a35b 100644
--- a/src/main/java/com/sparrowwallet/sparrow/io/keycard/KeycardApi.java
+++ b/src/main/java/com/sparrowwallet/sparrow/io/keycard/KeycardApi.java
@@ -370,6 +370,7 @@ public class KeycardApi extends CardApi {
this.fullPath = fullPath;
}
+ //Signatures returned here are verified against the wallet keys by PSBT.verifyCombinedSignatures() when the signed PSBT is combined
@Override
public TransactionSignature sign(Sha256Hash hash, SigHash sigHash, TransactionSignature.Type signatureType) {
try {
@@ -383,10 +384,8 @@ public class KeycardApi extends CardApi {
pubkey = ECKey.fromPublicOnly(compressedPub(sig.getPublicKey()));
ECDSASignature ecdsaSig = new ECDSASignature(new BigInteger(1, sig.getR()), new BigInteger(1, sig.getS())).toCanonicalised();
- TransactionSignature txSig = new TransactionSignature(ecdsaSig, sigHash);
- boolean isCorrect = pubkey.verify(hash, txSig);
- return txSig;
+ return new TransactionSignature(ecdsaSig, sigHash);
} else {
throw new CardException(WalletModel.KEYCARD.toDisplayString() + " cannot sign Taproot transactions");
}
diff --git a/src/main/java/com/sparrowwallet/sparrow/io/satochip/SatoCardApi.java b/src/main/java/com/sparrowwallet/sparrow/io/satochip/SatoCardApi.java
index 2d28991..edf4001 100644
--- a/src/main/java/com/sparrowwallet/sparrow/io/satochip/SatoCardApi.java
+++ b/src/main/java/com/sparrowwallet/sparrow/io/satochip/SatoCardApi.java
@@ -301,6 +301,7 @@ public class SatoCardApi extends CardApi {
this.fullPath = fullPath;
}
+ //Signatures returned here are verified against the wallet keys by PSBT.verifyCombinedSignatures() when the signed PSBT is combined
@Override
public TransactionSignature sign(Sha256Hash hash, SigHash sigHash, TransactionSignature.Type signatureType) {
try {
@@ -328,11 +329,7 @@ public class SatoCardApi extends CardApi {
APDUResponse rapdu2 = cardProtocol.cardSignTransactionHash(keynbr, hash.getBytes(), chalresponse);
byte[] sigBytes = rapdu2.getData();
ECDSASignature ecdsaSig = ECDSASignature.decodeFromDER(sigBytes).toCanonicalised();
- TransactionSignature txSig = new TransactionSignature(ecdsaSig, sigHash);
-
- // verify
- boolean isCorrect = pubkey.verify(hash, txSig);
- return txSig;
+ return new TransactionSignature(ecdsaSig, sigHash);
} else {
// Satochip supports schnorr signature only for version >= 0.14 !
byte[] versionBytes = cardStatus.getCardVersion();
@@ -353,12 +350,8 @@ public class SatoCardApi extends CardApi {
APDUResponse rapdu2 = cardProtocol.cardSignSchnorrHash(keynbr, hash.getBytes(), chalresponse);
byte[] sigBytes = rapdu2.getData();
SchnorrSignature schnorrSig = SchnorrSignature.decode(sigBytes);
- TransactionSignature txSig = new TransactionSignature(schnorrSig, sigHash);
-
- // verify sig with outputPubkey...
- boolean isCorrect2 = pubkey.verify(hash, txSig);
- return txSig; //new TransactionSignature(schnorrSig, sigHash);
+ return new TransactionSignature(schnorrSig, sigHash);
}
} catch(Exception e) {
throw new RuntimeException(e);
Why this scored 18/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.