[Script][BUG] Fix signature malleability for t inputs in Sapling txes

src/kernel.cpp

@@ -186,7 +186,7 @@ bool CheckProofOfStake(const CBlock& block, std::string& strError, const CBlockI
     const CTxIn& txin = tx->vin[0];
     ScriptError serror;
     if (!VerifyScript(txin.scriptSig, stakePrevout.scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS,
-             TransactionSignatureChecker(tx.get(), 0, stakePrevout.nValue), &serror)) {
+             TransactionSignatureChecker(tx.get(), 0, stakePrevout.nValue), tx->GetRequiredSigVersion(), &serror)) {
         strError = strprintf("signature fails: %s", serror ? ScriptErrorString(serror) : "");
         return false;
     }

src/pivx-tx.cpp

@@ -448,12 +448,14 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
         const CAmount& amount = coin.out.nValue;
 
         SignatureData sigdata;
+        SigVersion sigversion =  mergedTx.GetRequiredSigVersion();
         // Only sign SIGHASH_SINGLE if there's a corresponding output:
         if (!fHashSingle || (i < mergedTx.vout.size()))
             ProduceSignature(
                     MutableTransactionSignatureCreator(&keystore, &mergedTx, i, amount, nHashType),
                     prevPubKey,
                     sigdata,
+                    sigversion,
                     false // no cold stake
             );
 
@@ -462,7 +464,8 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
             sigdata = CombineSignatures(prevPubKey, MutableTransactionSignatureChecker(&mergedTx, i, amount), sigdata, DataFromTransaction(txv, i));
         }
         UpdateTransaction(mergedTx, i, sigdata);
-        if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker(&mergedTx, i, amount)))
+        if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS,
+                MutableTransactionSignatureChecker(&mergedTx, i, amount), sigversion))
             fComplete = false;
     }
 

src/policy/policy.cpp

@@ -215,7 +215,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
         // IsStandard() will have already returned false
         // and this method isn't called.
         std::vector<std::vector<unsigned char> > stack;
-        if (!EvalScript(stack, tx.vin[i].scriptSig, false, BaseSignatureChecker(), SIGVERSION_BASE))
+        if (!EvalScript(stack, tx.vin[i].scriptSig, false, BaseSignatureChecker(), tx.GetRequiredSigVersion()))
             return false;
 
         if (whichType == TX_SCRIPTHASH) {

src/primitives/transaction.h

@@ -21,6 +21,12 @@
 
 class CTransaction;
 
+enum SigVersion
+{
+    SIGVERSION_BASE = 0,
+    SIGVERSION_SAPLING = 1,
+};
+
 // contextual flag to guard the serialization for v5 upgrade.
 // can be removed once v5 enforcement is activated.
 extern std::atomic<bool> g_IsSaplingActive;
@@ -322,6 +328,12 @@ class CTransaction
         return isSaplingVersion() && nType != TxType::NORMAL && hasExtraPayload();
     }
 
+    // Ensure that special and sapling fields are signed
+    SigVersion GetRequiredSigVersion() const
+    {
+        return isSaplingVersion() ? SIGVERSION_SAPLING : SIGVERSION_BASE;
+    }
+
     /*
      * Context for the two methods below:
      * We can think of the Sapling shielded part of the transaction as an input
@@ -421,6 +433,17 @@ struct CMutableTransaction
      * fly, as opposed to GetHash() in CTransaction, which uses a cached result.
      */
     uint256 GetHash() const;
+
+    bool isSaplingVersion() const
+    {
+        return nVersion >= CTransaction::TxVersion::SAPLING;
+    }
+
+    // Ensure that special and sapling fields are signed
+    SigVersion GetRequiredSigVersion() const
+    {
+        return isSaplingVersion() ? SIGVERSION_SAPLING : SIGVERSION_BASE;
+    }
 };
 
 typedef std::shared_ptr<const CTransaction> CTransactionRef;

src/rpc/rawtransaction.cpp

@@ -799,9 +799,11 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
         }
 
         SignatureData sigdata;
