From 1589498ae178c0b440d4203e7bc065d2e5ebe71e Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 7 Mar 2025 11:18:25 +0000 Subject: [PATCH] Improve VaultClawback checks --- src/test/app/Vault_test.cpp | 31 ++++++++++++++++----- src/test/jtx/vault.h | 2 +- src/xrpld/app/tx/detail/VaultClawback.cpp | 33 ++++++++++++++++++++--- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index ebb0752fb6..98f245cc02 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -18,14 +18,17 @@ //============================================================================== #include +#include #include #include #include #include +#include #include #include #include #include +#include namespace ripple { @@ -136,6 +139,20 @@ class Vault_test : public beast::unit_test::suite env(tx); } + { + testcase(prefix + " fail to set zero domain"); + auto tx = vault.set({.owner = owner, .id = keylet.key}); + tx[sfDomainID] = to_string(base_uint<256>(beast::zero)); + env(tx, ter(temMALFORMED)); + } + + { + testcase(prefix + " fail to set nonexistent domain"); + auto tx = vault.set({.owner = owner, .id = keylet.key}); + tx[sfDomainID] = to_string(base_uint<256>(42ul)); + env(tx, ter(tecINVALID_DOMAIN)); + } + { testcase(prefix + " fail to deposit more than maximum"); auto tx = vault.deposit( @@ -189,7 +206,7 @@ class Vault_test : public beast::unit_test::suite .id = keylet.key, .holder = depositor, .amount = asset(50)}); - env(tx, ter(tecNO_PERMISSION)); + env(tx, ter(temMALFORMED)); } { @@ -204,8 +221,8 @@ class Vault_test : public beast::unit_test::suite { testcase(prefix + " clawback some"); - auto code = asset.raw().native() ? ter(tecNO_PERMISSION) - : ter(tesSUCCESS); + auto code = + asset.raw().native() ? ter(temMALFORMED) : ter(tesSUCCESS); auto tx = vault.clawback( {.issuer = issuer, .id = keylet.key, @@ -219,10 +236,7 @@ class Vault_test : public beast::unit_test::suite auto code = asset.raw().native() ? ter(tecNO_PERMISSION) : ter(tesSUCCESS); auto tx = vault.clawback( - {.issuer = issuer, - .id = keylet.key, - .holder = depositor, - .amount = std::nullopt}); + {.issuer = issuer, .id = keylet.key, .holder = depositor}); env(tx, code); } @@ -272,6 +286,9 @@ class Vault_test : public beast::unit_test::suite auto vault = env.vault(); env.fund(XRP(1000), issuer, owner, depositor); env.close(); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + env.require(flags(issuer, asfAllowTrustLineClawback)); PrettyAsset asset = setup(env, issuer, depositor); testSequence( diff --git a/src/test/jtx/vault.h b/src/test/jtx/vault.h index abccd67ec7..74c482bf17 100644 --- a/src/test/jtx/vault.h +++ b/src/test/jtx/vault.h @@ -95,7 +95,7 @@ struct Vault Account issuer; uint256 id; Account holder; - std::optional amount; + std::optional amount{}; }; Json::Value diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index 4385e0b6ce..aba8bb67e7 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2022 Ripple Labs Inc. + Copyright (c) 2025 Ripple Labs Inc. Permission to use, copy, modify, and/or distribute this software for any purpose with or without fee is hereby granted, provided that the above @@ -41,10 +41,23 @@ VaultClawback::preflight(PreflightContext const& ctx) if (ctx.tx.getFlags() & tfUniversalMask) return temINVALID_FLAG; - // Note, zero amount is valid, it means "all". It is also the default. + AccountID const issuer = ctx.tx[sfAccount]; + AccountID const holder = ctx.tx[sfHolder]; + + if (issuer == holder) + return temMALFORMED; + auto const amount = ctx.tx[~sfAmount]; - if (amount && *amount < beast::zero) - return temBAD_AMOUNT; + if (amount) + { + // Note, zero amount is valid, it means "all". It is also the default. + if (*amount < beast::zero) + return temBAD_AMOUNT; + else if (isXRP(amount->asset())) + return temMALFORMED; + else if (amount->asset().getIssuer() != issuer) + return temMALFORMED; + } return preflight2(ctx); } @@ -57,6 +70,10 @@ VaultClawback::preclaim(PreclaimContext const& ctx) return tecOBJECT_NOT_FOUND; auto account = ctx.tx[sfAccount]; + auto const issuer = ctx.view.read(keylet::account(account)); + if (!issuer) + return tefINTERNAL; // Transactor should have enforced this + Asset const asset = vault->at(sfAsset); if (asset.native()) return tecNO_PERMISSION; // Cannot clawback XRP. @@ -67,6 +84,14 @@ VaultClawback::preclaim(PreclaimContext const& ctx) if (amount && asset != amount->asset()) return tecWRONG_ASSET; + std::uint32_t const issuerFlags = issuer->getFieldU32(sfFlags); + + // If AllowTrustLineClawback is not set or NoFreeze is set, return no + // permission + if (!(issuerFlags & lsfAllowTrustLineClawback) || + (issuerFlags & lsfNoFreeze)) + return tecNO_PERMISSION; + return tesSUCCESS; }