diff --git a/include/xrpl/protocol/Fees.h b/include/xrpl/protocol/Fees.h index fc6b2bdb3b..a7ec00ba5b 100644 --- a/include/xrpl/protocol/Fees.h +++ b/include/xrpl/protocol/Fees.h @@ -2,6 +2,8 @@ #include +#include + namespace xrpl { // Deprecated constant for backwards compatibility with pre-XRPFees amendment. @@ -11,6 +13,10 @@ inline constexpr std::uint32_t kFeeUnitsDeprecated = 10; // Number of micro-drops in one drop. constexpr std::uint32_t microDropsPerDrop{1'000'000}; +/** Maximum Feature Extension fee settings. */ +inline constexpr std::uint32_t kMaxExtensionComputeLimit{2'000'000}; +inline constexpr std::uint32_t kMaxExtensionSizeLimit{200'000}; + /** Reflects the fee settings for a particular ledger. The fees are always the same for any transactions applied diff --git a/src/libxrpl/tx/transactors/system/Change.cpp b/src/libxrpl/tx/transactors/system/Change.cpp index 02904682a4..8691834ea3 100644 --- a/src/libxrpl/tx/transactors/system/Change.cpp +++ b/src/libxrpl/tx/transactors/system/Change.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -129,6 +130,9 @@ Change::preclaim(PreclaimContext const& ctx) !ctx.tx.isFieldPresent(sfExtensionSizeLimit) || !ctx.tx.isFieldPresent(sfGasPrice)) return temMALFORMED; + if (ctx.tx[sfExtensionComputeLimit] > kMaxExtensionComputeLimit || + ctx.tx[sfExtensionSizeLimit] > kMaxExtensionSizeLimit) + return temBAD_FEE; } else { diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index ac149156ae..416ae44122 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -305,16 +305,14 @@ class FeeVote_test : public beast::unit_test::Suite "extension_compute_limit = -100", "extension_size_limit = -200", "gas_price = -300"}); - // Negative values wrap to large positive uint32_t values. - // reference_fee is uint64_t and has bounds checking, so it keeps - // the default. - // Illegal values are ignored, and the defaults left unchanged + // Negative extension limit values wrap past their maximum and are + // ignored. Other uint32_t fields keep the existing behavior. auto setup = setupFeeVote(config); BEAST_EXPECT(setup.reference_fee == defaultSetup.reference_fee); BEAST_EXPECT(setup.account_reserve == static_cast(-1234567)); BEAST_EXPECT(setup.owner_reserve == static_cast(-1234)); - BEAST_EXPECT(setup.extension_compute_limit == static_cast(-100)); - BEAST_EXPECT(setup.extension_size_limit == static_cast(-200)); + BEAST_EXPECT(setup.extension_compute_limit == defaultSetup.extension_compute_limit); + BEAST_EXPECT(setup.extension_size_limit == defaultSetup.extension_size_limit); BEAST_EXPECT(setup.gas_price == static_cast(-300)); } { @@ -337,6 +335,24 @@ class FeeVote_test : public beast::unit_test::Suite BEAST_EXPECT(setup.extension_size_limit == defaultSetup.extension_size_limit); BEAST_EXPECT(setup.gas_price == defaultSetup.gas_price); } + { + Section config; + config.append( + {"extension_compute_limit = " + std::to_string(kMaxExtensionComputeLimit + 1), + "extension_size_limit = " + std::to_string(kMaxExtensionSizeLimit + 1)}); + auto setup = setupFeeVote(config); + BEAST_EXPECT(setup.extension_compute_limit == defaultSetup.extension_compute_limit); + BEAST_EXPECT(setup.extension_size_limit == defaultSetup.extension_size_limit); + } + { + Section config; + config.append( + {"extension_compute_limit = " + std::to_string(kMaxExtensionComputeLimit), + "extension_size_limit = " + std::to_string(kMaxExtensionSizeLimit)}); + auto setup = setupFeeVote(config); + BEAST_EXPECT(setup.extension_compute_limit == kMaxExtensionComputeLimit); + BEAST_EXPECT(setup.extension_size_limit == kMaxExtensionSizeLimit); + } } void @@ -433,6 +449,40 @@ class FeeVote_test : public beast::unit_test::Suite BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields)); } + // Test that Smart Escrow limits reject values above their maximums. + { + jtx::Env env(*this, jtx::testableAmendments()); + auto ledger = std::make_shared( + kCreateGenesis, + Rules{env.app().config().features}, + env.app().config().FEES.toFees(), + std::vector{}, + env.app().getNodeFamily()); + + ledger = std::make_shared(*ledger, env.app().getTimeKeeper().closeTime()); + + auto testBadFields = [&](FeeSettingsFields const& fields) { + auto feeTx = createFeeTx(ledger->rules(), ledger->seq(), fields); + OpenView accum(ledger.get()); + BEAST_EXPECT(!isTesSuccess(applyFeeAndTestResult(env, accum, feeTx))); + }; + + testBadFields( + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = kMaxExtensionComputeLimit + 1, + .extensionSizeLimit = kMaxExtensionSizeLimit, + .gasPrice = 300}); + testBadFields( + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = kMaxExtensionComputeLimit, + .extensionSizeLimit = kMaxExtensionSizeLimit + 1, + .gasPrice = 300}); + } + // Test that the Smart Escrow fields are rejected if the // feature is disabled { diff --git a/src/xrpld/app/misc/FeeVoteImpl.cpp b/src/xrpld/app/misc/FeeVoteImpl.cpp index 24ba7fdaaa..5feb95ca82 100644 --- a/src/xrpld/app/misc/FeeVoteImpl.cpp +++ b/src/xrpld/app/misc/FeeVoteImpl.cpp @@ -117,6 +117,12 @@ public: FeeVoteImpl::FeeVoteImpl(FeeSetup const& setup, beast::Journal journal) : target_(setup), journal_(journal) { + XRPL_ASSERT( + target_.extension_compute_limit <= kMaxExtensionComputeLimit, + "xrpl::FeeVoteImpl::FeeVoteImpl : extension compute limit in range"); + XRPL_ASSERT( + target_.extension_size_limit <= kMaxExtensionSizeLimit, + "xrpl::FeeVoteImpl::FeeVoteImpl : extension size limit in range"); } void @@ -287,10 +293,14 @@ FeeVoteImpl::doVoting( { auto doVote = [](std::shared_ptr const& val, detail::VotableValue& value, - SF_UINT32 const& extensionField) { + SF_UINT32 const& extensionField, + std::uint32_t maxValue) { if (auto const field = ~val->at(~extensionField); field) { - value.addVote(field.value()); + if (field.value() <= maxValue) + value.addVote(field.value()); + else + value.noVote(); } else { @@ -302,9 +312,9 @@ FeeVoteImpl::doVoting( { if (!val->isTrusted()) continue; - doVote(val, extensionComputeVote, sfExtensionComputeLimit); - doVote(val, extensionSizeVote, sfExtensionSizeLimit); - doVote(val, gasPriceVote, sfGasPrice); + doVote(val, extensionComputeVote, sfExtensionComputeLimit, kMaxExtensionComputeLimit); + doVote(val, extensionSizeVote, sfExtensionSizeLimit, kMaxExtensionSizeLimit); + doVote(val, gasPriceVote, sfGasPrice, std::numeric_limits::max()); } } diff --git a/src/xrpld/core/detail/Config.cpp b/src/xrpld/core/detail/Config.cpp index 9595766e01..d1666e0e52 100644 --- a/src/xrpld/core/detail/Config.cpp +++ b/src/xrpld/core/detail/Config.cpp @@ -1194,9 +1194,9 @@ setupFeeVote(Section const& section) setup.account_reserve = temp; if (set(temp, "owner_reserve", section)) setup.owner_reserve = temp; - if (set(temp, "extension_compute_limit", section)) + if (set(temp, "extension_compute_limit", section) && temp <= kMaxExtensionComputeLimit) setup.extension_compute_limit = temp; - if (set(temp, "extension_size_limit", section)) + if (set(temp, "extension_size_limit", section) && temp <= kMaxExtensionSizeLimit) setup.extension_size_limit = temp; if (set(temp, "gas_price", section)) setup.gas_price = temp;