diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index 612451c764..1c4486df14 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -408,7 +408,6 @@ Batch::preflightSigValidated(PreflightContext const& ctx) } // Validation Batch Signers - std::unordered_set batchSigners; if (ctx.tx.isFieldPresent(sfBatchSigners)) { STArray const& signers = ctx.tx.getFieldArray(sfBatchSigners); @@ -421,11 +420,8 @@ Batch::preflightSigValidated(PreflightContext const& ctx) return temARRAY_TOO_LARGE; } - // Add batch signers to the set to ensure all signer accounts are - // unique. Meanwhile, remove signer accounts from the set of inner - // transaction accounts (`requiredSigners`). By the end of the loop, - // `requiredSigners` should be empty, indicating that all inner - // accounts are matched with signers. + // Batch signers must be in strictly ascending order by AccountID. + AccountID lastBatchSigner(beast::zero); for (auto const& signer : signers) { AccountID const signerAccount = signer.getAccountID(sfAccount); @@ -436,13 +432,21 @@ Batch::preflightSigValidated(PreflightContext const& ctx) return temBAD_SIGNER; } - if (!batchSigners.insert(signerAccount).second) + if (lastBatchSigner == signerAccount) { JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " << "duplicate signer found: " << signerAccount; - return temREDUNDANT; + return temBAD_SIGNER; } + if (lastBatchSigner > signerAccount) + { + JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " + << "unsorted signers array: " << signerAccount; + return temBAD_SIGNER; + } + lastBatchSigner = signerAccount; + // Check that the batch signer is in the required signers set. // Remove it if it does, as it can be crossed off the list. if (requiredSigners.erase(signerAccount) == 0) diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 25f0bb9d20..9d72bc84e2 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -522,7 +522,7 @@ class Batch_test : public beast::unit_test::suite env.close(); } - // temREDUNDANT: Batch: duplicate signer found + // temBAD_SIGNER: Batch: duplicate signer (caught by ascending order check) { auto const seq = env.seq(alice); auto const batchFee = batch::calcBatchFee(env, 2, 2); @@ -530,7 +530,7 @@ class Batch_test : public beast::unit_test::suite batch::inner(pay(alice, bob, XRP(10)), seq + 1), batch::inner(pay(bob, alice, XRP(5)), env.seq(bob)), batch::sig(bob, bob), - ter(temREDUNDANT)); + ter(temBAD_SIGNER)); env.close(); } @@ -4459,6 +4459,41 @@ class Batch_test : public beast::unit_test::suite } } + void + testUnsortedBatchSigners(FeatureBitset features) + { + testcase("unsorted batch signers"); + + using namespace test::jtx; + + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, bob, carol); + env.close(); + + auto const seq = env.seq(alice); + auto const bobSeq = env.seq(bob); + auto const carolSeq = env.seq(carol); + auto const batchFee = batch::calcBatchFee(env, 2, 2); + + auto jt = env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(pay(bob, alice, XRP(10)), bobSeq), + batch::inner(pay(carol, alice, XRP(5)), carolSeq), + batch::sig(bob, carol)); + + auto const s0 = jt.jv[sfBatchSigners.jsonName][0u]; + auto const s1 = jt.jv[sfBatchSigners.jsonName][1u]; + jt.jv[sfBatchSigners.jsonName][0u] = s1; + jt.jv[sfBatchSigners.jsonName][1u] = s0; + + env(jt.jv, ter(temBAD_SIGNER)); + env.close(); + } + void testWithFeats(FeatureBitset features) { @@ -4494,6 +4529,7 @@ class Batch_test : public beast::unit_test::suite testValidateRPCResponse(features); testBatchCalculateBaseFee(features); testStandaloneInnerBatchFlag(features); + testUnsortedBatchSigners(features); } public: