fix(p2pk): validate expected pubkey correctly for p2pk inputs

mm2src/coins/qrc20.rs

@@ -847,7 +847,8 @@ impl SwapOps for Qrc20Coin {
             },
         };
         let fee_tx_hash = fee_tx.hash().reversed().into();
-        let inputs_signed_by_pub = check_all_utxo_inputs_signed_by_pub(fee_tx, validate_fee_args.expected_sender)?;
+        let inputs_signed_by_pub =
+            check_all_utxo_inputs_signed_by_pub(self, fee_tx, validate_fee_args.expected_sender)?;
         if !inputs_signed_by_pub {
             return MmError::err(ValidatePaymentError::WrongPaymentTx(
                 "The dex fee was sent from wrong address".to_string(),

mm2src/coins/utxo/utxo_common.rs

@@ -2019,19 +2019,99 @@ pub async fn send_maker_refunds_payment<T: UtxoCommonOps + SwapOps>(
     refund_htlc_payment(coin, args).await.map(|tx| tx.into())
 }
 
-/// Extracts pubkey from script sig
-fn pubkey_from_script_sig(script: &Script) -> Result<H264, String> {
+/// Extracts the signature from a scriptSig at instruction 0.
+///
+/// Usable for P2PK and P2PKH scripts.
+fn extract_signature(script: &Script) -> Result<Vec<u8>, String> {
     match script.get_instruction(0) {
         Some(Ok(instruction)) => match instruction.opcode {
             Opcode::OP_PUSHBYTES_70 | Opcode::OP_PUSHBYTES_71 | Opcode::OP_PUSHBYTES_72 => match instruction.data {
-                Some(bytes) => try_s!(SecpSignature::from_der(&bytes[..bytes.len() - 1])),
-                None => return ERR!("No data at instruction 0 of script {:?}", script),
+                Some(bytes) => Ok(bytes.to_vec()),
+                None => ERR!("No data at instruction 0 of script {:?}", script),
             },
-            _ => return ERR!("Unexpected opcode {:?}", instruction.opcode),
+            _ => ERR!("Unexpected opcode {:?}", instruction.opcode),
         },
-        Some(Err(e)) => return ERR!("Error {} on getting instruction 0 of script {:?}", e, script),
-        None => return ERR!("None instruction 0 of script {:?}", script),
+        Some(Err(e)) => ERR!("Error {} on getting instruction 0 of script {:?}", e, script),
+        None => ERR!("None instruction 0 of script {:?}", script),
+    }
+}
+
+/// Checks if a scriptSig is a script that spends a P2PK output.
+fn does_script_spend_p2pk(script: &Script) -> bool {
+    // P2PK scriptSig is just a single signature. The script should consist of a single push bytes
+    // instruction with the data as the signature.
+    extract_signature(script).is_ok() && script.get_instruction(1).is_none()
+}
+
+/// Verifies that the script that spends a P2PK is signed by the expected pubkey.
+fn verify_p2pk_input_pubkey(
+    script: &Script,
+    expected_pubkey: &Public,
+    unsigned_tx: &TransactionInputSigner,
+    index: usize,
+    signature_version: SignatureVersion,
+    fork_id: u32,
+) -> Result<bool, String> {
+    // Extract the signature from the scriptSig.
+    let signature = extract_signature(script)?;
+    // Validate the signature.
+    try_s!(SecpSignature::from_der(&signature[..signature.len() - 1]));
+    let signature = signature.into();
+    // Make sure we have no more instructions. P2PK scriptSigs consist of a single instruction only containing the signature.
+    if script.get_instruction(1).is_some() {
+        return ERR!("Unexpected instruction at position 2 of script {:?}", script);
     };
+    // Get the scriptPub for this input. We need it to get the transaction sig_hash to sign (but actually "to verify" in this case).
+    let pubkey = expected_pubkey
+        .to_secp256k1_pubkey()
+        .map_err(|e| ERRL!("Error converting plain pubkey to secp256k1 pubkey: {}", e))?;
+    // P2PK scriptPub has two valid possible formats depending on whether the public key is written in compressed or uncompressed form.
+    let possible_pubkey_scripts = [
+        Builder::build_p2pk(&Public::Compressed(pubkey.serialize().into())),
+        Builder::build_p2pk(&Public::Normal(pubkey.serialize_uncompressed().into())),
+    ];
+    for pubkey_script in possible_pubkey_scripts {
+        // Get the transaction hash that has been signed in the scriptSig.
+        let hash = match signature_hash_to_sign(
+            unsigned_tx,
+            index,
+            &pubkey_script,
+            // FIXME: But P2PK scripts never use segwit as signature version. Should we hardcode this to SignatureVersion::Base or ::ForkId?
+            //        UPD: I think we can safely remove this fixme if the following reasoning holds:
+            //        Since we are using p2pk here, this means the coin isn't segwit. right? but it's the coin supplied from the caller anyways
+            //        inside `check_all_utxo_inputs_signed_by_pub` and we can use the signature version it uses (which isn't segwit for sure).
+            //        But: why is it true that a segwit coin will never produce p2pk spends? (check with_key_pair.rs - sign_tx(), it doesn't reject signing a p2pk input)
+            signature_version,
+            SIGHASH_ALL,
+            fork_id,
+        ) {
+            Ok(hash) => hash,
+            Err(e) => return ERR!("Error calculating signature hash: {}", e),
+        };
+        // Verify that the signature is valid for the transaction hash with respect to the expected public key.
+        return match expected_pubkey.verify(&hash, &signature) {
+            Ok(is_successful) => {
+                if is_successful {
+                    Ok(true)
+                } else {
+                    // If the verification isn't successful, try the other possible pubkey script.
+                    continue;
+                }
+            },
+            Err(e) => ERR!("Error verifying signature: {}", e),
+        };
+    }
+
+    // Both possible pubkey scripts failed to verify the signature.
+    Ok(false)
+}
+
+/// Extracts pubkey from script sig
+fn pubkey_from_script_sig(script: &Script) -> Result<H264, String> {
+    // Extract the signature from the scriptSig.
+    let signature = extract_signature(script)?;
+    // Validate the signature.
+    try_s!(SecpSignature::from_der(&signature[..signature.len() - 1]));
 
     let pubkey = match script.get_instruction(1) {
         Some(Ok(instruction)) => match instruction.opcode {
@@ -2087,18 +2167,44 @@ where
     }
 }
 
-pub fn check_all_utxo_inputs_signed_by_pub(
+pub fn check_all_utxo_inputs_signed_by_pub<T: UtxoCommonOps>(
+    coin: &T,
     tx: &UtxoTx,
     expected_pub: &[u8],
 ) -> Result<bool, MmError<ValidatePaymentError>> {
-    for input in &tx.inputs {
+    let expected_pub =
+        H264::from_slice(expected_pub).map_to_mm(|e| ValidatePaymentError::TxDeserializationError(e.to_string()))?;
+    let mut unsigned_tx: TransactionInputSigner = tx.clone().into();
+    unsigned_tx.consensus_branch_id = coin.as_ref().conf.consensus_branch_id;
+
+    for (idx, input) in tx.inputs.iter().enumerate() {
+        let script = Script::from(input.script_sig.clone());
         let pubkey = if input.has_witness() {
+            // Extract the pubkey from a P2WPKH scriptSig.
             pubkey_from_witness_script(&input.script_witness).map_to_mm(ValidatePaymentError::TxDeserializationError)?
+        } else if does_script_spend_p2pk(&script) {
+            // For P2PK scriptsSigs, verfiy that the signature corresponds to the expected public key.
+            let successful_verification = verify_p2pk_input_pubkey(
+                &script,
+                &Public::Compressed(expected_pub),
+                // FIXME: For overwintered txs, we also need to set the input amount as it's used in sighash calcuations!
+                &unsigned_tx,
+                idx,
+                coin.as_ref().conf.signature_version,
+                coin.as_ref().conf.fork_id,
+            )
+            .map_to_mm(ValidatePaymentError::TxDeserializationError)?;
+            if successful_verification {
+                // No pubkey extraction for P2PK inputs. Continue.
+                continue;
+            }
+            return Ok(false);
         } else {
-            let script: Script = input.script_sig.clone().into();
+            // Extract the pubkey from a P2PKH scriptSig.
             pubkey_from_script_sig(&script).map_to_mm(ValidatePaymentError::TxDeserializationError)?
         };
-        if *pubkey != expected_pub {
+
+        if pubkey != expected_pub {
             return Ok(false);
         }
     }
@@ -2146,7 +2252,7 @@ pub fn watcher_validate_taker_fee<T: UtxoCommonOps + SwapOps>(
             };
 
             let taker_fee_tx: UtxoTx = deserialize(tx_from_rpc.hex.0.as_slice())?;
-            let inputs_signed_by_pub = check_all_utxo_inputs_signed_by_pub(&taker_fee_tx, &sender_pubkey)?;
+            let inputs_signed_by_pub = check_all_utxo_inputs_signed_by_pub(&coin, &taker_fee_tx, &sender_pubkey)?;
             if !inputs_signed_by_pub {
                 return MmError::err(ValidatePaymentError::WrongPaymentTx(format!(
                     "{}: Taker fee does not belong to the verified public key",
@@ -2278,7 +2384,7 @@ pub fn validate_fee<T: UtxoCommonOps + SwapOps>(
 ) -> ValidatePaymentFut<()> {
     let dex_address = try_f!(dex_address(&coin).map_to_mm(ValidatePaymentError::InternalError));
     let burn_address = try_f!(burn_address(&coin).map_to_mm(ValidatePaymentError::InternalError));
-    let inputs_signed_by_pub = try_f!(check_all_utxo_inputs_signed_by_pub(&tx, sender_pubkey));
+    let inputs_signed_by_pub = try_f!(check_all_utxo_inputs_signed_by_pub(&coin, &tx, sender_pubkey));
     if !inputs_signed_by_pub {
         return Box::new(futures01::future::err(
             ValidatePaymentError::WrongPaymentTx(format!(
@@ -2394,7 +2500,7 @@ pub fn watcher_validate_taker_payment<T: UtxoCommonOps + SwapOps>(
     let coin = coin.clone();
 
     let fut = async move {
-        let inputs_signed_by_pub = check_all_utxo_inputs_signed_by_pub(&taker_payment_tx, &input.taker_pub)?;
+        let inputs_signed_by_pub = check_all_utxo_inputs_signed_by_pub(&coin, &taker_payment_tx, &input.taker_pub)?;
         if !inputs_signed_by_pub {
             return MmError::err(ValidatePaymentError::WrongPaymentTx(format!(
                 "{INVALID_SENDER_ERR_LOG}: Taker payment does not belong to the verified public key"
@@ -5292,6 +5398,41 @@ fn test_pubkey_from_script_sig() {
     pubkey_from_script_sig(&script_sig_err).unwrap_err();
 }
 
+#[test]
+fn test_does_script_spend_p2pk() {
+    let script_sig = Script::from("473044022071edae37cf518e98db3f7637b9073a7a980b957b0c7b871415dbb4898ec3ebdc022031b402a6b98e64ffdf752266449ca979a9f70144dba77ed7a6a25bfab11648f6012103ad6f89abc2e5beaa8a3ac28e22170659b3209fe2ddf439681b4b8f31508c36fa");
+    assert!(!does_script_spend_p2pk(&script_sig));
+    // The scriptSig of the input spent from: https://mempool.space/tx/1db6251a9afce7025a2061a19e63c700dffc3bec368bd1883decfac353357a9d
+    let script_sig = Script::from("483045022078e86c021003cca23842d4b2862dfdb68d2478a98c08c10dcdffa060e55c72be022100f6a41da12cdc2e350045f4c97feeab76a7c0ab937bd8a9e507293ce6d37c9cc201");
+    assert!(does_script_spend_p2pk(&script_sig));
+}
+
+#[test]
+fn test_verify_p2pk_input_pubkey() {
+    // 65-byte (uncompressed) pubkey example.
+    // https://mempool.space/tx/1db6251a9afce7025a2061a19e63c700dffc3bec368bd1883decfac353357a9d
+    let tx: UtxoTx = "0100000001740443e82e526cef440ed590d1c43a67f509424134542de092e5ae68721575d60100000049483045022078e86c021003cca23842d4b2862dfdb68d2478a98c08c10dcdffa060e55c72be022100f6a41da12cdc2e350045f4c97feeab76a7c0ab937bd8a9e507293ce6d37c9cc201ffffffff0200f2052a010000001976a91431891996d28cc0214faa3760a765b40846bd035888ac00ba1dd2050000004341049464205950188c29d377eebca6535e0f3699ce4069ecd77ffebfbd0bcf95e3c134cb7d2742d800a12df41413a09ef87a80516353a2f0a280547bb5512dc03da8ac00000000".into();
+    let script_sig = tx.inputs[0].script_sig.clone().into();
+    let expected_pub = Public::Normal("049464205950188c29d377eebca6535e0f3699ce4069ecd77ffebfbd0bcf95e3c134cb7d2742d800a12df41413a09ef87a80516353a2f0a280547bb5512dc03da8".into());
+    let unsigned_tx: TransactionInputSigner = tx.into();
+    let successful_verification =
+        verify_p2pk_input_pubkey(&script_sig, &expected_pub, &unsigned_tx, 0, SignatureVersion::Base, 0).unwrap();
+    assert!(successful_verification);
+
+    // 33-byte (compressed) pubkey example.
+    // https://kmdexplorer.io/tx/07ceb50f9eedc3b820e48dc1e5250f6625115afe4ace3089bfcc66b34f5d4344
+    let tx: UtxoTx = "0400008085202f89013683897bf3bfb1e217663aa9591bd73c9eb105f8c8471e88dbe7152ca7627a19050000004948304502210087100bf4a665ebab3cc6d3472068905bdc6c6def37e432597e78e2ccc4da017a02205b5f0800cabe84bc49b5eb0997926b48dfee3b8ca5a31623ae9506272f8a5cd501ffffffff0288130000000000002321020e46e79a2a8d12b9b5d12c7a91adb4e454edfae43c0a0cb805427d2ac7613fd9ac0000000000000000226a20976bd7ad5596ac3521fd90295e753b1096e4eb90ad9ded1170b2ed81f810df5fc0dbf36752ea42000000000000000000000000".into();
+    let script_sig = tx.inputs[0].script_sig.clone().into();
+    let expected_pub = Public::Compressed("02f9a7b49282885cd03969f1f5478287497bc8edfceee9eac676053c107c5fcdaf".into());
+    let mut unsigned_tx: TransactionInputSigner = tx.into();
+    // For overwintered transactions, the amount must be set, as wel as the consensus branch id.
+    unsigned_tx.inputs[0].amount = 10000;
+    unsigned_tx.consensus_branch_id = 0x76b8_09bb;
+    let successful_verification =
+        verify_p2pk_input_pubkey(&script_sig, &expected_pub, &unsigned_tx, 0, SignatureVersion::Base, 0).unwrap();
+    assert!(successful_verification);
+}
+
 #[test]
 fn test_tx_v_size() {
     // Multiple legacy inputs with P2SH and P2PKH output

mm2src/mm2_bitcoin/keys/src/public.rs

@@ -3,8 +3,8 @@ use crypto::dhash160;
 use hash::{H160, H264, H520};
 use hex::ToHex;
 use secp256k1::{recovery::{RecoverableSignature, RecoveryId},
-                Message as SecpMessage, PublicKey, Signature as SecpSignature};
-use std::{fmt, ops};
+                Error as SecpError, Message as SecpMessage, PublicKey, Signature as SecpSignature};
+use std::{fmt, ops::Deref};
 use {CompactSignature, Error, Message, Signature};
 
 /// Secret public key
@@ -80,9 +80,11 @@ impl Public {
             Public::Normal(_) => None,
         }
     }
+
+    pub fn to_secp256k1_pubkey(&self) -> Result<PublicKey, SecpError> { PublicKey::from_slice(self.deref()) }
 }
 
-impl ops::Deref for Public {
+impl Deref for Public {
     type Target = [u8];
 
     fn deref(&self) -> &Self::Target {