diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 08832acde7..01f5b50fb6 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -830,7 +830,7 @@ TRANSACTION(ttDELEGATE_SET, 64, DelegateSet, # include #endif TRANSACTION(ttVAULT_CREATE, 65, VaultCreate, - Delegation::delegable, + Delegation::notDelegable, featureSingleAssetVault, createPseudoAcct | createMPTIssuance | mustModifyVault, ({ @@ -848,7 +848,7 @@ TRANSACTION(ttVAULT_CREATE, 65, VaultCreate, # include #endif TRANSACTION(ttVAULT_SET, 66, VaultSet, - Delegation::delegable, + Delegation::notDelegable, featureSingleAssetVault, mustModifyVault, ({ @@ -863,7 +863,7 @@ TRANSACTION(ttVAULT_SET, 66, VaultSet, # include #endif TRANSACTION(ttVAULT_DELETE, 67, VaultDelete, - Delegation::delegable, + Delegation::notDelegable, featureSingleAssetVault, mustDeleteAcct | destroyMPTIssuance | mustModifyVault, ({ @@ -875,7 +875,7 @@ TRANSACTION(ttVAULT_DELETE, 67, VaultDelete, # include #endif TRANSACTION(ttVAULT_DEPOSIT, 68, VaultDeposit, - Delegation::delegable, + Delegation::notDelegable, featureSingleAssetVault, mayAuthorizeMPT | mustModifyVault, ({ @@ -888,7 +888,7 @@ TRANSACTION(ttVAULT_DEPOSIT, 68, VaultDeposit, # include #endif TRANSACTION(ttVAULT_WITHDRAW, 69, VaultWithdraw, - Delegation::delegable, + Delegation::notDelegable, featureSingleAssetVault, mayDeleteMPT | mayAuthorizeMPT | mustModifyVault, ({ @@ -903,7 +903,7 @@ TRANSACTION(ttVAULT_WITHDRAW, 69, VaultWithdraw, # include #endif TRANSACTION(ttVAULT_CLAWBACK, 70, VaultClawback, - Delegation::delegable, + Delegation::notDelegable, featureSingleAssetVault, mayDeleteMPT | mustModifyVault, ({ @@ -932,7 +932,7 @@ TRANSACTION(ttBATCH, 71, Batch, # include #endif TRANSACTION(ttLOAN_BROKER_SET, 74, LoanBrokerSet, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, createPseudoAcct | mayAuthorizeMPT, ({ {sfVaultID, soeREQUIRED}, @@ -949,7 +949,7 @@ TRANSACTION(ttLOAN_BROKER_SET, 74, LoanBrokerSet, # include #endif TRANSACTION(ttLOAN_BROKER_DELETE, 75, LoanBrokerDelete, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, mustDeleteAcct | mayAuthorizeMPT, ({ {sfLoanBrokerID, soeREQUIRED}, @@ -960,7 +960,7 @@ TRANSACTION(ttLOAN_BROKER_DELETE, 75, LoanBrokerDelete, # include #endif TRANSACTION(ttLOAN_BROKER_COVER_DEPOSIT, 76, LoanBrokerCoverDeposit, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, noPriv, ({ {sfLoanBrokerID, soeREQUIRED}, @@ -972,7 +972,7 @@ TRANSACTION(ttLOAN_BROKER_COVER_DEPOSIT, 76, LoanBrokerCoverDeposit, # include #endif TRANSACTION(ttLOAN_BROKER_COVER_WITHDRAW, 77, LoanBrokerCoverWithdraw, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, mayAuthorizeMPT, ({ {sfLoanBrokerID, soeREQUIRED}, @@ -987,7 +987,7 @@ TRANSACTION(ttLOAN_BROKER_COVER_WITHDRAW, 77, LoanBrokerCoverWithdraw, # include #endif TRANSACTION(ttLOAN_BROKER_COVER_CLAWBACK, 78, LoanBrokerCoverClawback, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, noPriv, ({ {sfLoanBrokerID, soeOPTIONAL}, @@ -999,7 +999,7 @@ TRANSACTION(ttLOAN_BROKER_COVER_CLAWBACK, 78, LoanBrokerCoverClawback, # include #endif TRANSACTION(ttLOAN_SET, 80, LoanSet, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, mayAuthorizeMPT | mustModifyVault, ({ {sfLoanBrokerID, soeREQUIRED}, @@ -1026,7 +1026,7 @@ TRANSACTION(ttLOAN_SET, 80, LoanSet, # include #endif TRANSACTION(ttLOAN_DELETE, 81, LoanDelete, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, noPriv, ({ {sfLoanID, soeREQUIRED}, @@ -1037,7 +1037,7 @@ TRANSACTION(ttLOAN_DELETE, 81, LoanDelete, # include #endif TRANSACTION(ttLOAN_MANAGE, 82, LoanManage, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, // All of the LoanManage options will modify the vault, but the // transaction can succeed without options, essentially making it @@ -1051,7 +1051,7 @@ TRANSACTION(ttLOAN_MANAGE, 82, LoanManage, # include #endif TRANSACTION(ttLOAN_PAY, 84, LoanPay, - Delegation::delegable, + Delegation::notDelegable, featureLendingProtocol, mayAuthorizeMPT | mustModifyVault, ({ {sfLoanID, soeREQUIRED}, diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index 31a394eeeb..cbfa29c215 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -1614,13 +1614,7 @@ class Delegate_test : public beast::unit_test::suite {"CredentialDelete", featureCredentials}, {"NFTokenModify", featureDynamicNFT}, {"PermissionedDomainSet", featurePermissionedDomains}, - {"PermissionedDomainDelete", featurePermissionedDomains}, - {"VaultCreate", featureSingleAssetVault}, - {"VaultSet", featureSingleAssetVault}, - {"VaultDelete", featureSingleAssetVault}, - {"VaultDeposit", featureSingleAssetVault}, - {"VaultWithdraw", featureSingleAssetVault}, - {"VaultClawback", featureSingleAssetVault}}; + {"PermissionedDomainDelete", featurePermissionedDomains}}; // Can not delegate tx if any required feature disabled. { @@ -1660,6 +1654,56 @@ class Delegate_test : public beast::unit_test::suite } } + void + testTxDelegableCount() + { + testcase("Delegable Transactions Completeness"); + + std::size_t delegableCount = 0; + +#pragma push_macro("TRANSACTION") +#undef TRANSACTION + +#define TRANSACTION(tag, value, name, delegable, ...) \ + if (delegable == xrpl::delegable) \ + { \ + delegableCount++; \ + } + +#include + +#undef TRANSACTION +#pragma pop_macro("TRANSACTION") + + // ==================================================================== + // IMPORTANT NOTICE: + // + // If this test fails, it indicates that the 'Delegation::delegable' status + // in transactions.macro has been changed. Delegation allows accounts to act + // on behalf of others, significantly increasing the security surface. + // + // + // To ENSURE any added transaction is safe and compatible with delegation: + // + // 1. Verify that the transaction is intended to be delegable. + // 2. Every standard test case for that transaction MUST be + // duplicated and verified for a Delegated context. + // 3. Ensure that Fee, Reserve, and Signing are correctly handled. + // + // DO NOT modify expectedDelegableCount unless all scenarios, including + // edge cases, have been fully tested and verified. + // ==================================================================== + std::size_t const expectedDelegableCount = 75; + + BEAST_EXPECTS( + delegableCount == expectedDelegableCount, + "\n[SECURITY] New delegable transaction detected!" + "\n Expected: " + + std::to_string(expectedDelegableCount) + + "\n Actual: " + std::to_string(delegableCount) + + "\n Action: Verify security requirements to interact with Delegation feature"); + } + void run() override { @@ -1684,6 +1728,7 @@ class Delegate_test : public beast::unit_test::suite testMultiSignQuorumNotMet(); testPermissionValue(all); testTxRequireFeatures(all); + testTxDelegableCount(); } }; BEAST_DEFINE_TESTSUITE(Delegate, app, xrpl); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 7ae9faf18f..26ec59994d 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -4500,131 +4500,6 @@ class Vault_test : public beast::unit_test::suite } } - void - testDelegate() - { - using namespace test::jtx; - - Env env(*this, testable_amendments()); - Account alice{"alice"}; - Account bob{"bob"}; - Account carol{"carol"}; - - struct CaseArgs - { - PrettyAsset asset = xrpIssue(); - }; - - auto const xrpBalance = [this]( - Env const& env, Account const& account) -> std::optional { - auto sle = env.le(keylet::account(account.id())); - if (BEAST_EXPECT(sle != nullptr)) - return sle->getFieldAmount(sfBalance).xrp().drops(); - return std::nullopt; - }; - - auto testCase = [&, this](auto test, CaseArgs args = {}) { - Env env{*this, testable_amendments() | featureSingleAssetVault}; - - Vault vault{env}; - - // use different initial amount to distinguish the source balance - env.fund(XRP(10000), alice); - env.fund(XRP(20000), bob); - env.fund(XRP(30000), carol); - env.close(); - - env(delegate::set( - carol, - alice, - {"Payment", - "VaultCreate", - "VaultSet", - "VaultDelete", - "VaultDeposit", - "VaultWithdraw", - "VaultClawback"})); - - test(env, vault, args.asset); - }; - - testCase([&, this](Env& env, Vault& vault, PrettyAsset const& asset) { - testcase("delegated vault creation"); - auto startBalance = xrpBalance(env, carol); - if (!BEAST_EXPECT(startBalance.has_value())) - return; - - auto [tx, keylet] = vault.create({.owner = carol, .asset = asset}); - env(tx, delegate::as(alice)); - env.close(); - BEAST_EXPECT(xrpBalance(env, carol) == *startBalance); - }); - - testCase([&, this](Env& env, Vault& vault, PrettyAsset const& asset) { - testcase("delegated deposit and withdrawal"); - auto [tx, keylet] = vault.create({.owner = carol, .asset = asset}); - env(tx); - env.close(); - - auto const amount = 1513; - auto const baseFee = env.current()->fees().base; - - auto startBalance = xrpBalance(env, carol); - if (!BEAST_EXPECT(startBalance.has_value())) - return; - - tx = vault.deposit({.depositor = carol, .id = keylet.key, .amount = asset(amount)}); - env(tx, delegate::as(alice)); - env.close(); - BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - amount); - - tx = - vault.withdraw({.depositor = carol, .id = keylet.key, .amount = asset(amount - 1)}); - env(tx, delegate::as(alice)); - env.close(); - BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - 1); - - tx = vault.withdraw({.depositor = carol, .id = keylet.key, .amount = asset(1)}); - env(tx); - env.close(); - BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - baseFee); - }); - - testCase([&, this](Env& env, Vault& vault, PrettyAsset const& asset) { - testcase("delegated withdrawal same as base fee and deletion"); - auto [tx, keylet] = vault.create({.owner = carol, .asset = asset}); - env(tx); - env.close(); - - auto const amount = 25537; - auto const baseFee = env.current()->fees().base; - - auto startBalance = xrpBalance(env, carol); - if (!BEAST_EXPECT(startBalance.has_value())) - return; - - tx = vault.deposit({.depositor = carol, .id = keylet.key, .amount = asset(amount)}); - env(tx); - env.close(); - BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - amount - baseFee); - - tx = vault.withdraw({.depositor = carol, .id = keylet.key, .amount = asset(baseFee)}); - env(tx, delegate::as(alice)); - env.close(); - BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - amount); - - tx = vault.withdraw( - {.depositor = carol, .id = keylet.key, .amount = asset(amount - baseFee)}); - env(tx, delegate::as(alice)); - env.close(); - BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - baseFee); - - tx = vault.del({.owner = carol, .id = keylet.key}); - env(tx, delegate::as(alice)); - env.close(); - }); - } - void testVaultClawbackBurnShares() { @@ -5374,7 +5249,6 @@ public: testFailedPseudoAccount(); testScaleIOU(); testRPC(); - testDelegate(); testVaultClawbackBurnShares(); testVaultClawbackAssets(); testAssetsMaximum();