[Script] Fix "split P2CS" vulnerability

src/script/interpreter.cpp

@@ -962,7 +962,25 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
 
                 case OP_CHECKCOLDSTAKEVERIFY:
                 {
-                    // check it is used in a valid cold stake transaction.
+                    if (g_IsV6Active) {
+                        // the stack can contain only <sig> <pk> <pkh> at this point
+                        if ((int)stack.size() != 3) {
+                            return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
+                        }
+                        // check pubkey/signature encoding
+                        valtype& vchSig    = stacktop(-3);
+                        valtype& vchPubKey = stacktop(-2);
+                        if (!CheckSignatureEncoding(vchSig, flags, serror) ||
+                                !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
+                            // serror is set
+                            return false;
+                        }
+                        // check hash size
+                        valtype& vchPubKeyHash = stacktop(-1);
+                        if ((int)vchPubKeyHash.size() != 20) {
+                            return set_error(serror, SCRIPT_ERR_SCRIPT_SIZE);
+                        }
+                    }
                     if(!checker.CheckColdStake(script)) {
                         return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY);
                     }

src/script/script.cpp

@@ -8,6 +8,8 @@
 #include "tinyformat.h"
 #include "utilstrencodings.h"
 
+#include <atomic>
+
 
 const char* GetOpName(opcodetype opcode)
 {
@@ -223,14 +225,22 @@ bool CScript::IsPayToScriptHash() const
             (*this)[22] == OP_EQUAL);
 }
 
+// contextual flag to guard the new rules for P2CS.
+// can be removed once v6 enforcement is activated.
+std::atomic<bool> g_IsV6Active{false};
+
 bool CScript::IsPayToColdStaking() const
 {
-    // Extra-fast test for pay-to-cold-staking CScripts:
     return (this->size() == 51 &&
+            (!g_IsV6Active || (*this)[0] == OP_DUP) &&
+            (!g_IsV6Active || (*this)[1] == OP_HASH160) &&
             (*this)[2] == OP_ROT &&
+            (!g_IsV6Active || (*this)[3] == OP_IF) &&
             (*this)[4] == OP_CHECKCOLDSTAKEVERIFY &&
             (*this)[5] == 0x14 &&
+            (!g_IsV6Active || (*this)[26] == OP_ELSE) &&
             (*this)[27] == 0x14 &&
+            (!g_IsV6Active || (*this)[48] == OP_ENDIF) &&
             (*this)[49] == OP_EQUALVERIFY &&
             (*this)[50] == OP_CHECKSIG);
 }

src/script/script.h

@@ -661,4 +661,8 @@ class CScript : public CScriptBase
     size_t DynamicMemoryUsage() const;
 };
 
+// contextual flag to guard the new rules for P2CS.
+// can be removed once v6 enforcement is activated.
+extern std::atomic<bool> g_IsV6Active;
+
 #endif // BITCOIN_SCRIPT_SCRIPT_H

src/test/script_P2CS_tests.cpp

@@ -6,10 +6,12 @@
 #include "base58.h"
 #include "key.h"
 #include "policy/policy.h"
+#include "wallet/test/wallet_test_fixture.h"
+#include "wallet/wallet.h"
 
 #include <boost/test/unit_test.hpp>
 
-BOOST_FIXTURE_TEST_SUITE(script_P2CS_tests, TestingSetup)
+BOOST_FIXTURE_TEST_SUITE(script_P2CS_tests, WalletTestingSetup)
 
 void CheckValidKeyId(const CTxDestination& dest, const CKeyID& expectedKey)
 {
@@ -197,4 +199,105 @@ BOOST_AUTO_TEST_CASE(coldstake_script)
     BOOST_CHECK(CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err));
 }
 
