Segwit outputs are not spendable by nodes but can be stolen by miners.
grzegorzszczecin opened this issue · 20 comments
See also https://github.com/BTCPrivate/BitcoinPrivate/pull/77 for background
As suggested by that pull request, BTCP nodes currently reject TX with witnesses attached (reject: error parsing message). The recommended method of redemption is to use legacy TX serialization for signing, and pre-pend the scriptSig with <sig> <pubkey>.
My experiments show that such TX will still get rejected with the following cryptic message:
'reject' '\x02tx@+non-mandatory-script-verify-flag (No error)'
#77 --> what a shitty workaround
And very badly explained
Did you try scriptSig: <sig> <pubkey> <what they wrongly call scriptpubkey> (ie 00etc segwit address before hash)?
Will try it on my side with people trying to move segwit BTC coins with https://github.com/Ayms/bitcoin-transactions
See #144 also
What a mess...
@Ayms I have tried all sorts of different combinations...
For calculation of TX hash (for signature), I've tried both the "old" way and the "new (BIP143)" way.
I have also tried to put the script ("\x00\x14" + pubkeyhash160) at the bottom and at the top of the stack. Nothing works for me.
After several different attempts I'm experiencing similar problems as @grzegorzszczecin.
Has #77 Partial SegWit support been confirmed working at all?
For calculation of TX hash (for signature), I've tried both the "old" way and the "new (BIP143)" way.
BIP143 is not supported by this (stupid) fork (why "stupid" because if you fork something that supports segwit then you should support it also unless you have stated the contrary, which is not the case here), I modified my post above since the rendering was not correct
I have also tried to put the script ("\x00\x14" + pubkeyhash160) at the bottom and at the top of the stack. Nothing works for me.
At the bottom, it must work if everything is correct, will try it also
Has #77 Partial SegWit support been confirmed working at all?
Again, no hazard possible, It must work if everything is correct in the tx
@jc23424 can you take a look at #77 regarding the two VerifyWitnessProgram() calls? From my preliminary debugging, these code blocks were never triggered because flags does not have SCRIPT_VERIFY_WITNESS set. As a result we did not clean up the stack, and failed in the CLEANSTACK check:
// The CLEANSTACK check is only performed after potential P2SH evaluation,
// as the non-P2SH evaluation of a P2SH script will obviously not result in
// a clean stack (the P2SH inputs remain).
if ((flags & SCRIPT_VERIFY_CLEANSTACK) != 0) {
// Disallow CLEANSTACK without P2SH, as otherwise a switch CLEANSTACK->P2SH+CLEANSTACK
// would be possible, which is not a softfork (and P2SH should be one).
assert((flags & SCRIPT_VERIFY_P2SH) != 0);
if (stack.size() != 1) {
return set_error(serror, SCRIPT_ERR_CLEANSTACK);
}
}
If VerifyWitnessProgram() is never called because the SCRIPT_VERIFY_WITNESS flag is not set, that would mean that stack.resize(1); is never called, leaving a non-clean stack and thus is non-standard.
But if SCRIPT_VERIFY_WITNESS is not set, it also means that the Witness program is never evaluated/executed(?).
Miners could therefore steal P2WPKH in P2SH and P2WSH in P2SH outputs if they know the redeem script.
Am I missing something here?
AFAICT there is no activation of the SegWit flag similar to Bitcoin Core's validation.cpp@1750.
Bitcoin Private could have it enabled by default quite frankly (or remove it altogether) as it's not a Bitcoin hardfork, only a UTXO merge with ZClassic.
It seems like fixing this bug requires at least a softfork.
@hsjoberg are you sure that miners can bypass the SCRIPT_VERIFY_CLEANSTACK check? If that check is not part of the consensus rule then this is a very serious problem! Miners can figure out the redeem script easily if such P2WPKH-P2SH and P2WSH-P2SH outputs are already spent on the Bitcoin blockchain. BTCPrivate.org actually recommends "moving" the output on the Bitcoin blockchain before redeeming the corresponding BTCP balance.
are you sure that miners can bypass the SCRIPT_VERIFY_CLEANSTACK check? If that check is not part of the consensus rule then this is a very serious problem!
@grzegorzszczecin AFAICT the cleanstack rule is not consensus critical, if you have a non-clean stack, your transaction is just considered non-standard.
Miners can figure out the redeem script easily if such P2WPKH-P2SH and P2WSH-P2SH outputs are already spent on the Bitcoin blockchain.
Even worse, normal P2WPKH should be directly vulnerable, right now!!
Unfortunately I don't have all my time to investigate this right now
But what says #77 cannot be correct or si wrongly explained, because p2wpkh and p2wsh should end up with the same treatment, which at the end is to validate the segwit address
Then, maybe wrong here but logically it should be:
<sig> <pubkey> <scriptpubkey as a normal p2pkh or p2sh><what they wrongly call scriptpubkey> (ie 00etc segwit address before hash)
Unless they implemented something to detect segwit (via 00etc) and force the sig-pubkey check adding the scriptpubkey by themselves (from 00etc), I think I would have done something like this because they have all the required info and then don't need to ask the user, and the direct result here is to check the nested info and then the address (a bit like validating twice a transaction, or two)
@Ayms I think that is slightly off topic, but no, in P2WPKH/P2WSH in P2SH I believe that it would successfully interpret that it is a witness program, redeem script would be popped from the stack and checked with IsWitnessProgram(), just as it would in Bitcoin (scriptSig is used for the redeem script in Bitcoin as well, for P2PWKH/P2WSH in P2SH).
Check interpreter.cpp@1289 and interpreter.cpp@1305.
The problem here is that we never even get to that code, because the flag SCRIPT_VERIFY_WITNESS is never ever set.
Thank you - I believe you are correct - fix to consensus rules just released. Will add relevant script flags to policy in a follow up
@jc23424 I appreciate the fast response on this issue.
Note that this will cause a softfork which could (and probably will) lead to chainsplits.
I guess it will eventually resolve itself once a majority of the miners are on the new version, but those miners who haven't upgraded could lose their blocks.
Thank you for finding it!
Yes - people are coordinating with miners to coordinate upgrades in the safest possible manner given the circumstances
Quick update: this change is now locked in on a majority of hashrate
There were no invalid (with this change) blocks observed in before the change was adopted by a majority of the miners
@grzegorzszczecin, @Ayms and @hsjoberg please send an email to security@btcprivate.org
We would like to reward you for finding this critical vulnerability and bringing it to our attention.
The email should be from an address matching the one on your github profile. Send along a Bitcoin Private address. Thank you again.
Kind attention, but my participation here to the resolution of the issue (for a fork that I am still questioning until everything is square) is quite small, then the rewards must go to @grzegorzszczecin and @hsjoberg
Are you sure this is actually fixed? In the latest version I simply set STANDARD_SCRIPT_VERIFY_FLAGS to MANDATORY_SCRIPT_VERIFY_FLAGS and got a proof of concept "theft" transaction accepted. I don't have computing power enough to mine a block, but as far as I see there's nothing stopping a powerful miner from doing the same and the network would then accept the block since blocks only require MANDATORY_SCRIPT_VERIFY_FLAGS to be checked.
Hi - thanks for checking in - but afaik {STANDARD, MANDATORY}_SCRIPT_VERIFY_FLAGS are just used for mempool acceptance. You are correct that miners are free to change these in their own mempools if they wish. But in order for the network to accept blocks they mine they have to pass the script verify flags here: https://github.com/BTCPrivate/BitcoinPrivate/blob/master/src/main.cpp#L2146
where witness is set and mandatory.
Were you able to mine a block with those transactions, or just have them accepted to your mempool (to test you can run with -regtest where there's a reduced pow and you don't need hashpower)?
Yeah, sorry - that check should be enough to exclude those transactions from blocks. I don't have the power to do a test mine so I only tested mempool acceptance - didn't know there was separate code for the mandatory block flags.