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 <OP_TRUE OP_RETURN> 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 -- <x> 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.

5 files changed, 60 insertions, 9 deletions

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<uint256> 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());
 
 
diff --git a/src/main.h b/src/main.h
index 8bb8609..533974c 100644
--- a/src/main.h
+++ b/src/main.h
@@ -1073,7 +1073,7 @@ public:
     }
 
 
-    int64 GetBlockValue(int64 nFees) const;
+    int64 GetBlockValue(int nHeight, int64 nFees) const;
     bool DisconnectBlock(CTxDB& txdb, CBlockIndex* pindex);
     bool ConnectBlock(CTxDB& txdb, CBlockIndex* pindex);
     bool ReadFromDisk(const CBlockIndex* blockindex, bool fReadTransactions);
diff --git a/src/rpc.cpp b/src/rpc.cpp
index 1b529e8..3b2dae9 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -182,7 +182,7 @@ static bool RebuildJob()
             }
         }
 
-        block.vtx[0].vout[0].nValue = block.GetBlockValue(0);
+        block.vtx[0].vout[0].nValue = block.GetBlockValue(pindexPrev->nHeight + 1, 0);
         block.hashPrevBlock  = pindexPrev->GetBlockHash();
         block.hashMerkleRoot = block.BuildMerkleTree();
         block.nTime = max((unsigned int)(pindexPrev->GetMedianTimePast() + 1),
diff --git a/src/script.cpp b/src/script.cpp
index d2111ed..44eed93 100644
--- a/src/script.cpp
+++ b/src/script.cpp
@@ -67,6 +67,24 @@ bool EvalScript(const CScript& script, const CTransaction& txTo, unsigned int nI
         if (!script.GetOp(pc, opcode, vchPushValue))
             return false;
 
+        if (vchPushValue.size() > MAX_SCRIPT_ELEMENT_SIZE)
+            return false;
+
+        // Disabled opcodes. Checked before the fExec test, so they invalidate
+        // the script even inside a branch that is never taken.
+        //
+        // These are unbounded: OP_CAT grows a stack element without limit
+        // (<x> OP_DUP OP_CAT repeated doubles it each time, so a tiny script
+        // exhausts memory), and OP_LSHIFT shifts by an attacker-supplied
+        // amount with no ceiling. Both let any transaction take down every
+        // node that validates it. Bitcoin removed the whole group for this.
+        if (opcode == OP_CAT   || opcode == OP_SUBSTR || opcode == OP_LEFT  ||
+            opcode == OP_RIGHT || opcode == OP_INVERT || opcode == OP_AND   ||
+            opcode == OP_OR    || opcode == OP_XOR    || opcode == OP_2MUL  ||
+            opcode == OP_2DIV  || opcode == OP_MUL    || opcode == OP_DIV   ||
+            opcode == OP_MOD   || opcode == OP_LSHIFT || opcode == OP_RSHIFT)
+            return false;
+
         if (fExec && opcode <= OP_PUSHDATA4)
             stack.push_back(vchPushValue);
         else if (fExec || (OP_IF <= opcode && opcode <= OP_ENDIF))
@@ -169,9 +187,18 @@ bool EvalScript(const CScript& script, const CTransaction& txTo, unsigned int nI
 
             case OP_RETURN:
             {
-                pc = pend;
+                // Must fail the script outright, never just stop executing it.
+                //
+                // scriptSig and scriptPubKey are evaluated as one concatenated
+                // script (see VerifySignature). The 0.1.0 behaviour here was
+                // "pc = pend", which jumps past everything that follows — so a
+                // scriptSig of <OP_TRUE OP_RETURN> skipped the entire
+                // scriptPubKey, including OP_CHECKSIG, and the final
+                // CastToBool(stack.back()) saw the attacker's own OP_TRUE.
+                // That let anyone spend anyone else's outputs without a
+                // signature. Bitcoin closed this in 0.3.5.
+                return false;
             }
-            break;
 
 
             //
diff --git a/src/script.h b/src/script.h
index d52d06b..27c1b13 100644
--- a/src/script.h
+++ b/src/script.h
@@ -4,6 +4,10 @@
 
 class CTransaction;
 
+// Largest byte vector a script may push onto the stack. Without a ceiling,
+// script evaluation allocates whatever the transaction asks it to.
+static const unsigned int MAX_SCRIPT_ELEMENT_SIZE = 520;
+
 enum
 {
     SIGHASH_ALL = 1,