+        SigVersion sigversion = mergedTx.GetRequiredSigVersion();
         // Only sign SIGHASH_SINGLE if there's a corresponding output:
         if (!fHashSingle || (i < mergedTx.vout.size()))
-            ProduceSignature(MutableTransactionSignatureCreator(&keystore, &mergedTx, i, amount, nHashType), prevPubKey, sigdata, fColdStake);
+            ProduceSignature(MutableTransactionSignatureCreator(&keystore, &mergedTx, i, amount, nHashType),
+                    prevPubKey, sigdata, sigversion, fColdStake);
 
         // ... and merge in other signatures:
         for (const CMutableTransaction& txv : txVariants) {
@@ -811,7 +813,8 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
         UpdateTransaction(mergedTx, i, sigdata);
 
         ScriptError serror = SCRIPT_ERR_OK;
-        if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount), &serror)) {
+        if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS,
+                TransactionSignatureChecker(&txConst, i, amount), sigversion, &serror)) {
             TxInErrorToJSON(txin, vErrors, ScriptErrorString(serror));
         }
     }

src/sapling/saplingscriptpubkeyman.cpp

@@ -671,6 +671,9 @@ isminetype SaplingScriptPubKeyMan::IsMine(const CWalletTx& wtx, const SaplingOut
 
 CAmount SaplingScriptPubKeyMan::GetCredit(const CWalletTx& tx, const isminefilter& filter, const bool fUnspent) const
 {
+    if (!tx.IsShieldedTx() || tx.sapData->vShieldedOutput.empty()) {
+        return 0;
+    }
     CAmount nCredit = 0;
     for (int i = 0; i < (int) tx.sapData->vShieldedOutput.size(); ++i) {
         SaplingOutPoint op(tx.GetHash(), i);
@@ -697,6 +700,9 @@ CAmount SaplingScriptPubKeyMan::GetCredit(const CWalletTx& tx, const isminefilte
 
 CAmount SaplingScriptPubKeyMan::GetDebit(const CTransaction& tx, const isminefilter& filter) const
 {
+    if (!tx.IsShieldedTx() || tx.sapData->vShieldedSpend.empty()) {
+        return 0;
+    }
     CAmount nDebit = 0;
     for (const SpendDescription& spend : tx.sapData->vShieldedSpend) {
         const auto &it = mapSaplingNullifiersToNotes.find(spend.nullifier);
@@ -720,7 +726,7 @@ CAmount SaplingScriptPubKeyMan::GetDebit(const CTransaction& tx, const isminefil
 
 CAmount SaplingScriptPubKeyMan::GetShieldedChange(const CWalletTx& wtx) const
 {
-    if (!wtx.isSaplingVersion() || wtx.sapData->vShieldedOutput.empty()) {
+    if (!wtx.IsShieldedTx() || wtx.sapData->vShieldedOutput.empty()) {
         return 0;
     }
     const uint256& txHash = wtx.GetHash();

src/sapling/transaction_builder.cpp

@@ -364,7 +364,7 @@ TransactionBuilderResult TransactionBuilder::Build()
         bool signSuccess = ProduceSignature(
             TransactionSignatureCreator(
                 keystore, &txNewConst, nIn, tIn.value, SIGHASH_ALL),
-            tIn.scriptPubKey, sigdata, false);
+            tIn.scriptPubKey, sigdata, SIGVERSION_SAPLING, false);
 
         if (!signSuccess) {
             return TransactionBuilderResult("Failed to sign transaction");

src/script/bitcoinconsensus.cpp

@@ -88,7 +88,8 @@ int bitcoinconsensus_verify_script(const unsigned char *scriptPubKey, unsigned i
          // Regardless of the verification result, the tx did not error.
          set_error(err, bitcoinconsensus_ERR_OK);
 
-        return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), flags, TransactionSignatureChecker(&tx, nIn), NULL);
+        return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen),
+                flags, TransactionSignatureChecker(&tx, nIn), tx.GetRequiredSigVersion(), NULL);
     } catch (const std::exception&) {
         return set_error(err, bitcoinconsensus_ERR_TX_DESERIALIZE); // Error deserializing
     }

src/script/interpreter.cpp

@@ -1170,7 +1170,10 @@ uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsig
         return UINT256_ONE;
     }
 
-    // currently: sigversion_sapling is disabled everywhere.
+    if (txTo.isSaplingVersion() && sigversion != SIGVERSION_SAPLING) {
+        throw std::runtime_error("SignatureHash in Sapling tx with wrong sigversion " + std::to_string(sigversion));
+    }
+
     if (sigversion == SIGVERSION_SAPLING) {
 
         uint256 hashPrevouts;
@@ -1292,7 +1295,7 @@ bool TransactionSignatureChecker::CheckSig(const std::vector<unsigned char>& vch
     int nHashType = vchSig.back();
     vchSig.pop_back();
 
-    uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->precomTxData);
+    const uint256& sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->precomTxData);
 
     if (!VerifySignature(vchSig, pubkey, sighash))
         return false;
@@ -1337,7 +1340,7 @@ bool TransactionSignatureChecker::CheckLockTime(const CScriptNum& nLockTime) con
 }
 
 
-bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
+bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* serror)
 {
     set_error(serror, SCRIPT_ERR_UNKNOWN_ERROR);
 
@@ -1346,12 +1349,12 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigne
     }
 
     std::vector<std::vector<unsigned char> > stack, stackCopy;
-    if (!EvalScript(stack, scriptSig, flags, checker, SIGVERSION_BASE, serror))
+    if (!EvalScript(stack, scriptSig, flags, checker, sigversion, serror))
         // serror is set
         return false;
     if (flags & SCRIPT_VERIFY_P2SH)
         stackCopy = stack;
-    if (!EvalScript(stack, scriptPubKey, flags, checker, SIGVERSION_BASE, serror))
+    if (!EvalScript(stack, scriptPubKey, flags, checker, sigversion, serror))
         // serror is set
         return false;
     if (stack.empty())
@@ -1376,7 +1379,7 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigne
         CScript pubKey2(pubKeySerialized.begin(), pubKeySerialized.end());
         popstack(stackCopy);
 
-        if (!EvalScript(stackCopy, pubKey2, flags, checker, SIGVERSION_BASE, serror))
+        if (!EvalScript(stackCopy, pubKey2, flags, checker, sigversion, serror))
             // serror is set
             return false;
         if (stackCopy.empty())

src/script/interpreter.h

@@ -96,12 +96,6 @@ struct PrecomputedTransactionData
     PrecomputedTransactionData(const CTransaction& tx);
 };
 
-enum SigVersion
-{
-    SIGVERSION_BASE = 0,
-    SIGVERSION_SAPLING = 1,
-};
-
 uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr);
 
 class BaseSignatureChecker
