bitlogik / uniblow

UNIversal BLOckchain Wallet
GNU General Public License v3.0
17 stars 11 forks source link

Wrong transaction signatures for Dogecoin (and possibly Bitcoin & Litecoin) #19

Closed Toporin closed 1 year ago

Toporin commented 1 year ago

When performing a Dogecoin transaction (using a LocalFile device), I get the following error:

400 : {"error": "Error validating transaction: Error running script for input 0 referencing 96a541140e1274663df7f21bcb54bf8c891dbccf93128f59af1504d3b34e5929 at 2: Script was NOT verified successfully.."}

As far as I can tell, it is an issue with the signature which is not correct. I tried to debug the error and one possible explanation is that the computation of the tx hash is not done correctly: In DOGEwallet.py:

    def raw_tx(self, amount, fee, to_account):
        hashes_to_sign = self.doge.prepare(to_account, amount, fee)
        tx_signatures = []
        for msg in hashes_to_sign:
            asig = self.current_device.sign(msg) # compute signature based on tx hash ("msg")
            tx_signatures.append(asig)
        return self.doge.send(tx_signatures)

in the method "prepare()":

for i in range(self.leninputs):
            signing_tx = cryptolib.coins.signature_form(
                self.tx, i, script, cryptolib.coins.SIGHASH_ALL
            )
            datahashes.append(cryptolib.coins.bin_txhash(signing_tx, cryptolib.coins.SIGHASH_ALL))

"bin_txhash" is defined in cryptolib.coins.transaction:

def txhash(tx, hashcode=None, wtxid=True):
    if isinstance(tx, str) and re.match("^[0-9a-fA-F]*$", tx):
        tx = changebase(tx, 16, 256)
    if not wtxid and is_segwit(tx):
        tx = serialize(deserialize(tx), include_witness=False)
    if hashcode:
        return sha2(from_string_to_bytes(tx) + encode(int(hashcode), 256, 4)[::-1])
    else:
        return sha2(tx)

def public_txhash(tx, hashcode=None):
    return txhash(tx, hashcode=hashcode, wtxid=False)

def bin_txhash(tx, hashcode=None):
    return txhash(tx, hashcode)

From this code, we can see that only a single SHA256 hash is computed, while Doge (and Bitcoin) requires a double hash.

Using a LocalFile device, tx is signed with:

def sign(self, hashed_msg):
        return self.pvkey.sign(hashed_msg)

which uses ECKeyPair to sign with prehashed data (so no hash is performed during signature). See sign() method in ECKeyPair.py:

if self.curve == "K1" or self.curve == "R1":
            sign_alg = ec.ECDSA(utils.Prehashed(hashes.SHA256()))
            return makeup_sig(self.key_obj.sign(data, sign_alg), self.curve)

A way to correct the issue is simply to compute a second hash before signing. For Dogecoin, adding one line in DogeWallet.py solve the issue for LocalFile:

def raw_tx(self, amount, fee, to_account):
        hashes_to_sign = self.doge.prepare(to_account, amount, fee)
        tx_signatures = []
        for msg in hashes_to_sign:
            # debug 
            msg = sha2(msg) # <= add this line
            # endbug
            asig = self.current_device.sign(msg)
            tx_signatures.append(asig)
        return self.doge.send(tx_signatures)

However, self.current_device.sign(msg) might require different value (single or double hash) depending on the device. Also, this issue might also affect Bitcoin and Litecoin wallets since they use similar signature algorithms. In this case, it may be possible to fix this bug in cryptolib.coins.transaction by computing a double hash in the "txhash()" method.

bitlogik commented 1 year ago

Thanks for your detailed report and the investigation. The dogecoin support is probably the less tested, hence the less reliable of all supported blockchains. Mostly because there's no testnet with simple faucet. For Bitcoin, this is regularly tested, and we think it's OK for Bitcoin. We feel that for some reasons the behavior would be the same between DOGE and BTC, but probably DOGE is different on some points.. We'll investigate and fix this.

bitlogik commented 1 year ago

We traced the probable root cause of this issue to this commit made 10 months ago: 315f2a1 - Split BTC tx double hash in October v2.0.2 This has changed how a Bitcoin transaction is performed, in order to improve device compatibility (wallet device "signer" abstraction), and also simplify transaction building. Sadly, we missed to apply this modification to LTC and DOGE. It was mostly due to DOGE/LTC API issues requiring a change, and the new API endpoint has no testnet access.

bitlogik commented 1 year ago

This issue is fixed by the latest commit 511e222. We'll wait some days before releasing a new version, possibly adding others changes.

Toporin commented 1 year ago

This seems to solve the issue for Dogecoin!