Fix xahau v1audit (XRPLF#250)

Author: RichardAH

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<std::uint32_t> 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);

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 "

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<SLE const> sleU;
     uint32_t leFlags;
     std::optional<AccountID> 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

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