Validate fee during preflight

src/ripple/app/tx/impl/CancelOffer.cpp

@@ -29,6 +29,10 @@ namespace ripple {
 TER
 CancelOffer::preflight (PreflightContext const& ctx)
 {
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     auto const uTxFlags = ctx.tx.getFlags();
 
     if (uTxFlags & tfUniversalMask)
@@ -46,7 +50,7 @@ CancelOffer::preflight (PreflightContext const& ctx)
         return temBAD_SEQUENCE;
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2(ctx);
 }
 
 //------------------------------------------------------------------------------

src/ripple/app/tx/impl/CancelTicket.cpp

@@ -32,7 +32,12 @@ CancelTicket::preflight (PreflightContext const& ctx)
     if (! (ctx.flags & tapENABLE_TESTING))
         return temDISABLED;
 #endif
-    return Transactor::preflight(ctx);
+
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
+    return preflight2 (ctx);
 }
 
 TER

src/ripple/app/tx/impl/Change.cpp

@@ -32,13 +32,19 @@ TER
 Change::preflight (PreflightContext const& ctx)
 {
     auto account = ctx.tx.getAccountID(sfAccount);
-
+    if (account != zero)
-    if (account.isNonZero ())
     {
         JLOG(ctx.j.warning) << "Bad source id";
         return temBAD_SRC_ACCOUNT;
     }
 
+    auto const fee = ctx.tx.getTransactionFee ();
+    if (!fee.native () || fee != beast::zero)
+    {
+        JLOG(ctx.j.warning) << "Non-zero fee";
+        return temBAD_FEE;
+    }
+
     return tesSUCCESS;
 }
 
@@ -89,7 +95,7 @@ Change::checkSeq()
 TER
 Change::payFee()
 {
-    if (tx().getTransactionFee () != STAmount ())
+    if (tx().getTransactionFee () != beast::zero)
     {
         j_.warning << "Non-zero fee";
         return temBAD_FEE;

src/ripple/app/tx/impl/CreateOffer.cpp

@@ -36,6 +36,10 @@ namespace ripple {
 TER
 CreateOffer::preflight (PreflightContext const& ctx)
 {
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     auto& tx = ctx.tx;
     auto& j = ctx.j;
 
@@ -123,7 +127,7 @@ CreateOffer::preflight (PreflightContext const& ctx)
         return temBAD_ISSUER;
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2(ctx);
 }
 
 TER

src/ripple/app/tx/impl/CreateTicket.cpp

@@ -33,6 +33,10 @@ CreateTicket::preflight (PreflightContext const& ctx)
         return temDISABLED;
 #endif
 
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     if (ctx.tx.isFieldPresent (sfExpiration))
     {
         if (ctx.tx.getFieldU32 (sfExpiration) == 0)
@@ -43,7 +47,7 @@ CreateTicket::preflight (PreflightContext const& ctx)
         }
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2 (ctx);
 }
 
 STAmount

src/ripple/app/tx/impl/Payment.cpp

@@ -41,6 +41,10 @@ allowDeliverMin (ApplyView const& view)
 TER
 Payment::preflight (PreflightContext const& ctx)
 {
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     auto& tx = ctx.tx;
     auto& j = ctx.j;
 
@@ -187,7 +191,7 @@ Payment::preflight (PreflightContext const& ctx)
         }
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2 (ctx);
 }
 
 TER

src/ripple/app/tx/impl/SetAccount.cpp

@@ -31,6 +31,10 @@ namespace ripple {
 TER
 SetAccount::preflight (PreflightContext const& ctx)
 {
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     auto& tx = ctx.tx;
     auto& j = ctx.j;
 
@@ -99,7 +103,7 @@ SetAccount::preflight (PreflightContext const& ctx)
         }
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2(ctx);
 }
 
 TER

src/ripple/app/tx/impl/SetRegularKey.cpp

@@ -44,6 +44,10 @@ SetRegularKey::calculateBaseFee ()
 TER
 SetRegularKey::preflight (PreflightContext const& ctx)
 {
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     std::uint32_t const uTxFlags = ctx.tx.getFlags ();
 
     if (uTxFlags & tfUniversalMask)
@@ -54,7 +58,7 @@ SetRegularKey::preflight (PreflightContext const& ctx)
         return temINVALID_FLAG;
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2(ctx);
 }
 
 TER

src/ripple/app/tx/impl/SetSignerList.cpp

@@ -80,6 +80,10 @@ SetSignerList::preflight (PreflightContext const& ctx)
         return temDISABLED;
 #endif
 
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     auto const result = determineOperation(ctx.tx, ctx.flags, ctx.j);
     if (std::get<0>(result) != tesSUCCESS)
         return std::get<0>(result);
@@ -105,7 +109,7 @@ SetSignerList::preflight (PreflightContext const& ctx)
         }
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2 (ctx);
 }
 
 TER

