From 0ddcf690bf48075927f9d7596c62ddb41e79b2e0 Mon Sep 17 00:00:00 2001 From: Bitflash Date: Tue, 28 Jul 2026 13:58:43 -0300 Subject: Security: anyone can spend anyone's coins, plus three more consensus bugs (#12) * script: fix OP_RETURN spend bypass, disable unbounded opcodes OP_RETURN let anyone spend anyone else's outputs. VerifySignature evaluates scriptSig and scriptPubKey as one concatenated script. OP_RETURN was implemented as 'pc = pend', which jumps past everything that follows, and EvalScript ends with CastToBool(stack.back()). So a scriptSig of skipped the entire scriptPubKey -- OP_CHECKSIG included -- and the final check read the attacker's own OP_TRUE. No signature required, any output spendable. This is the 0.1.0 behaviour Bitcoin fixed in 0.3.5. OP_RETURN now fails the script outright. Also disables the opcodes Bitcoin removed for being unbounded, and caps pushed elements at 520 bytes: OP_CAT grows a stack element with no limit -- OP_DUP OP_CAT repeated doubles it each time, so a script that fits in a transaction exhausts memory on every node that validates it. OP_LSHIFT shifts by an attacker-supplied amount with no ceiling, same effect. There were no element, stack or opcode-count limits of any kind. Nothing on this chain uses these opcodes -- all outputs are standard pay-to-pubkey-hash or coinbase pay-to-pubkey -- so no existing transaction changes validity. * main: reject blocks containing a duplicate transaction (CVE-2012-2459) BuildMerkleTree duplicates the trailing hash whenever a tree level has an odd width. Appending a copy of the last transaction therefore produces a block whose merkle root -- and whose block hash -- match a legitimate block exactly. A node handed the forgery rejects it (the duplicated inputs are already spent) and remembers that hash as bad. When the genuine block arrives carrying the same hash, it is refused too, and the node stops following the real chain. CheckBlock now rejects any block that carries the same transaction twice, which is what the mutation requires. * main: halve the subsidy on the block's height, not the node's tip GetBlockValue derived the halving from nBestHeight -- the validating node's own chain tip -- rather than from the height of the block being valued. The subsidy a block may pay is a property of that block, so reading it off local state makes validity depend on how far the node has synced: across a halving boundary a node still catching up would allow 50 BTF where a synced node allows 25, and the two would follow different chains. Reorgs have the same problem in miniature. Harmless today (every node computes nHeight/210000 == 0 at height ~2400) and it changes no existing block's validity, but it splits the network at height 210000. The height is now passed in explicitly: pindex->nHeight when connecting, tip + 1 when mining or building pool work. --- src/main.cpp | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index aff7bab..a7298ea 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -729,12 +729,16 @@ uint256 GetOrphanRoot(const CBlock* pblock) return pblock->GetHash(); } -int64 CBlock::GetBlockValue(int64 nFees) const +int64 CBlock::GetBlockValue(int nHeight, int64 nFees) const { int64 nSubsidy = 50 * COIN; - // Subsidy is cut in half every 4 years - nSubsidy >>= (nBestHeight / 210000); + // Halve on the height of this block, never on nBestHeight. The subsidy a + // block is allowed to pay is a property of that block; deriving it from + // the validating node's own tip makes the answer depend on how far that + // node happens to have synced, so two nodes at different heights would + // disagree about the same block across a halving boundary. + nSubsidy >>= (nHeight / 210000); return nSubsidy + nFees; } @@ -1011,7 +1015,7 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex) return false; } - if (vtx[0].GetValueOut() > GetBlockValue(nFees)) + if (vtx[0].GetValueOut() > GetBlockValue(pindex->nHeight, nFees)) return false; // Update block index on disk without changing it in memory. @@ -1237,6 +1241,22 @@ bool CBlock::CheckBlock() const if (!tx.CheckTransaction()) return error("CheckBlock() : CheckTransaction failed"); + // Reject blocks carrying the same transaction twice (CVE-2012-2459). + // + // BuildMerkleTree duplicates the last hash when a level has an odd width, + // so appending a copy of the trailing transaction(s) yields a block with + // an identical merkle root -- and therefore an identical block hash -- to + // a legitimate one. The forgery fails validation, the hash gets recorded + // as bad, and the real block is then refused because it hashes the same. + set setTxHashes; + foreach(const CTransaction& tx, vtx) + { + uint256 hashTx = tx.GetHash(); + if (setTxHashes.count(hashTx)) + return error("CheckBlock() : duplicate transaction in block"); + setTxHashes.insert(hashTx); + } + // Check proof of work matches claimed amount (memory-hard RandomX PoW) if (CBigNum().SetCompact(nBits) > bnProofOfWorkLimit) return error("CheckBlock() : nBits below minimum work"); @@ -2803,7 +2823,7 @@ bool BitcoinMiner() } } pblock->nBits = nBits; - pblock->vtx[0].vout[0].nValue = pblock->GetBlockValue(nFees); + pblock->vtx[0].vout[0].nValue = pblock->GetBlockValue(nBestHeight + 1, nFees); if (LogAcceptsCategory("net")) printf("\n\nRunning BitcoinMiner with %d transactions in block\n", pblock->vtx.size()); -- cgit v1.2.3