+// Check that it's not possible to "fake" a P2CS script for the owner by splitting the locking
+// and unlocking parts. This particular script can be spent by any key, with a
+// unlocking script composed like: <sig> <pk> <DUP> <HASH160> <pkh>
+static CScript GetFakeLockingScript(const CKeyID staker, const CKeyID& owner)
+{
+    CScript script;
+    script << opcodetype(0x2F) << opcodetype(0x01) << OP_ROT <<
+            OP_IF << OP_CHECKCOLDSTAKEVERIFY << ToByteVector(staker) <<
+            OP_ELSE << ToByteVector(owner) << OP_DROP <<
+            OP_EQUALVERIFY << OP_CHECKSIG;
+
+    return script;
+}
+
+void FakeUnlockColdStake(CMutableTransaction& tx, const CScript& prevScript, const CKey& key)
+{
+    // sign the first input
+    tx.vin[0].scriptSig.clear();
+    const CTransaction _tx(tx);
+    SigVersion sv = _tx.GetRequiredSigVersion();
+    const uint256& hash = SignatureHash(prevScript, _tx, 0, SIGHASH_ALL, amtIn, sv);
+    std::vector<unsigned char> vchSig;
+    BOOST_CHECK(key.Sign(hash, vchSig));
+    vchSig.push_back((unsigned char)SIGHASH_ALL);
+    tx.vin[0].scriptSig << vchSig << ToByteVector(key.GetPubKey()) << OP_DUP << OP_HASH160 << ToByteVector(key.GetPubKey().GetID());
+}
+
+static void setupWallet(CWallet& wallet)
+{
+    wallet.SetMinVersion(FEATURE_SAPLING);
+    wallet.SetupSPKM(false);
+}
+
+BOOST_AUTO_TEST_CASE(fake_script_test)
+{
+    BOOST_ASSERT(!g_IsV6Active);
+
+    CWallet& wallet = *pwalletMain;
+    LOCK(wallet.cs_wallet);
+    setupWallet(wallet);
+    CKey stakerKey;         // dummy staker key (not in the wallet)
+    stakerKey.MakeNewKey(true);
+    CKeyID stakerId = stakerKey.GetPubKey().GetID();
+    CPubKey ownerPubKey;
+    BOOST_ASSERT(wallet.GetKeyFromPool(ownerPubKey));
+    const CKeyID& ownerId = ownerPubKey.GetID();
+    CKey ownerKey;          // owner key (in the wallet)
+    BOOST_ASSERT(wallet.GetKey(ownerId, ownerKey));
+
+    const CScript& scriptP2CS = GetFakeLockingScript(stakerId, ownerId);
+
+    // Create prev transaction:
+    // It has two outputs. The first one is spent before v6.
+    // The second one is tested after v6 enforcement.
+    CMutableTransaction txFrom;
+    txFrom.vout.resize(2);
+    for (size_t i = 0; i < 2; i++) {
+        txFrom.vout[i].nValue = amtIn;
+        txFrom.vout[i].scriptPubKey = scriptP2CS;
+    }
+
+    // passes IsPayToColdStaking
+    BOOST_CHECK(scriptP2CS.IsPayToColdStaking());
+
+    // the output amount is credited to the owner wallet
+    wallet.AddToWallet({&wallet, MakeTransactionRef(CTransaction(txFrom))});
+    BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), 2 * amtIn);
+
+    // create spend tx
+    CMutableTransaction tx;
+    tx.vin.resize(1);
+    tx.vout.resize(1);
+    tx.vin[0].prevout.n = 0;
+    tx.vin[0].prevout.hash = txFrom.GetHash();
+    tx.vout[0].nValue = amtIn - 10000;
+    tx.vout[0].scriptPubKey = GetScriptForDestination(stakerId);
+
+    // it cannot be spent with the owner key, using the P2CS unlocking script
+    SignColdStake(tx, 0, scriptP2CS, ownerKey, false);
+    ScriptError err = SCRIPT_ERR_OK;
+    BOOST_CHECK(!CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err));
+    BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_EQUALVERIFY, ScriptErrorString(err));
+
+    // ... but it can be spent by the staker (or any) key, with the fake unlocking script
+    FakeUnlockColdStake(tx, scriptP2CS, stakerKey);
+    if (!CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err)) {
+        BOOST_ERROR(strprintf("P2CS verification failed: %s", ScriptErrorString(err)));
+    }
+    wallet.AddToWallet({&wallet, MakeTransactionRef(CTransaction(tx))});
+    BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), amtIn);
+
+    // Now let's activate v6
+    g_IsV6Active = true;
+
+    // it does NOT pass IsPayToColdStaking
+    BOOST_CHECK_MESSAGE(!scriptP2CS.IsPayToColdStaking(), "Fake script passes as P2CS");
+
+    // the output amount is NOT credited to the owner wallet
+    BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), 0);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

@@ -1922,6 +1922,7 @@ void static UpdateTip(CBlockIndex* pindexNew)
 {
     AssertLockHeld(cs_main);
     chainActive.SetTip(pindexNew);
+    g_IsV6Active = Params().GetConsensus().NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_V6_0);
 
     // New best block
     mempool.AddTransactionsUpdated(1);
@@ -3660,6 +3661,9 @@ bool LoadChainTip(const CChainParams& chainparams)
 
     const CBlockIndex* pChainTip = chainActive.Tip();
 
+    // initial global flag update
+    g_IsV6Active = Params().GetConsensus().NetworkUpgradeActive(pChainTip->nHeight, Consensus::UPGRADE_V5_0);
+
     LogPrintf("Loaded best chain: hashBestChain=%s height=%d date=%s progress=%f\n",
             pChainTip->GetBlockHash().GetHex(), pChainTip->nHeight,
             FormatISO8601DateTime(pChainTip->GetBlockTime()),