diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index a4360ccc8b..5b3c0d2372 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/src/test/rpc/Simulate_test.cpp @@ -720,7 +720,11 @@ class Simulate_test : public beast::unit_test::suite Json::Value const& tx) { auto result = resp[jss::result]; checkBasicReturnValidity( - result, tx, env.seq(alice), env.current()->fees().base * 2); + result, + tx, + env.seq(alice), + tx.isMember(jss::Signers) ? env.current()->fees().base * 2 + : env.current()->fees().base); BEAST_EXPECT(result[jss::engine_result] == "tesSUCCESS"); BEAST_EXPECT(result[jss::engine_result_code] == 0); @@ -762,6 +766,10 @@ class Simulate_test : public beast::unit_test::suite tx[jss::Account] = alice.human(); tx[jss::TransactionType] = jss::AccountSet; tx[sfDomain] = newDomain; + + // test with autofill + testTx(env, tx, validateOutput, false); + tx[sfSigners] = Json::arrayValue; { Json::Value signer; @@ -771,7 +779,7 @@ class Simulate_test : public beast::unit_test::suite tx[sfSigners].append(signerOuter); } - // test with autofill + // test with just signer accounts testTx(env, tx, validateOutput, false); tx[sfSigningPubKey] = ""; @@ -780,8 +788,7 @@ class Simulate_test : public beast::unit_test::suite // transaction requires a non-base fee tx[sfFee] = (env.current()->fees().base * 2).jsonClipped().asString(); - tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = - strHex(becky.pk().slice()); + tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = ""; tx[sfSigners][0u][sfSigner][jss::TxnSignature] = ""; // test without autofill @@ -830,11 +837,12 @@ class Simulate_test : public beast::unit_test::suite tx[jss::Account] = env.master.human(); tx[jss::TransactionType] = jss::AccountSet; tx[sfDomain] = newDomain; + // master key is disabled, so this is invalid + tx[jss::SigningPubKey] = strHex(env.master.pk().slice()); // test with autofill testTx(env, tx, testSimulation); - tx[sfSigningPubKey] = ""; tx[sfTxnSignature] = ""; tx[sfSequence] = env.seq(env.master); tx[sfFee] = env.current()->fees().base.jsonClipped().asString(); @@ -844,6 +852,79 @@ class Simulate_test : public beast::unit_test::suite } } + void + testInvalidSingleAndMultiSigningTransaction() + { + testcase( + "Transaction with both single-signing SigningPubKey and " + "multi-signing Signers"); + + using namespace jtx; + Env env(*this); + static auto const newDomain = "123ABC"; + Account const alice("alice"); + Account const becky("becky"); + Account const carol("carol"); + env.fund(XRP(10000), alice); + env.close(); + + // set up valid multisign + env(signers(alice, 1, {{becky, 1}, {carol, 1}})); + env.close(); + + { + std::function const& + testSimulation = [&](Json::Value const& resp, + Json::Value const& tx) { + auto result = resp[jss::result]; + checkBasicReturnValidity( + result, + tx, + env.seq(env.master), + env.current()->fees().base * 2); + + BEAST_EXPECT(result[jss::engine_result] == "temINVALID"); + BEAST_EXPECT(result[jss::engine_result_code] == -277); + BEAST_EXPECT( + result[jss::engine_result_message] == + "The transaction is ill-formed."); + + BEAST_EXPECT( + !result.isMember(jss::meta) && + !result.isMember(jss::meta_blob)); + }; + + Json::Value tx; + + tx[jss::Account] = env.master.human(); + tx[jss::TransactionType] = jss::AccountSet; + tx[sfDomain] = newDomain; + // master key is disabled, so this is invalid + tx[jss::SigningPubKey] = strHex(env.master.pk().slice()); + tx[sfSigners] = Json::arrayValue; + { + Json::Value signer; + signer[jss::Account] = becky.human(); + Json::Value signerOuter; + signerOuter[sfSigner] = signer; + tx[sfSigners].append(signerOuter); + } + + // test with autofill + testTx(env, tx, testSimulation, false); + + tx[sfTxnSignature] = ""; + tx[sfSequence] = env.seq(env.master); + tx[sfFee] = env.current()->fees().base.jsonClipped().asString(); + tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = + strHex(becky.pk().slice()); + tx[sfSigners][0u][sfSigner][jss::TxnSignature] = ""; + + // test without autofill + testTx(env, tx, testSimulation); + } + } + void testMultisignedBadPubKey() { @@ -1117,6 +1198,7 @@ public: testTransactionTecFailure(); testSuccessfulTransactionMultisigned(); testTransactionSigningFailure(); + testInvalidSingleAndMultiSigningTransaction(); testMultisignedBadPubKey(); testDeleteExpiredCredentials(); testSuccessfulTransactionNetworkID(); diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 5f4cba5cf8..0db0484842 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -184,6 +184,12 @@ preflight2(PreflightContext const& ctx) return temINVALID; // LCOV_EXCL_LINE } } + + if (!ctx.tx.getSigningPubKey().empty()) + { + // trying to single-sign _and_ multi-sign a transaction + return temINVALID; + } return tesSUCCESS; } @@ -297,9 +303,9 @@ Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee) if (balance < feePaid) { - JLOG(ctx.j.trace()) << "Insufficient balance:" - << " balance=" << to_string(balance) - << " paid=" << to_string(feePaid); + JLOG(ctx.j.trace()) + << "Insufficient balance:" << " balance=" << to_string(balance) + << " paid=" << to_string(feePaid); if ((balance > beast::zero) && !ctx.view.open()) { @@ -571,13 +577,13 @@ Transactor::apply() NotTEC Transactor::checkSign(PreclaimContext const& ctx) { + auto const pkSigner = ctx.tx.getSigningPubKey(); // Ignore signature check on batch inner transactions if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.view.rules().enabled(featureBatch)) { // Defensive Check: These values are also checked in Batch::preflight - if (ctx.tx.isFieldPresent(sfTxnSignature) || - !ctx.tx.getSigningPubKey().empty() || + if (ctx.tx.isFieldPresent(sfTxnSignature) || !pkSigner.empty() || ctx.tx.isFieldPresent(sfSigners)) { return temINVALID_FLAG; // LCOV_EXCL_LINE @@ -585,25 +591,30 @@ Transactor::checkSign(PreclaimContext const& ctx) return tesSUCCESS; } + if ((ctx.flags & tapDRY_RUN) && pkSigner.empty() && + !ctx.tx.isFieldPresent(sfSigners)) + { + // simulate: skip signature validation when neither SigningPubKey nor + // Signers are provided + return tesSUCCESS; + } + auto const idAccount = ctx.tx[~sfDelegate].value_or(ctx.tx[sfAccount]); // If the pk is empty and not simulate or simulate and signers, // then we must be multi-signing. - if ((ctx.flags & tapDRY_RUN && ctx.tx.isFieldPresent(sfSigners)) || - (!(ctx.flags & tapDRY_RUN) && ctx.tx.getSigningPubKey().empty())) + if (ctx.tx.isFieldPresent(sfSigners)) { STArray const& txSigners(ctx.tx.getFieldArray(sfSigners)); return checkMultiSign(ctx.view, idAccount, txSigners, ctx.flags, ctx.j); } // Check Single Sign - auto const pkSigner = ctx.tx.getSigningPubKey(); - // This ternary is only needed to handle `simulate` XRPL_ASSERT( - (ctx.flags & tapDRY_RUN) || !pkSigner.empty(), + !pkSigner.empty(), "ripple::Transactor::checkSingleSign : non-empty signer or simulation"); - if (!(ctx.flags & tapDRY_RUN) && !publicKeyType(makeSlice(pkSigner))) + if (!publicKeyType(makeSlice(pkSigner))) { JLOG(ctx.j.trace()) << "checkSingleSign: signing public key type is unknown"; @@ -798,14 +809,15 @@ Transactor::checkMultiSign( // public key. auto const spk = txSigner.getFieldVL(sfSigningPubKey); - if (!(flags & tapDRY_RUN) && !publicKeyType(makeSlice(spk))) + // spk being non-empty in non-simulate is checked in + // STTx::checkMultiSign + if (!spk.empty() && !publicKeyType(makeSlice(spk))) { JLOG(j.trace()) << "checkMultiSign: signing public key type is unknown"; return tefBAD_SIGNATURE; } - // This ternary is only needed to handle `simulate` XRPL_ASSERT( (flags & tapDRY_RUN) || !spk.empty(), "ripple::Transactor::checkMultiSign : non-empty signer or "