src/ripple/app/tx/impl/SetTrust.cpp

@@ -30,6 +30,10 @@ namespace ripple {
 TER
 SetTrust::preflight (PreflightContext const& ctx)
 {
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     auto& tx = ctx.tx;
     auto& j = ctx.j;
 
@@ -79,7 +83,7 @@ SetTrust::preflight (PreflightContext const& ctx)
         return temDST_NEEDED;
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2 (ctx);
 }
 
 TER

src/ripple/app/tx/impl/SusPay.cpp

@@ -109,7 +109,7 @@ namespace ripple {
     SusPayCancel
 
         Any account may submit a SusPayCancel transaction.
-        
+
         If the SusPay ledger entry does not specify a
         sfCancelAfter, the cancel transaction will fail.
 
@@ -136,6 +136,10 @@ SusPayCreate::preflight (PreflightContext const& ctx)
             ctx.config.features))
         return temDISABLED;
 
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     if (! isXRP(ctx.tx[sfAmount]))
         return temBAD_AMOUNT;
 
@@ -150,7 +154,7 @@ SusPayCreate::preflight (PreflightContext const& ctx)
             ctx.tx[sfCancelAfter] <= ctx.tx[sfFinishAfter])
         return temBAD_EXPIRATION;
 
-    return Transactor::preflight(ctx);
+    return preflight2 (ctx);
 }
 
 TER
@@ -187,12 +191,12 @@ SusPayCreate::doApply()
     auto const sle = ctx_.view().peek(
         keylet::account(account));
 
-    if (XRPAmount(STAmount((*sle)[sfBalance]).mantissa()) < 
+    if (XRPAmount(STAmount((*sle)[sfBalance]).mantissa()) <
         XRPAmount(ctx_.view().fees().accountReserve(
             (*sle)[sfOwnerCount] + 1)))
         return tecINSUFFICIENT_RESERVE;
 
-    if (XRPAmount(STAmount((*sle)[sfBalance]).mantissa()) < 
+    if (XRPAmount(STAmount((*sle)[sfBalance]).mantissa()) <
         amount + XRPAmount(ctx_.view().fees().accountReserve(
             (*sle)[sfOwnerCount] + 1)))
         return tecUNFUNDED;
@@ -239,7 +243,7 @@ SusPayCreate::doApply()
     (*sle)[sfBalance] = (*sle)[sfBalance] - ctx_.tx[sfAmount];
     (*sle)[sfOwnerCount] = (*sle)[sfOwnerCount] + 1;
     ctx_.view().update(sle);
-    
+
     return tesSUCCESS;
 }
 
@@ -253,6 +257,10 @@ SusPayFinish::preflight (PreflightContext const& ctx)
             ctx.config.features))
         return temDISABLED;
 
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
     if (ctx.tx[~sfMethod])
     {
         // Condition
@@ -291,7 +299,7 @@ SusPayFinish::preflight (PreflightContext const& ctx)
             return temMALFORMED;
     }
 
