Ripple: XRPL Permission Delegation

Summary

An account that has been delegated any permissions can delete their account, making any delegated permissions irrevocable while the account does not exist. This account can be later recreated, reaccessing the delegated permissions that were irrevocable.

Finding Description

Let's consider this scenario:

1. Bob (sfAccount) delegates a set of permissions to Alice (sfAuthorize).
2. Alice deletes her account
3. Bob wants to revoke all delegated permissions from Alice

Alice is allowed to delete her account while being a delegate for Bob. The Delegate object doesn't exist on Alice's account, and delegations are non-blocking for deletion anyway.

The Delegate object exists in Bob's account dir. If Alice deletes her account, then the delegations in Alice's dir are deleted at src/libxrpl/tx/transactors/delegate/DelegateSet.cpp?lines=117,117, but her delegate permissions from Bob are not deleted on Bob's account.

Bob now tries to remove Alice as a delegate by submitting an empty DelegateSet for Alice. Because the existence of Alice's account is checked at src/libxrpl/tx/transactors/delegate/DelegateSet.cpp?lines=41,42 in the preclaim function, this would mean that Bob cannot remove Alice's delegated permissions after Alice has deleted her account.

Impact

This has a few consequences:

1. The delegate account has the power to retain their permissions regardless of the delegator's wishes. This itself is a high-severity problem. The delegator MUST have control over all delegated permissions at all times.
2. The irrevocable delegation holds hostage reserve on the delegator account.
3. Regardless of permission irrevocability, the delegate account can delete and re-create it's account, retaining all delegate permissions. This can have unintended consequences if developers rely on account state for delegation.

Likelihood

The path for performing this is simple and is a valid, normal transaction in the XRP Ledger protocol.

Proof of Concept

I have created a new test case in Delegate_test.cpp that walks through this:

void    testAccountDeleteDelegate()    {        testcase("test deleting delegate account");        using namespace jtx;
        Env env(*this);        Account alice{"alice"};        Account bob{"bob"};        env.fund(XRP(100000), alice, bob);        env.close();
        // Bob sets Alice as delegate.        env(delegate::set(bob, alice, {"Payment"}));        env.close();        BEAST_EXPECT(env.closed()->exists(keylet::delegate(bob.id(), alice.id())));
        for (std::uint32_t i = 0; i < 256; ++i)            env.close();
        auto const aliceBalance = env.balance(alice);        auto const bobBalance = env.balance(bob);
        // Alice deletes account, this will not remove Delegate object.        auto const deleteFee = drops(env.current()->fees().increment);        env(acctdelete(alice, bob), fee(deleteFee));        env.close();
        // Verify Alice's account is deleted.        BEAST_EXPECT(!env.closed()->exists(keylet::account(alice.id())));        BEAST_EXPECT(!env.closed()->exists(keylet::ownerDir(alice.id())));
        // @AUDIT: Delegate object still exists even though the delegate account is deleted.        BEAST_EXPECT(env.closed()->exists(keylet::delegate(bob.id(), alice.id())));
        // @AUDIT: Bob attempts to remove Alice's delegate permissions. This will fail with tecNO_TARGET because the delegate account is deleted, even though the ledger entry still exists.        //env(delegate::set(bob, alice, {}));        //env.close();
        // recreate alice account        env.fund(XRP(10000), alice);        env.close();
        // @AUDIT: Delegate object still exists, and new Alice account is delegate        BEAST_EXPECT(env.closed()->exists(keylet::delegate(bob.id(), alice.id())));
        // Delegate permissions can now be removed successfully        env(delegate::set(bob, alice, {}));        env.close();    }

Recommendation

Proposed solution from the Ripple team - "We'll save the Delegate object on both sides, so when the delegated account is deleted, the object from the delegator's side will be removed as well."

In doing this, we must consider the change in reserve costs as well as the edge cases regarding the self-revocability of delegated permissions.

Summary

A uint16_t overflow in permissionToTxType allows for non-delegable values to be counted as isDelegable().

Finding Description

permissionToTxType performs a static_cast<TxType> on the permissionValue at src/libxrpl/protocol/Permissions.cpp?lines=187,187. This converts the original uint32_t value into a uint16_t, leaving room for overflow. In isDelegable() at src/libxrpl/protocol/Permissions.cpp?lines=155,159, if a value is greater than UINT16_MAX and not a granular permission, then the value will wrap around and be checked against delegableTx_.

One such example (of several) is the value 65550. permissionToTxType will subtract one and then truncate: (65550 - 1) % 65536 = 13. 13 corresponds to ttPAYCHAN_CREATE which is delegable, resulting in isDelegable(65550, ...) returning true.

Impact Explanation

Note that the DelegateSet::preflight function checks for duplicate permissions. I could submit a DelegateSet with permissions [13, 65550, 131085, <any uint32_t X, where X % 65536 == 13>] and skirt around this duplicate check. This does not seem to have any meaningful impact.

Likelihood Explanation

Unlikely due to misbehavior being required by the delegator.

Recommendation

After the granularPermission check in isDelegable() at src/libxrpl/protocol/Permissions.cpp?lines=149,153, return error if the permissionValue is greater than UINT16_MAX.

Finding Description

The DelegateSet preflight function at src/libxrpl/tx/transactors/delegate/DelegateSet.cpp?lines=19,29 resorts to temMALFORMED under any error. These errors are ambiguous and make it more difficult to debug from a user perspective.

Recommendation

Add new tem error codes for each case. Here are some examples:

• if (ctx.tx[sfAccount] == ctx.tx[sfAuthorize]) returns temREDUNDANT or temNO_SELF_DELEGATE
• if (!permissionSet.insert(permission[sfPermissionValue]).second) returns temDUPLICATE_PERMISSION
• if (!Permission::getInstance().isDelegable(permission[sfPermissionValue], ctx.rules)) return temNOT_DELEGABLE

Finding Description

The assert statement should properly reflect the assertion and state "value must exceed the maximum uint16_t value."