fix: Disallow empty permission list when Delegate object is absent (#6542)

This change fixes delegation:
* If the Delegate object is not present, we should disallow empty permission list in DelegateSet preclaim.
* Empty permission list is only allowed to delete the existing Delegate object.
* In `doApply`, permission list being empty returns `tecINTERNAL`, which should not happen.

src/libxrpl/tx/transactors/delegate/DelegateSet.cpp

@@ -41,6 +41,13 @@ DelegateSet::preclaim(PreclaimContext const& ctx)
     if (!ctx.view.exists(keylet::account(ctx.tx[sfAuthorize])))
         return tecNO_TARGET;
 
+    // Deleting the delegate object is invalid if it doesn’t exist.
+    if (ctx.tx.getFieldArray(sfPermissions).empty() &&
+        !ctx.view.exists(keylet::delegate(ctx.tx[sfAccount], ctx.tx[sfAuthorize])))
+    {
+        return tecNO_ENTRY;
+    }
+
     return tesSUCCESS;
 }
 
@@ -67,30 +74,30 @@ DelegateSet::doApply()
         return tesSUCCESS;
     }
 
+    auto const& permissions = ctx_.tx.getFieldArray(sfPermissions);
+    if (permissions.empty())
+        return tecINTERNAL;  // LCOV_EXCL_LINE
+
     STAmount const reserve{
         ctx_.view().fees().accountReserve(sleOwner->getFieldU32(sfOwnerCount) + 1)};
 
     if (preFeeBalance_ < reserve)
         return tecINSUFFICIENT_RESERVE;
 
-    auto const& permissions = ctx_.tx.getFieldArray(sfPermissions);
-    if (!permissions.empty())
-    {
-        sle = std::make_shared<SLE>(delegateKey);
-        sle->setAccountID(sfAccount, account_);
-        sle->setAccountID(sfAuthorize, authAccount);
+    sle = std::make_shared<SLE>(delegateKey);
+    sle->setAccountID(sfAccount, account_);
+    sle->setAccountID(sfAuthorize, authAccount);
 
-        sle->setFieldArray(sfPermissions, permissions);
-        auto const page = ctx_.view().dirInsert(
-            keylet::ownerDir(account_), delegateKey, describeOwnerDir(account_));
+    sle->setFieldArray(sfPermissions, permissions);
+    auto const page =
+        ctx_.view().dirInsert(keylet::ownerDir(account_), delegateKey, describeOwnerDir(account_));
 
-        if (!page)
-            return tecDIR_FULL;  // LCOV_EXCL_LINE
+    if (!page)
+        return tecDIR_FULL;  // LCOV_EXCL_LINE
 
-        (*sle)[sfOwnerNode] = *page;
-        ctx_.view().insert(sle);
-        adjustOwnerCount(ctx_.view(), sleOwner, 1, ctx_.journal);
-    }
+    (*sle)[sfOwnerNode] = *page;
+    ctx_.view().insert(sle);
+    adjustOwnerCount(ctx_.view(), sleOwner, 1, ctx_.journal);
 
     return tesSUCCESS;
 }

src/test/app/Delegate_test.cpp

@@ -45,11 +45,9 @@ class Delegate_test : public beast::unit_test::suite
         env.close();
 
         // delegating an empty permission list when the delegate ledger object
-        // does not exist will not create the ledger object
-        env(delegate::set(gw, alice, std::vector<std::string>{}));
+        // does not exist is not allowed
+        env(delegate::set(gw, alice, {}), ter(tecNO_ENTRY));
         env.close();
-        auto const entry = delegate::entry(env, gw, alice);
-        BEAST_EXPECT(entry[jss::result][jss::error] == "entryNotFound");
 
         auto const permissions = std::vector<std::string>{
             "Payment", "EscrowCreate", "EscrowFinish", "TrustlineAuthorize", "CheckCreate"};
@@ -92,9 +90,7 @@ class Delegate_test : public beast::unit_test::suite
         // newPermissions
         comparePermissions(delegate::entry(env, gw, alice), newPermissions, gw, alice);
 
-        // gw deletes all permissions delegated to alice, this will delete
-        // the
-        // ledger entry
+        // gw deletes all permissions delegated to alice, this will delete the ledger entry
         env(delegate::set(gw, alice, {}));
         env.close();
         auto const jle = delegate::entry(env, gw, alice);
@@ -204,27 +200,48 @@ class Delegate_test : public beast::unit_test::suite
         testcase("test reserve");
         using namespace jtx;
 
-        // test reserve for DelegateSet
+        // reserve requirement not met
         {
             Env env(*this);
             Account alice{"alice"};
             Account bob{"bob"};
-            Account carol{"carol"};
 
-            env.fund(drops(env.current()->fees().accountReserve(0)), alice);
-            env.fund(drops(env.current()->fees().accountReserve(1)), bob, carol);
+            auto const txFee = env.current()->fees().base;
+            env.fund(env.current()->fees().accountReserve(0) + txFee, alice);
+            env.fund(XRP(100000), bob);
             env.close();
 
             // alice does not have enough reserve to create Delegate
             env(delegate::set(alice, bob, {"Payment"}), ter(tecINSUFFICIENT_RESERVE));
+        }
 
-            // bob has enough reserve
-            env(delegate::set(bob, alice, {"Payment"}));
+        // reserve recovered after deleting delegation object
+        {
+            Env env(*this);
+            Account bob{"bob"};
+            Account alice{"alice"};
+            Account carol{"carol"};
+
+            auto const txFee = env.current()->fees().base;
+
+            env.fund(env.current()->fees().accountReserve(1) + (txFee * 4), alice);
+            env.fund(XRP(100000), bob, carol);
             env.close();
 
-            // now bob create another Delegate, he does not have
-            // enough reserve
-            env(delegate::set(bob, carol, {"Payment"}), ter(tecINSUFFICIENT_RESERVE));
+            // alice consumes 1 txFee and requires 1 object reserve
+            env(delegate::set(alice, bob, {"Payment"}));
+            env.close();
+
+            // alice does not have enough reserve to create another delegation object
+            env(delegate::set(alice, carol, {"Payment"}), ter(tecINSUFFICIENT_RESERVE));
+            env.close();
+
+            // deleting delegation object recovers 1 reserve
+            env(delegate::set(alice, bob, {}));
+            env.close();
+
+            // now alice can delegate again
+            env(delegate::set(alice, carol, {"Payment"}));
         }
 
         // test reserve when sending transaction on behalf of other account