LedgerHQ / ledgerjs

⛔️ MOVED to monorepo "ledger-live"
https://github.com/LedgerHQ/ledger-live
Apache License 2.0
574 stars 379 forks source link

signP2SHTransaction broken on hw-btc-app #488

Closed ambush276 closed 4 years ago

ambush276 commented 4 years ago

I have been debugging this for a while now and believe there is a pretty serious bug in this function. Please note this is from testing code on multiple ledgers, multiple computers, and still the same consistent result (using a variety of versions of hw-btc-app and hw-transport-node-hid).

The main issue is that the signature that is returned is invalid. I am testing it against ecdsa tools and can confirm the signature that is returned is not valid.

What adds to the level of bizarreness is that when i do signing on bitcoin testnet... that signature returns correctly. So this issue is very specific to mainnet btc.

The code below i use for both btc mainnet and btc testnet, and the inputs are in the same format as well. The only difference is that the mainnet signing is returning an invalid signature and the testnet signing is returning a correct signature.

I have even created transactions on mainnet and testnet using the same public keys (p2sh) in the same order and the testnet will correctly sign and the mainnet will return an incorrect signature....

I also want to note that for the past 18 months this has not been an issue but recently became an issue as of feb 2019.

Nano ledger S:

version(of libs): 5.12.0 bitcoin app version (on ledger): 1.3.13 bitcoin tesnet app version (on ledger): 1.3.13 mcu: 1.7 Secure Element: 1.5.5

const regeneratorRuntime = require("regenerator-runtime");
var Transport = require("@ledgerhq/hw-transport-node-hid");
var AppBtc  = require("@ledgerhq/hw-app-btc");

const {serializeTransactionOutputs } = require("@ledgerhq/hw-app-btc/lib/serializeTransaction");

// (data being set here)....
//keyset gotten here ...

const getBtcAddress = async () => {

  const transport = await Transport.default.create();
  const btc = new AppBtc.default(transport);
  const tx1 = await btc.splitTransaction(rawtx);
  const tx2 = await btc.splitTransaction(txin);
  const outputscript = serializeTransactionOutputs(tx1).toString("hex");
  let input = [];
  input.push(tx2);
  input.push(vout);
  input.push(redeem);
  console.log(outputscript);
  await btc.signP2SHTransaction({
    inputs: [input],
    associatedKeysets: keyset,
    outputScriptHex: outputscript
  }).then(sig => console.log(sig)).catch(err => console.log(err));
};

getBtcAddress().then(a => a).catch(error => console.log(error +"\n Got Error"));
ambush276 commented 4 years ago

nobody has any ideas? The fact that bitcoin testnet is working fine but mainnet is not leads me to believe it is some issue in the bitcoin app on the ledger.

ambush276 commented 4 years ago

is this a broken function? i have confirmed with other users this is not working.

ambush276 commented 4 years ago

ok. I used a BIP 39 tool to recover the keys and addresses associated to my 24 word phrase. After doing so i extracted the private keys from the correct keyset path, and signed the transaction manually. This worked with the same data i am feeding this function. This means that there is something seriously wrong either in this function for btc or in the btc app on the ledger. The btc testnet app works perfectly fine.

ambush276 commented 4 years ago

nobody has any ideas... this seems like a pretty serious bug.

ambush276 commented 4 years ago

still no comment. This seems like a very serious bug.

ambush276 commented 4 years ago

bump..

ambush276 commented 4 years ago

bump..

ambush276 commented 4 years ago

bump. this is still broken

ambush276 commented 4 years ago

issue was related to splitTransaction. The underlying api had isSegwit bool in a different position in older versions (pre v 3.0). Before p2sh transactions did not need to set this variable in splitTransaction. Some later version requires this to be set to 'true' for p2sh transactions (when earlier you did not need to specify this bool at all).