@@ -157,6 +151,6 @@ class MutableTransactionSignatureChecker : public TransactionSignatureChecker
 };
 
 bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* error = NULL);
-bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror = NULL);
+bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* serror = NULL);
 
 #endif // BITCOIN_SCRIPT_INTERPRETER_H

src/script/sign.cpp

@@ -151,13 +151,13 @@ static CScript PushAll(const std::vector<valtype>& values)
     return result;
 }
 
-bool ProduceSignature(const BaseSignatureCreator& creator, const CScript& fromPubKey, SignatureData& sigdata, bool fColdStake, ScriptError* serror)
+bool ProduceSignature(const BaseSignatureCreator& creator, const CScript& fromPubKey, SignatureData& sigdata, SigVersion sigversion, bool fColdStake, ScriptError* serror)
 {
     CScript script = fromPubKey;
     bool solved = true;
     std::vector<valtype> result;
     txnouttype whichType;
-    solved = SignStep(creator, script, result, whichType, SIGVERSION_BASE, fColdStake);
+    solved = SignStep(creator, script, result, whichType, sigversion, fColdStake);
     CScript subscript;
 
     if (solved && whichType == TX_SCRIPTHASH)
@@ -166,14 +166,14 @@ bool ProduceSignature(const BaseSignatureCreator& creator, const CScript& fromPu
         // the final scriptSig is the signatures from that
         // and then the serialized subscript:
         script = subscript = CScript(result[0].begin(), result[0].end());
-        solved = solved && SignStep(creator, script, result, whichType, SIGVERSION_BASE, fColdStake) && whichType != TX_SCRIPTHASH;
+        solved = solved && SignStep(creator, script, result, whichType, sigversion, fColdStake) && whichType != TX_SCRIPTHASH;
         result.emplace_back(subscript.begin(), subscript.end());
     }
 
     sigdata.scriptSig = PushAll(result);
 
     // Test solution
-    return solved && VerifyScript(sigdata.scriptSig, fromPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker(), serror);
+    return solved && VerifyScript(sigdata.scriptSig, fromPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker(), sigversion, serror);
 }
 
 SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nIn)
