diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 92f286ca6a..8f889f4b00 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -2313,9 +2313,12 @@ class Batch_test : public beast::unit_test::suite Serializer s; parsed.object->add(s); auto const jrr = env.rpc("submit", strHex(s.slice()))[jss::result]; - BEAST_EXPECT( - jrr[jss::status] == "success" && - jrr[jss::engine_result] == "temINVALID_FLAG"); + BEAST_EXPECTS( + jrr[jss::status] == "error" && + jrr[jss::error] == "invalidTransaction" && + jrr[jss::error_exception] == + "fails local checks: Empty SigningPubKey.", + to_string(jrr)); env.close(); } @@ -2553,6 +2556,242 @@ class Batch_test : public beast::unit_test::suite } } + void + testLoan(FeatureBitset features) + { + testcase("loan"); + + using namespace test::jtx; + + test::jtx::Env env{ + *this, + envconfig(), + features | featureSingleAssetVault | featureLendingProtocol | + featureMPTokensV1}; + + Account const issuer{"issuer"}; + // For simplicity, lender will be the sole actor for the vault & + // brokers. + Account const lender{"lender"}; + // Borrower only wants to borrow + Account const borrower{"borrower"}; + + // Fund the accounts and trust lines with the same amount so that tests + // can use the same values regardless of the asset. + env.fund(XRP(100'000), issuer, noripple(lender, borrower)); + env.close(); + + // Just use an XRP asset + PrettyAsset const asset{xrpIssue(), 1'000'000}; + + Vault vault{env}; + + auto const deposit = asset(50'000); + auto const debtMaximumValue = asset(25'000).value(); + auto const coverDepositValue = asset(1000).value(); + + auto [tx, vaultKeylet] = + vault.create({.owner = lender, .asset = asset}); + env(tx); + env.close(); + BEAST_EXPECT(env.le(vaultKeylet)); + + env(vault.deposit( + {.depositor = lender, .id = vaultKeylet.key, .amount = deposit})); + env.close(); + + auto const brokerKeylet = + keylet::loanbroker(lender.id(), env.seq(lender)); + + { + using namespace loanBroker; + env(set(lender, vaultKeylet.key), + managementFeeRate(TenthBips16(100)), + debtMaximum(debtMaximumValue), + coverRateMinimum(TenthBips32(percentageToTenthBips(10))), + coverRateLiquidation(TenthBips32(percentageToTenthBips(25)))); + + env(coverDeposit(lender, brokerKeylet.key, coverDepositValue)); + + env.close(); + } + + { + using namespace loan; + using namespace std::chrono_literals; + + auto const lenderSeq = env.seq(lender); + auto const batchFee = batch::calcBatchFee(env, 0, 2); + + auto const loanKeylet = keylet::loan(brokerKeylet.key, 1); + { + auto const [txIDs, batchID] = submitBatch( + env, + temBAD_SIGNATURE, + batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), + batch::inner( + env.json( + set(lender, + brokerKeylet.key, + asset(1000).value(), + env.now() + 3600s), + // Not allowed to include the counterparty signature + sig(sfCounterpartySignature, borrower), + sig(none), + fee(none), + seq(none)), + lenderSeq + 1), + batch::inner( + draw( + lender, + loanKeylet.key, + STAmount{asset, asset(500).value()}), + lenderSeq + 2)); + } + { + auto const [txIDs, batchID] = submitBatch( + env, + telENV_RPC_FAILED, + batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), + batch::inner( + env.json( + set(lender, + brokerKeylet.key, + asset(1000).value(), + env.now() + 3600s), + // Must include a CounterpartySignature field. + // Transaction will not even parse at the RPC layer + sig(none), + fee(none), + seq(none)), + lenderSeq + 1), + batch::inner( + draw( + lender, + loanKeylet.key, + STAmount{asset, asset(500).value()}), + lenderSeq + 2)); + } + { + auto const [txIDs, batchID] = submitBatch( + env, + temINVALID_INNER_BATCH, + batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), + batch::inner( + env.json( + set(lender, + brokerKeylet.key, + asset(1000).value(), + env.now() + 3600s), + json(sfCounterpartySignature, Json::objectValue), + // Counterparty must be set + sig(none), + fee(none), + seq(none)), + lenderSeq + 1), + batch::inner( + draw( + lender, + loanKeylet.key, + STAmount{asset, asset(500).value()}), + lenderSeq + 2)); + } + { + auto const [txIDs, batchID] = submitBatch( + env, + temBAD_SIGNER, + batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), + batch::inner( + env.json( + set(lender, + brokerKeylet.key, + asset(1000).value(), + env.now() + 3600s), + // Counterparty must sign the outer transaction + counterparty(borrower.id()), + json(sfCounterpartySignature, Json::objectValue), + sig(none), + fee(none), + seq(none)), + lenderSeq + 1), + batch::inner( + draw( + lender, + loanKeylet.key, + STAmount{asset, asset(500).value()}), + lenderSeq + 2)); + } + { + // LoanSet normally charges at least 2x base fee, but since the + // signature check is done by the batch, it only charges the + // base fee. + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const [txIDs, batchID] = submitBatch( + env, + tesSUCCESS, + batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), + batch::inner( + env.json( + set(lender, + brokerKeylet.key, + asset(1000).value(), + env.now() + 3600s), + counterparty(borrower.id()), + json(sfCounterpartySignature, Json::objectValue), + sig(none), + fee(none), + seq(none)), + lenderSeq + 1), + batch::inner( + draw( + // However, this inner transaction will fail, + // because the lender is not allowed to draw the + // transaction + lender, + loanKeylet.key, + STAmount{asset, asset(500).value()}), + lenderSeq + 2), + batch::sig(borrower)); + } + env.close(); + BEAST_EXPECT(env.le(brokerKeylet)); + BEAST_EXPECT(!env.le(loanKeylet)); + { + // LoanSet normally charges at least 2x base fee, but since the + // signature check is done by the batch, it only charges the + // base fee. + auto const lenderSeq = env.seq(lender); + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const [txIDs, batchID] = submitBatch( + env, + tesSUCCESS, + batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), + batch::inner( + env.json( + set(lender, + brokerKeylet.key, + asset(1000).value(), + env.now() + 3600s), + counterparty(borrower.id()), + json(sfCounterpartySignature, Json::objectValue), + sig(none), + fee(none), + seq(none)), + lenderSeq + 1), + batch::inner( + manage(lender, loanKeylet.key, tfLoanImpair), + lenderSeq + 2), + batch::sig(borrower)); + } + env.close(); + BEAST_EXPECT(env.le(brokerKeylet)); + if (auto const sleLoan = env.le(loanKeylet); BEAST_EXPECT(sleLoan)) + { + BEAST_EXPECT(sleLoan->isFlag(lsfLoanImpaired)); + } + } + } + void testObjectCreateSequence(FeatureBitset features) { @@ -4147,6 +4386,7 @@ class Batch_test : public beast::unit_test::suite testAccountActivation(features); testAccountSet(features); testAccountDelete(features); + testLoan(features); testObjectCreateSequence(features); testObjectCreateTicket(features); testObjectCreate3rdParty(features); diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 57fe79e433..b366d1df1a 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -1938,7 +1938,7 @@ class Loan_test : public beast::unit_test::suite env(batch::outer(borrower, seq, batchFee, tfAllOrNothing), batch::inner(forgedLoanSet, seq + 1), batch::inner(pay(borrower, lender, XRP(1)), seq + 2), - ter(tesSUCCESS)); + ter(temBAD_SIGNATURE)); env.close(); // ? Check that the loan was created @@ -1949,37 +1949,7 @@ class Loan_test : public beast::unit_test::suite auto const res = env.rpc("json", "account_objects", to_string(params)); auto const objects = res[jss::result][jss::account_objects]; - BEAST_EXPECT(objects.size() == 1); - - auto const loan = objects[0u]; - BEAST_EXPECT(loan[sfAssetsAvailable] == "1000"); - BEAST_EXPECT(loan[sfBorrower] == borrower.human()); - BEAST_EXPECT(loan[sfCloseInterestRate] == 0); - BEAST_EXPECT(loan[sfClosePaymentFee] == "0"); - BEAST_EXPECT(loan[sfFlags] == 0); - BEAST_EXPECT(loan[sfGracePeriod] == 60); - BEAST_EXPECT(loan[sfInterestRate] == 0); - BEAST_EXPECT(loan[sfLateInterestRate] == 0); - BEAST_EXPECT(loan[sfLatePaymentFee] == "0"); - BEAST_EXPECT(loan[sfLoanBrokerID] == to_string(broker.brokerID)); - BEAST_EXPECT(loan[sfLoanOriginationFee] == "0"); - BEAST_EXPECT(loan[sfLoanSequence] == 1); - BEAST_EXPECT(loan[sfLoanServiceFee] == "0"); - BEAST_EXPECT( - loan[sfNextPaymentDueDate] == loan[sfStartDate].asUInt() + 60); - BEAST_EXPECT(loan[sfOverpaymentFee] == 0); - BEAST_EXPECT(loan[sfOverpaymentInterestRate] == 0); - BEAST_EXPECT(loan[sfPaymentInterval] == 60); - BEAST_EXPECT(loan[sfPaymentRemaining] == 1); - BEAST_EXPECT(loan[sfPreviousPaymentDate] == 0); - BEAST_EXPECT(loan[sfPrincipalOutstanding] == "1000"); - BEAST_EXPECT(loan[sfPrincipalRequested] == "1000"); - BEAST_EXPECT( - loan[sfStartDate].asUInt() == - startDate.time_since_epoch().count()); - - // TODO: Draw the funds for a full exploit - // env(draw(borrower, loanKeylet.key, XRP(500))); + BEAST_EXPECT(objects.size() == 0); } } diff --git a/src/test/jtx/batch.h b/src/test/jtx/batch.h index ab235c293f..2baa52fd06 100644 --- a/src/test/jtx/batch.h +++ b/src/test/jtx/batch.h @@ -68,8 +68,7 @@ public: inner( Json::Value const& txn, std::uint32_t const& sequence, - std::optional const& ticket = std::nullopt, - std::optional const& fee = std::nullopt) + std::optional const& ticket = std::nullopt) : txn_(txn), seq_(sequence), ticket_(ticket) { txn_[jss::SigningPubKey] = ""; diff --git a/src/xrpld/app/ledger/detail/OpenLedger.cpp b/src/xrpld/app/ledger/detail/OpenLedger.cpp index 2c98caaa6d..f3c779209e 100644 --- a/src/xrpld/app/ledger/detail/OpenLedger.cpp +++ b/src/xrpld/app/ledger/detail/OpenLedger.cpp @@ -124,8 +124,11 @@ OpenLedger::accept( auto const txId = tx->getTransactionID(); // skip batch txns + // The flag should only be settable if Batch feature is enabled. If + // Batch is not enabled, the flag is always invalid, so don't relay it + // regardless. // LCOV_EXCL_START - if (tx->isFlag(tfInnerBatchTxn) && rules.enabled(featureBatch)) + if (tx->isFlag(tfInnerBatchTxn)) { XRPL_ASSERT( txpair.second && txpair.second->isFieldPresent(sfParentBatchID), diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 3220ce99fc..992a72cb9e 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1687,10 +1687,11 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) app_.getHashRouter().shouldRelay(e.transaction->getID()); if (auto const sttx = *(e.transaction->getSTransaction()); toSkip && - // Skip relaying if it's an inner batch txn and batch - // feature is enabled - !(sttx.isFlag(tfInnerBatchTxn) && - newOL->rules().enabled(featureBatch))) + // Skip relaying if it's an inner batch txn. The flag should + // only be set if the Batch feature is enabled. If Batch is + // not enabled, the flag is always invalid, so don't relay + // it regardless. + !sttx.isFlag(tfInnerBatchTxn)) { protocol::TMTransaction tx; Serializer s; @@ -3051,9 +3052,11 @@ NetworkOPsImp::pubProposedTransaction( std::shared_ptr const& transaction, TER result) { - // never publish an inner txn inside a batch txn - if (transaction->isFlag(tfInnerBatchTxn) && - ledger->rules().enabled(featureBatch)) + // never publish an inner txn inside a batch txn. The flag should + // only be set if the Batch feature is enabled. If Batch is not + // enabled, the flag is always invalid, so don't publish it + // regardless. + if (transaction->isFlag(tfInnerBatchTxn)) return; MultiApiJson jvObj = diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index 5f05d9919e..0678998dc3 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -245,6 +245,39 @@ Batch::preflight(PreflightContext const& ctx) std::unordered_set uniqueHashes; std::unordered_map> accountSeqTicket; + auto checkSignatureFields = [&parentBatchId, &j = ctx.j]( + STObject const& sig, + uint256 const& hash, + char const* label = "") -> NotTEC { + if (sig.isFieldPresent(sfTxnSignature)) + { + JLOG(j.debug()) + << "BatchTrace[" << parentBatchId << "]: " + << "inner txn " << label << "cannot include TxnSignature. " + << "txID: " << hash; + return temBAD_SIGNATURE; + } + + if (sig.isFieldPresent(sfSigners)) + { + JLOG(j.debug()) + << "BatchTrace[" << parentBatchId << "]: " + << "inner txn " << label << " cannot include Signers. " + << "txID: " << hash; + return temBAD_SIGNER; + } + + if (!sig.getFieldVL(sfSigningPubKey).empty()) + { + JLOG(j.debug()) + << "BatchTrace[" << parentBatchId << "]: " + << "inner txn " << label << " SigningPubKey must be empty. " + << "txID: " << hash; + return temBAD_REGKEY; + } + + return tesSUCCESS; + }; for (STObject rb : rawTxns) { STTx const stx = STTx{std::move(rb)}; @@ -274,28 +307,18 @@ Batch::preflight(PreflightContext const& ctx) return temINVALID_FLAG; } - if (stx.isFieldPresent(sfTxnSignature)) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "inner txn cannot include TxnSignature. " - << "txID: " << hash; - return temBAD_SIGNATURE; - } + if (auto const ret = checkSignatureFields(stx, hash)) + return ret; - if (stx.isFieldPresent(sfSigners)) + if (stx.isFieldPresent(sfCounterpartySignature)) { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "inner txn cannot include Signers. " - << "txID: " << hash; - return temBAD_SIGNER; - } - - if (!stx.getSigningPubKey().empty()) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "inner txn SigningPubKey must be empty. " - << "txID: " << hash; - return temBAD_REGKEY; + auto const counterpartySignature = + stx.getFieldObject(sfCounterpartySignature); + if (auto const ret = checkSignatureFields( + counterpartySignature, hash, "counterparty signature ")) + { + return ret; + } } auto const innerAccount = stx.getAccountID(sfAccount); @@ -376,6 +399,11 @@ Batch::preflight(PreflightContext const& ctx) // inner account to the required signers set. if (innerAccount != outerAccount) requiredSigners.insert(innerAccount); + // Some transactions have a Counterparty, who must also sign the + // transaction if they are not the outer account + if (auto const counterparty = stx.at(~sfCounterparty); + counterparty && counterparty != outerAccount) + requiredSigners.insert(*counterparty); } // Validation Batch Signers diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index 1ef84c0346..8aec6cc268 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -62,6 +62,17 @@ NotTEC LoanSet::preflight(PreflightContext const& ctx) { auto const& tx = ctx.tx; + + // Special case for Batch inner transactions + if (tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatch) && + !tx.isFieldPresent(sfCounterparty)) + { + auto const parentBatchId = ctx.parentBatchId.value_or(uint256{0}); + JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " + << "no Counterparty for inner LoanSet transaction."; + return temBAD_SIGNER; + } + auto const counterPartySig = ctx.tx.getFieldObject(sfCounterpartySignature); if (auto const ret = @@ -139,10 +150,12 @@ LoanSet::calculateBaseFee(ReadView const& view, STTx const& tx) auto const counterSig = tx.getFieldObject(sfCounterpartySignature); // Each signer adds one more baseFee to the minimum required fee - // for the transaction. Note that unlike the base class, if there are no - // signers, 1 extra signature is still counted for the single signer. - std::size_t const signerCount = - tx.isFieldPresent(sfSigners) ? tx.getFieldArray(sfSigners).size() : 1; + // for the transaction. Note that unlike the base class, the single signer + // is counted if present. It will only be absent in a batch inner + // transaction. + std::size_t const signerCount = tx.isFieldPresent(sfSigners) + ? tx.getFieldArray(sfSigners).size() + : (counterSig.isFieldPresent(sfTxnSignature) ? 1 : 0); return normalCost + (signerCount * baseFee); } diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 2b2a9b9e83..fc7888cc87 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -231,12 +231,17 @@ Transactor::preflight2(PreflightContext const& ctx) // regardless of success or failure return *ret; - auto const sigValid = checkValidity( - ctx.app.getHashRouter(), ctx.tx, ctx.rules, ctx.app.config()); - if (sigValid.first == Validity::SigBad) + // Skip signature check on batch inner transactions + if (!ctx.tx.isFlag(tfInnerBatchTxn) || !ctx.rules.enabled(featureBatch)) { - JLOG(ctx.j.debug()) << "preflight2: bad signature. " << sigValid.second; - return temINVALID; // LCOV_EXCL_LINE + auto const sigValid = checkValidity( + ctx.app.getHashRouter(), ctx.tx, ctx.rules, ctx.app.config()); + if (sigValid.first == Validity::SigBad) + { + JLOG(ctx.j.debug()) + << "preflight2: bad signature. " << sigValid.second; + return temINVALID; // LCOV_EXCL_LINE + } } return tesSUCCESS; } @@ -662,8 +667,7 @@ Transactor::checkSign( auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey); // Ignore signature check on batch inner transactions - if (sigObject.isFlag(tfInnerBatchTxn) && - ctx.view.rules().enabled(featureBatch)) + if (ctx.parentBatchId && ctx.view.rules().enabled(featureBatch)) { // Defensive Check: These values are also checked in Batch::preflight if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.empty() || diff --git a/src/xrpld/app/tx/detail/apply.cpp b/src/xrpld/app/tx/detail/apply.cpp index e2e0adae45..99f063407b 100644 --- a/src/xrpld/app/tx/detail/apply.cpp +++ b/src/xrpld/app/tx/detail/apply.cpp @@ -59,16 +59,6 @@ checkValidity( return { Validity::SigBad, "Malformed: Invalid inner batch transaction."}; - - std::string reason; - if (!passesLocalChecks(tx, reason)) - { - router.setFlags(id, SF_LOCALBAD); - return {Validity::SigGoodOnly, reason}; - } - - router.setFlags(id, SF_SIGGOOD); - return {Validity::Valid, ""}; } if (any(flags & SF_SIGBAD)) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 23b4760488..d36ccf2d3c 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1286,8 +1286,7 @@ PeerImp::handleTransaction( // Charge strongly for attempting to relay a txn with tfInnerBatchTxn // LCOV_EXCL_START - if (stx->isFlag(tfInnerBatchTxn) && - getCurrentTransactionRules()->enabled(featureBatch)) + if (stx->isFlag(tfInnerBatchTxn)) { JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing " "tfInnerBatchTxn (handleTransaction)."; @@ -2851,8 +2850,7 @@ PeerImp::checkTransaction( { // charge strongly for relaying batch txns // LCOV_EXCL_START - if (stx->isFlag(tfInnerBatchTxn) && - getCurrentTransactionRules()->enabled(featureBatch)) + if (stx->isFlag(tfInnerBatchTxn)) { JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing " "tfInnerBatchTxn (checkSignature).";