Firaenix / bsv-wasm

BSV stdlib written in Rust and runs in WASM environments
MIT License
70 stars 19 forks source link

`ECDSA::sign_with_deterministic_k_impl` produces wrong `(r, s)` #66

Closed blocksurf closed 12 months ago

blocksurf commented 1 year ago

The reverse_k flag only changes the byte order of the k_digest but not the msg_scalar even though they're the same digest.

let private_key = PrivateKey::from_wif("KzmbmFNo39aF85nxV24MqK3JCat3TYNUVJVaQdzgdSHLwuPFFyMt").unwrap();
let preimage = hex::decode("010000008c8dfd409fbe3bd76c6c074086b89b56e18c31899da98ccd7b05baf769a096ae3bb13029ce7b1f559ef5e747fcac439f1455a2ec7c5f09b72290795e7066504442c4eb085fdcf6c06faed60e1cbfcc852bb04801c70d5b499f122171b14f61af000000001976a914cdc1c584ca737579b470ab4407220d67a294774488ac4086010000000000ffffffff7bc9a0bf72f4ab98d03bd5b1f309976d9f2d30ff8e2c64ad2b3a13456015654b0000000041000000").unwrap();
let sig = ECDSA::sign_with_deterministic_k(&private_key, &preimage, bsv::SigningHash::Sha256d, true).unwrap();
// r - 00b2327c56ced7cf78414c0115e3642525899bba85cc3f63a0fe23ff9c8f1b858b
// s - 5bd5fb8f4147387269503637b43de3620a735665906831a238a9a021de8914b8

If we sign the same hashbuf in bsv.js our r-values match but the s-values don’t.

const privkey =  bsv.PrivateKey.fromWIF("KzmbmFNo39aF85nxV24MqK3JCat3TYNUVJVaQdzgdSHLwuPFFyMt");
const sighashType = bsv.crypto.Signature.SIGHASH_ALL | bsv.crypto.Signature.SIGHASH_FORKID;
const hashbuf = Buffer.from('17cac225a81ac668a63a330b0ad3c7218b94e11f466b44405debd2d572290605', 'hex');
// 'little' to match bsv.Transaction.sighash.sign
const sig = bsv.crypto.ECDSA.sign(hashbuf, privkey, 'little').set({ nhashtype: sighashType });
// r - 00b2327c56ced7cf78414c0115e3642525899bba85cc3f63a0fe23ff9c8f1b858b
// s - 676d53f10b4c8412864d896c07e2c2187203b988904a28a7e7facd96eeccba0e

This makes sense because we only used the k_digest to calculate r (k * G). But the signature proof relies on k, r & the msg_scalar

Since the msg_scalar is backwards we get a totally different value for s than what we should expect.

Fix

We should update this line to reverse the msg_scalar whenever we reverse the k_digest

let msg_scalar = match reverse_endian_k {
  true => <Scalar as Reduce<U256>>::from_le_bytes_reduced(final_digest),
  false => <Scalar as Reduce<U256>>::from_be_bytes_reduced(final_digest),
};

This enables us to generate signatures that are fully backwards compatible with bsv.js & allows us to verify signatures from either library

let sig_le = ECDSA::sign_with_deterministic_k(&private_key, &preimage, bsv::SigningHash::Sha256d, true).unwrap();
// r - 00b2327c56ced7cf78414c0115e3642525899bba85cc3f63a0fe23ff9c8f1b858b
// s - 676d53f10b4c8412864d896c07e2c2187203b988904a28a7e7facd96eeccba0e

let sig_be = ECDSA::sign_with_deterministic_k(&private_key, &preimage, bsv::SigningHash::Sha256d, false).unwrap();
// r - 00f81e04ae4e1be88c9eca52679e397c15cf40dd3678ff2328ef513af406edff38 
// s - 1bc7116d947e934b375c6af4b90c2c029b9bf4a9dfcb3e3a28ed2723cd51b0cf
let sig_le = bsv.crypto.ECDSA.sign(hashbuf, privkey, 'little').set({ nhashtype: sighashType });
// r - 00b2327c56ced7cf78414c0115e3642525899bba85cc3f63a0fe23ff9c8f1b858b
// s - 676d53f10b4c8412864d896c07e2c2187203b988904a28a7e7facd96eeccba0e

// endian flag set to 'big' or undefined 
let sig_be = bsv.crypto.ECDSA.sign(hashbuf, privkey).set({ nhashtype: sighashType });
// r - 00f81e04ae4e1be88c9eca52679e397c15cf40dd3678ff2328ef513af406edff38 
// s - 1bc7116d947e934b375c6af4b90c2c029b9bf4a9dfcb3e3a28ed2723cd51b0cf

My main concern here is backwards compatibility. Since reverse_k is baked into Transaction::sign_impl, this affects all transactions signed using tx.sign

I will submit a PR for this shortly

blocksurf commented 1 year ago

Pull request https://github.com/Firaenix/bsv-wasm/pull/67