@@ -198,7 +198,7 @@ bool SignSignature(const CKeyStore &keystore, const CScript& fromPubKey, CMutabl
     TransactionSignatureCreator creator(&keystore, &txToConst, nIn, amount, nHashType);
 
     SignatureData sigdata;
-    bool ret = ProduceSignature(creator, fromPubKey, sigdata, fColdStake);
+    bool ret = ProduceSignature(creator, fromPubKey, sigdata, txToConst.GetRequiredSigVersion(), fColdStake);
     UpdateTransaction(txTo, nIn, sigdata);
     return ret;
 }
@@ -276,8 +276,8 @@ struct Stacks
 
     Stacks() {}
     explicit Stacks(const std::vector<valtype>& scriptSigStack_) : script(scriptSigStack_) {}
-    explicit Stacks(const SignatureData& data) {
-        EvalScript(script, data.scriptSig, SCRIPT_VERIFY_STRICTENC, BaseSignatureChecker(), SIGVERSION_BASE);
+    explicit Stacks(const SignatureData& data, SigVersion sigversion) {
+        EvalScript(script, data.scriptSig, SCRIPT_VERIFY_STRICTENC, BaseSignatureChecker(), sigversion);
     }
 
     SignatureData Output() const {
@@ -342,7 +342,7 @@ SignatureData CombineSignatures(const CScript& scriptPubKey, const BaseSignature
     std::vector<std::vector<unsigned char> > vSolutions;
     Solver(scriptPubKey, txType, vSolutions);
 
-    return CombineSignatures(scriptPubKey, checker, txType, vSolutions, Stacks(scriptSig1), Stacks(scriptSig2), SIGVERSION_BASE).Output();
+    return CombineSignatures(scriptPubKey, checker, txType, vSolutions, Stacks(scriptSig1, SIGVERSION_BASE), Stacks(scriptSig2, SIGVERSION_BASE), SIGVERSION_BASE).Output();
 }
 
 namespace {
@@ -388,9 +388,9 @@ bool IsSolvable(const CKeyStore& store, const CScript& script, bool fColdStaking
     // if found in a transaction, we would still accept and relay that transaction. In particular,
     DummySignatureCreator creator(&store);
     SignatureData sigs;
-    if (ProduceSignature(creator, script, sigs, fColdStaking)) {
+    if (ProduceSignature(creator, script, sigs, SIGVERSION_BASE, fColdStaking)) {
         // VerifyScript check is just defensive, and should never fail.
-        assert(VerifyScript(sigs.scriptSig, script, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker()));
+        assert(VerifyScript(sigs.scriptSig, script, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker(), SIGVERSION_BASE));
         return true;
     }
     return false;

src/script/sign.h

@@ -68,7 +68,7 @@ struct SignatureData {
 };
 
 /** Produce a script signature using a generic signature creator. */
-bool ProduceSignature(const BaseSignatureCreator& creator, const CScript& fromPubKey, SignatureData& sigdata, bool fColdStake, ScriptError* serror = nullptr);
+bool ProduceSignature(const BaseSignatureCreator& creator, const CScript& fromPubKey, SignatureData& sigdata, SigVersion sigversion, bool fColdStake, ScriptError* serror = nullptr);
 
 /** Produce a script signature for a transaction. */
 bool SignSignature(const CKeyStore& keystore, const CScript& fromPubKey, CMutableTransaction& txTo, unsigned int nIn, const CAmount& amount, int nHashType, bool fColdStake = false);