-    return Transactor::preflight(ctx);
+    return preflight2 (ctx);
 }
 
 TER
@@ -362,7 +370,11 @@ SusPayCancel::preflight (PreflightContext const& ctx)
             ctx.config.features))
         return temDISABLED;
 
-    return Transactor::preflight(ctx);
+    auto const ret = preflight1 (ctx);
+    if (!isTesSuccess (ret))
+        return ret;
+
+    return preflight2 (ctx);
 }
 
 TER

src/ripple/app/tx/impl/Transactor.cpp

@@ -30,30 +30,42 @@
 
 namespace ripple {
 
+/** Performs early sanity checks on the account and fee fields */
 TER
-Transactor::preflight (PreflightContext const& ctx)
+preflight1 (PreflightContext const& ctx)
 {
-    auto& tx = ctx.tx;
+    auto const id = ctx.tx.getAccountID(sfAccount);
-    auto& j = ctx.j;
-
-    auto const id = tx.getAccountID(sfAccount);
     if (id == zero)
     {
-        JLOG(j.warning) << "Transactor::preflight: bad account id";
+        JLOG(ctx.j.warning) << "preflight1: bad account id";
         return temBAD_SRC_ACCOUNT;
     }
 
+    // No point in going any further if the transaction fee is malformed.
+    auto const fee = ctx.tx.getTransactionFee ();
+    if (!fee.native () || fee < beast::zero || !isLegalNet (fee))
+    {
+        JLOG(ctx.j.debug) << "preflight1: invalid fee";
+        return temBAD_FEE;
+    }
+
+    return tesSUCCESS;
+}
+
+/** Checks whether the signature appears valid */
+TER
+preflight2 (PreflightContext const& ctx)
+{
     // Extract signing key
     // Transactions contain a signing key.  This allows us to trivially verify a
     // transaction has at least been properly signed without going to disk.
     // Each transaction also notes a source account id. This is used to verify
     // that the signing key is associated with the account.
     // XXX This could be a lot cleaner to prevent unnecessary copying.
-    auto const pk =
+    auto const pk = RippleAddress::createAccountPublic(
-        RippleAddress::createAccountPublic(
+        ctx.tx.getSigningPubKey());
-            tx.getSigningPubKey());
 
-    if(! ctx.verify(tx,
+    if(! ctx.verify(ctx.tx,
         [&, ctx] (STTx const& tx)
         {
             return (ctx.flags & tapNO_CHECK_SIGN) ||
@@ -66,7 +78,7 @@ Transactor::preflight (PreflightContext const& ctx)
                 );
         }))
     {
-        JLOG(j.debug) << "apply: Invalid transaction (bad signature)";
+        JLOG(ctx.j.debug) << "preflight2: bad signature";
         return temINVALID;
     }
 
@@ -202,12 +214,6 @@ void Transactor::preCompute ()
 
 TER Transactor::apply ()
 {
-    // No point in going any further if the transaction fee is malformed.
-    STAmount const saTxnFee = tx().getTransactionFee ();
-
-    if (!saTxnFee.native () || saTxnFee.negative () || !isLegalNet (saTxnFee))
-        return temBAD_FEE;
-
     preCompute();
 
     // Find source account

src/ripple/app/tx/impl/Transactor.h

@@ -87,10 +87,6 @@ public:
         return ctx_.tx;
     }
 
-    static
-    TER
-    preflight (PreflightContext const& ctx);
-
 protected:
     TER
     apply();
@@ -124,6 +120,14 @@ private:
     TER checkMultiSign ();
 };
 
+/** Performs early sanity checks on the account and fee fields */
+TER
+preflight1 (PreflightContext const& ctx);
+
+/** Checks whether the signature appears valid */
+TER
+preflight2 (PreflightContext const& ctx);
+
 }
 
 #endif