From 4ad697069f0dba765cb8899d4dde0aae51d3f691 Mon Sep 17 00:00:00 2001 From: RichardAH Date: Wed, 27 Dec 2023 14:53:38 +0100 Subject: [PATCH] Fix xahau v1audit (#250) --- src/ripple/app/tx/impl/Escrow.cpp | 53 ++++++------- src/ripple/app/tx/impl/Import.cpp | 21 +++--- src/ripple/app/tx/impl/URIToken.cpp | 113 +++++++++++++++++++++------- src/test/app/URIToken_test.cpp | 5 +- 4 files changed, 126 insertions(+), 66 deletions(-) diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 0ea66498b08..1b5d559b1e3 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -437,11 +437,19 @@ EscrowFinish::preflight(PreflightContext const& ctx) { if (!ctx.tx.isFieldPresent(sfOfferSequence)) return temMALFORMED; - } - if (!ctx.tx.isFieldPresent(sfEscrowID) && - !ctx.tx.isFieldPresent(sfOfferSequence)) - return temMALFORMED; + if (ctx.tx.isFieldPresent(sfEscrowID) && + ctx.tx.getFieldU32(sfOfferSequence) != 0) + return temMALFORMED; + } + else + { + if ((!ctx.tx.isFieldPresent(sfEscrowID) && + !ctx.tx.isFieldPresent(sfOfferSequence)) || + ctx.tx.isFieldPresent(sfEscrowID) && + ctx.tx.isFieldPresent(sfOfferSequence)) + return temMALFORMED; + } return tesSUCCESS; } @@ -472,17 +480,6 @@ EscrowFinish::doApply() bool const fixV1 = view().rules().enabled(fixXahauV1); - if (!fixV1) - { - if (escrowID && ctx_.tx[sfOfferSequence] != 0) - return temMALFORMED; - } - else - { - if (escrowID && offerSequence) - return temMALFORMED; - } - Keylet k = escrowID ? Keylet(ltESCROW, *escrowID) : keylet::escrow(ctx_.tx[sfOwner], *offerSequence); @@ -723,11 +720,19 @@ EscrowCancel::preflight(PreflightContext const& ctx) { if (!ctx.tx.isFieldPresent(sfOfferSequence)) return temMALFORMED; - } - if (!ctx.tx.isFieldPresent(sfEscrowID) && - !ctx.tx.isFieldPresent(sfOfferSequence)) - return temMALFORMED; + if (ctx.tx.isFieldPresent(sfEscrowID) && + ctx.tx.getFieldU32(sfOfferSequence) != 0) + return temMALFORMED; + } + else + { + if ((!ctx.tx.isFieldPresent(sfEscrowID) && + !ctx.tx.isFieldPresent(sfOfferSequence)) || + ctx.tx.isFieldPresent(sfEscrowID) && + ctx.tx.isFieldPresent(sfOfferSequence)) + return temMALFORMED; + } return preflight2(ctx); } @@ -744,16 +749,6 @@ EscrowCancel::doApply() std::optional offerSequence = ctx_.tx[~sfOfferSequence]; bool const fixV1 = view().rules().enabled(fixXahauV1); - if (!fixV1) - { - if (escrowID && ctx_.tx[sfOfferSequence] != 0) - return temMALFORMED; - } - else - { - if (escrowID && offerSequence) - return temMALFORMED; - } Keylet k = escrowID ? Keylet(ltESCROW, *escrowID) : keylet::escrow(ctx_.tx[sfOwner], *offerSequence); diff --git a/src/ripple/app/tx/impl/Import.cpp b/src/ripple/app/tx/impl/Import.cpp index 6b4c335d4f1..d136c7d6733 100644 --- a/src/ripple/app/tx/impl/Import.cpp +++ b/src/ripple/app/tx/impl/Import.cpp @@ -817,16 +817,17 @@ Import::preflight(PreflightContext const& ctx) << " validation count: " << validationCount; // check if the validation count is adequate - auto hasInsufficientQuorum = [&ctx](int quorum, int validationCount) { - if (ctx.rules.enabled(fixXahauV1)) - { - return quorum > validationCount; - } - else - { - return quorum >= validationCount; - } - }; + auto hasInsufficientQuorum = + [&ctx](uint64_t quorum, uint64_t validationCount) { + if (ctx.rules.enabled(fixXahauV1)) + { + return quorum > validationCount; + } + else + { + return quorum >= validationCount; + } + }; if (hasInsufficientQuorum(quorum, validationCount)) { JLOG(ctx.j.warn()) << "Import: xpop did not contain an 80% quorum for " diff --git a/src/ripple/app/tx/impl/URIToken.cpp b/src/ripple/app/tx/impl/URIToken.cpp index 13880684594..741019923cc 100644 --- a/src/ripple/app/tx/impl/URIToken.cpp +++ b/src/ripple/app/tx/impl/URIToken.cpp @@ -144,6 +144,8 @@ URIToken::preflight(PreflightContext const& ctx) TER URIToken::preclaim(PreclaimContext const& ctx) { + bool const fixV1 = ctx.view.rules().enabled(fixXahauV1); + std::shared_ptr sleU; uint32_t leFlags; std::optional issuer; @@ -180,6 +182,11 @@ URIToken::preclaim(PreclaimContext const& ctx) AccountID const acc = ctx.tx.getAccountID(sfAccount); uint16_t tt = ctx.tx.getFieldU16(sfTransactionType); + auto const sle = + ctx.view.read(keylet::account(ctx.tx.getAccountID(sfAccount))); + if (!sle) + return tefINTERNAL; + switch (tt) { case ttURITOKEN_MINT: { @@ -228,24 +235,75 @@ URIToken::preclaim(PreclaimContext const& ctx) if (purchaseAmount < saleAmount) return tecINSUFFICIENT_PAYMENT; - if (purchaseAmount.native() && saleAmount->native()) + if (fixV1) { - // if it's an xrp sale/purchase then no trustline needed - if (purchaseAmount > - (sleOwner->getFieldAmount(sfBalance) - ctx.tx[sfFee])) - return tecINSUFFICIENT_FUNDS; - } + if (purchaseAmount.native() && saleAmount->native()) + { + // native transfer - // execution to here means it's an IOU sale - // check if the buyer has the right trustline with an adequate - // balance + STAmount needed{ctx.view.fees().accountReserve( + sle->getFieldU32(sfOwnerCount) + 1)}; - STAmount availableFunds{accountFunds( - ctx.view, acc, purchaseAmount, fhZERO_IF_FROZEN, ctx.j)}; + STAmount const fee = ctx.tx.getFieldAmount(sfFee).xrp(); - if (purchaseAmount > availableFunds) - return tecINSUFFICIENT_FUNDS; + if (needed + fee < needed) + return tecINTERNAL; + needed += fee; + + if (needed + purchaseAmount < needed) + return tecINTERNAL; + + needed += purchaseAmount; + + if (needed > sle->getFieldAmount(sfBalance)) + return tecINSUFFICIENT_FUNDS; + } + else if (purchaseAmount.native() || saleAmount->native()) + { + // should not be able to happen + return tecINTERNAL; + } + else + { + // iou transfer + + STAmount availableFunds{accountFunds( + ctx.view, + acc, + purchaseAmount, + fhZERO_IF_FROZEN, + ctx.j)}; + + if (purchaseAmount > availableFunds) + return tecINSUFFICIENT_FUNDS; + } + } + else + { + // old logic + + if (purchaseAmount.native() && saleAmount->native()) + { + // if it's an xrp sale/purchase then no trustline needed + if (purchaseAmount > + (sleOwner->getFieldAmount(sfBalance) - ctx.tx[sfFee])) + return tecINSUFFICIENT_FUNDS; + } + else + { + // iou + STAmount availableFunds{accountFunds( + ctx.view, + acc, + purchaseAmount, + fhZERO_IF_FROZEN, + ctx.j)}; + + if (purchaseAmount > availableFunds) + return tecINSUFFICIENT_FUNDS; + } + } return tesSUCCESS; } @@ -412,17 +470,6 @@ URIToken::doApply() } case ttURITOKEN_BUY: { - if (account_ == *owner) - { - // this is a clear operation - sleU->makeFieldAbsent(sfAmount); - if (sleU->isFieldPresent(sfDestination)) - sleU->makeFieldAbsent(sfDestination); - sb.update(sleU); - sb.apply(ctx_.rawView()); - return tesSUCCESS; - } - STAmount const purchaseAmount = ctx_.tx.getFieldAmount(sfAmount); // check if the seller has listed it at all @@ -446,8 +493,22 @@ URIToken::doApply() // if it's an xrp sale/purchase then no trustline needed if (purchaseAmount.native()) { - if (purchaseAmount > - ((*sleOwner)[sfBalance] - ctx_.tx[sfFee])) + STAmount needed{sb.fees().accountReserve( + sle->getFieldU32(sfOwnerCount) + 1)}; + + STAmount const fee = ctx_.tx.getFieldAmount(sfFee).xrp(); + + if (needed + fee < needed) + return tecINTERNAL; + + needed += fee; + + if (needed + purchaseAmount < needed) + return tecINTERNAL; + + needed += purchaseAmount; + + if (needed > mPriorBalance) return tecINSUFFICIENT_FUNDS; } else diff --git a/src/test/app/URIToken_test.cpp b/src/test/app/URIToken_test.cpp index 9c5090efb3a..79088f5794a 100644 --- a/src/test/app/URIToken_test.cpp +++ b/src/test/app/URIToken_test.cpp @@ -454,7 +454,10 @@ struct URIToken_test : public beast::unit_test::suite using namespace std::literals::chrono_literals; // setup env - Env env{*this, features}; + Env env{ + *this, envconfig(), features, nullptr, beast::severities::kWarning + // beast::severities::kTrace + }; auto const alice = Account("alice"); auto const bob = Account("bob"); auto const carol = Account("carol");