Ripple - Single Asset Vault - Smart Contract Assessment - Ripple

The vault implementation does not properly handle IOU transfer fees during deposit/withdraw operations, which can result in unexpected negative balances for users. When an IOU has a transfer fee (e.g., 25%), the vault calculates shares based on the pre-fee amount but the actual transferred amount is reduced by the fee:

testcase("Deposit transfer fee");
env(rate(issuer, 1.25));  // Set 25% transfer fee
env.close();

// Alice deposits 100 USD into vault
auto txDeposit = vault.deposit({
    .depositor = alice,
    .id = keylet.key,
    .amount = asset(100)
});
env(txDeposit);
env.close(); 
// @audit balance becomes negative (-25 USD) due to fee not being accounted for

1. User attempts to deposit 100 USD

2. 25% transfer fee is applied (25 USD)

3. Only 75 USD actually reaches the vault

4. But the vault calculates shares based on 100 USD

5. Results in a negative balance for the user

Impact

• Users can end up with unexpected negative balances

• Incorrect share allocation relative to the actual deposited amount

• Possible economic imbalance between assets and shares

        using namespace test::jtx;
        testcase("Test IOU fees");

        Env env{*this};
        Account const alice{"alice"};
        Account const issuer{"issuer"};
        env.fund(XRP(1000), alice, issuer);
        env.close();

        auto asset = issuer["USD"];
        auto vault = env.vault();
        
        auto [tx, keylet] = vault.create({.owner = alice, .asset = asset});
        env(tx);
        env.close();
        env.trust(asset(1000), alice);
        env.close();
        
        env(pay(issuer, alice, asset(100)));
        env.close();
        std::cout << "aliceInitialBalance: " << env.balance(alice, asset) << std::endl;

        {
            testcase("Deposit transfer fee");
            
            env(rate(issuer, 1.25));
            env.close();

            // Alice deposits 100 USD into vault
            auto txDeposit = vault.deposit({
                .depositor = alice,
                .id = keylet.key,
                .amount = asset(100)
            });
            env(txDeposit);
            env.close();             
            std::cout << "vault after deposit: " << *env.le(keylet) << std::endl;
        }

• Handle transfer fees in deposit/withdraw calculations

• Add validation to prevent negative balance scenarios

SOLVED: The Ripple team implemented enforcement measures to ensure that the account balance never falls below 0.

Multiple instances of unsafe arithmetic operations have been identified across the vault management transactions that could lead to integer overflow or underflow conditions. These operations directly modify critical vault state variables without proper safeguards.

vault->at(sfAssetTotal) -= assets;    // Possible underflow
vault->at(sfAssetAvailable) -= assets;

vault->at(sfAssetTotal) += assets;    // Possible overflow
vault->at(sfAssetAvailable) += assets;

vault->at(sfAssetTotal) -= assets;    // Possible underflow
vault->at(sfAssetAvailable) -= assets;

assetTotal -= vault->at(sfLossUnrealized); // Could underflow if loss > assetTotal

Implement safe arithmetic operations across all vault transactions.

NOT APPLICABLE: The Ripple team stated the following:

"The type used on both sides of -= and += is Number, which implements checks for overflow and underflow and will throw an exception in such a situation. The exception will be intercepted in the transactor and turned into a well-defined transaction error tefEXCEPTION and the transaction will be aborted. I think a unit test for such a case might be impossible, as it would require the total issuance of the asset to exceed the Number limits."

The current implementation lacks validation to prevent the creation of private vaults for XRP native assets, which contradicts the protocol specification. According to the specification, XRP vaults cannot be private, but there is no enforcement of this rule in the code.

        Env env(*this);
        Account const alice{"alice"};
        env.fund(XRP(10000), alice);
        env.close();

        auto vault = env.vault();
        Asset xrpAsset = xrpIssue();

        // Try to create vault with XRP and private flag
        auto [tx, keylet] = vault.create({
            .owner = alice,
            .asset = xrpAsset,
        });
        tx[sfFlags] = tfVaultPrivate;

        // Should fail since XRP vaults cannot be private
        env(tx, ter(temMALFORMED));
        env.close();

Add validation in the VaultCreate transaction's preflight or preclaim check to prevent creation of private vaults for XRP.

RISK ACCEPTED: The Ripple team stated the following:

"Seem to be based on an obsolete version of XLS-0065d or perhaps pre-proposal discussion. Creating private vaults for native XRP is allowed, by design."

In VaultCreate, there is no validation to prevent setting contradictory flags during vault creation. Specifically, the code allows setting both tfVaultPrivate and tfVaultShareNonTransferable flags simultaneously, which creates a logical contradiction in the vault's behavior.

Setting both private and non-transferable flags creates an ambiguous state:

1. Private vaults require authorization for transfers

2. Non-transferable shares cannot be transferred at all

        testcase("Test contradictory flags");
        {
            Env env(*this);
            Account const alice{"alice"};
            Account const issuer{"issuer"};
            env.fund(XRP(10000), alice, issuer);
            env.close();

            auto vault = env.vault();
            Asset iouAsset = issuer["USD"];

            // Try to create vault with both private and non-transferable flags
            auto [tx, keylet] = vault.create({
                .owner = alice,
                .asset = iouAsset,
            });
            tx[sfFlags] = tfVaultPrivate | tfVaultShareNonTransferable;

            // Should fail since private vaults must have transferable shares
            env(tx, ter(temMALFORMED));
            env.close();
        }

Add validation in the preflight or preclaim check to prevent setting contradictory flags.

RISK ACCEPTED: The Ripple team stated the following:

"Flags tfVaultPrivate and tfVaultShareNonTransferable are not mutually exclusive. The authorization for tfVaultPrivate is enforced only in VaultDeposit, while tfVaultShareNonTransferable is enforced on all transfers of MPT shares of the vault (other than back to the vault, since the flag does not apply to the issuer). This means that, if shares are non-transferable and the vault is private, only the original investors can withdraw assets back from the vault, and only using the amount of MPT shares they originally received in exchange for their assets. This means the vault functionality is limited, but it does not make the vault useless as such. If the vault is private, but shares are transferable, then the original investors can transfer the MPT shares to someone else (who can also transfer the shares etc.) and the ultimate holder of the shares will be able to withdraw the assets (matching the amount of shares held) from the vault, despite not being allowed to invest themselves. This is by design."

In VaultWithdraw, there is no validation to enforce the tfVaultShareNonTransferable flag restriction before transferring shares. According to the specification in section 1.1.3.

The accountSend function doesn't have checks for tfVaultShareNonTransferable.

Add validation to check the non-transferable flag before allowing share transfers.

RISK ACCEPTED: The Ripple team stated the following:

"Flag tfVaultShareNonTransferable does not impact transfer of shares to/from the issuer, which in this case is the vault pseudo-account, so there is nothing to enforce. The purpose of the flag is to enforce that the MPT shares of the vault cannot be transferred by the investors other than back to the vault, in which case the investors receive a matching amount of assets back. Similarly, the withdraw operation now allows the assets to be sent to a 3rd party via Destination field, without regard to this flag. This is by design."

In the Vault withdrawal implementation, authorization checks are performed late in the transaction processing flow via accountSend rather than being rejected at an early stage.
Currently, unauthorized withdrawal attempts are processed through multiple stages of transaction validation before being rejected, rather than being caught early in the preflight or preclaim stages. This creates unnecessary processing overhead.

Move authorization checks to the preflight or preclaim stage in VaultWithdraw.

SOLVED: The Ripple team introduced extra checks on VaultWithdraw.