ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-28 17:40:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

Ensure Counterparty Signatures are properly handled by Batch tx

Browse Source 
- Addresses FIND-001 from audit
- LoanSet::preflight will require Counterparty to be set (it's normally
  optional) for an inner batch transaction, because the checks are done
  before the LoanBroker object can be accessed.
- Adjust LoanSet::calculateBaseFee to not charge extra if an inner
  transaction.
- Adds a Loan-specific test to Batch_test.
This commit is contained in:
Ed Hennis
2025-07-29 18:34:14 -04:00
parent 421cbb9abd
commit 9d1a23a811
10 changed files with 338 additions and 90 deletions
Download Patch File Download Diff File Expand all files Collapse all files

246
src/test/app/Batch_test.cpp
View File

@@ -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);

34
src/test/app/Loan_test.cpp
View File

@@ -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);
}
}

3
src/test/jtx/batch.h
View File

@@ -68,8 +68,7 @@ public:
inner(
Json::Value const& txn,
std::uint32_t const& sequence,
std::optional<std::uint32_t> const& ticket = std::nullopt,
std::optional<std::uint32_t> const& fee = std::nullopt)
std::optional<std::uint32_t> const& ticket = std::nullopt)
: txn_(txn), seq_(sequence), ticket_(ticket)
{
txn_[jss::SigningPubKey] = "";

5
src/xrpld/app/ledger/detail/OpenLedger.cpp
View File

@@ -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),

17
src/xrpld/app/misc/NetworkOPs.cpp
View File

@@ -1687,10 +1687,11 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& 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<STTx const> 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 =

68
src/xrpld/app/tx/detail/Batch.cpp
View File

@@ -245,6 +245,39 @@ Batch::preflight(PreflightContext const& ctx)
std::unordered_set<uint256> uniqueHashes;
std::unordered_map<AccountID, std::unordered_set<std::uint32_t>>
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

21
src/xrpld/app/tx/detail/LoanSet.cpp
View File

@@ -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);
}

18
src/xrpld/app/tx/detail/Transactor.cpp
View File

@@ -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() ||

10
src/xrpld/app/tx/detail/apply.cpp
View File

@@ -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))

6
src/xrpld/overlay/detail/PeerImp.cpp
View File

@@ -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).";