Ripple - Smart Contract Audit - Credentials - Ripple

An account that previously set its Regular Key equal to its Master Key before the activation of the fixMasterKeyAsRegularKey amendment can still sign transactions using the Master Key even after explicitly disabling it. This occurs because the Master Key remains valid through the Regular Key mechanism.

This issue affects all transaction types, as accounts that believe they have disabled their Master Key can still sign transactions unintentionally. For example, in credential management transactions, an account that has disabled its Master Key may still sign CredentialCreate, CredentialAccept, or CredentialDelete transactions, despite assuming it has removed its ability to authorize such actions.

Because Transactor::checkSingleSign() does not properly reject a disabled Master Key when it is also used as a Regular Key, affected accounts may unknowingly retain unauthorized signing capabilities:

    if (ctx.view.rules().enabled(fixMasterKeyAsRegularKey))
    {
        // Signed with regular key.
        if ((*sleAccount)[~sfRegularKey] == idSigner)
        {
            // ❌ Allows signing with RegularKey even 
            // if it's the disabled Master Key
            return tesSUCCESS;
        }

        // Signed with enabled mater key.
        if (!isMasterDisabled && idAccount == idSigner)
        {
            return tesSUCCESS;
        }

        // Signed with disabled master key.
        if (isMasterDisabled && idAccount == idSigner)
        {
            return tefMASTER_DISABLED;
        }

        // Signed with any other key.
        return tefBAD_AUTH;
    }

It is recommended to modify Transactor::checkSingleSign to explicitly reject transactions signed with a disabled Master Key, even if it is also set as the Regular Key. The validation should ensure that once the Master Key is disabled (via lsfDisableMaster), it cannot be used for signing in any way, preventing unintended access through the Regular Key mechanism. However, to allow affected legacy accounts to recover, SetRegularKey transactions should be exempted from this restriction.

RISK ACCEPTED: The Ripple team accepted this risk and stated the following:

Only 23 accounts out of approximately 6 million are affected by this issue, so the complexity of implementing the solution, including the necessary exception, may not be justified.

The codebase extensively uses assert() statements, which are disabled in production builds when NDEBUG is defined. This can result in silent failures, undefined behavior, and inconsistent ledger states since conditions enforced during development may not be validated in production. Additionally, assertions are used in transaction processing, ledger operations, and security-sensitive functions, which can introduce unintended side effects when they are omitted.

For example, the following assertion ensures a pointer is not null, but in a production build, it would be skipped, potentially leading to a segmentation fault or undefined behavior:

TER
Transactor::consumeSeqProxy(SLE::pointer const& sleAccount)
{
    assert(sleAccount);
    SeqProxy const seqProx = ctx_.tx.getSeqProxy();
    if (seqProx.isSeq())
    {
        // Note that if this transaction is a TicketCreate,
        // then the transaction will modify the account root 
        // sfSequence yet again.
        sleAccount->setFieldU32(sfSequence, seqProx.value() + 1);
        return tesSUCCESS;
    }
    return ticketDelete(
        view(), account_, getTicketIndex(account_, seqProx), j_);
}

Assertions should be replaced with explicit error handling that operates in all build configurations. Instead of using assert(), apply conditionals with proper logging and error responses. For instance, use if (!condition) { return errorCode; } along with appropriate logging mechanisms to ensure that failures are captured and handled correctly.

RISK ACCEPTED: The Ripple team accepted this risk and stated the following:

We don't know why these asserts are left in the code, may be for some debug purpose. Their presence is useless, the code is constructed in such a way that those asserts will never fire (nor debug either release).

When Deposit Auth is not configured for a recipient, transactions that include credentials behave inconsistently across multiple functions. The functions DeleteAccount::doApply(), EscrowFinish::doApply(), PayChanClaim::doApply(), and Payment::doApply() validate if featureDepositAuth is enabled by checking ctx_.view().rules().enabled(featureDepositAuth) before invoking verifyDepositPreauth. However, these functions do not verify whether the recipient has actually configured Deposit Auth.

As a result:

• If valid credentials are included, the transaction succeeds.

• If expired or invalid credentials are included, the transaction fails.

This partial validation leads to unintended behavior, where credentials influence transaction outcomes even when Deposit Auth is not set by the recipient:

if (ctx_.view().rules().enabled(featureDepositAuth)) {
    // Called without confirming if Deposit Auth
    // is configured by the recipient
    verifyDepositPreauth(...); 
}

Update the functions DeleteAccount::doApply(), EscrowFinish::doApply(), PayChanClaim::doApply(), and Payment::doApply() to include a check that confirms whether the recipient has configured Deposit Auth before invoking verifyDepositPreauth. If Deposit Auth is not configured, credentials should be ignored entirely to ensure consistent transaction behavior.

RISK ACCEPTED: The Ripple team accepted this risk and stated the following:

It's intentional, for a better user experience.

The function authorized in the CredentialHelpers.cpp incorrectly returns tefINTERNAL when a transaction includes duplicate credential entries with the same (Issuer, CredentialType). This error code is meant for internal failures but is being used for a user-input validation failure, which can mislead users and make debugging difficult. The appropriate response should be tefBAD_LEDGER, which indicates an unexpected state in the ledger caused by invalid transaction data.

At the end of the function, the issue occurs when inserting credentials into the std::set, where a duplicate entry results in tefINTERNAL:

auto [it, ins] =
    sorted.emplace((*sleCred)[sfIssuer],
        (*sleCred)[sfCredentialType]);
if (!ins)
    return tefINTERNAL;

The function should return tefBAD_LEDGER instead of tefINTERNAL when detecting duplicate credential entries. This ensures that the error properly reflects a ledger state issue rather than an internal failure, improving debugging clarity and user feedback.

ACKNOWLEDGED: The Ripple team acknowledged this issue and stated the following:

It returns tefINTERNAL because this function is only used in the apply stage, where the incoming credentials array has already been checked for duplicates. tefINTERNAL represents an impossible scenario that should never occur, so this behavior is expected and correct.

The jvParseLedger function in RPCCall.cpp fails to properly handle invalid ledger_index values, defaulting to 0 instead of rejecting them. When a user provides an out-of-range or malformed ledger index—such as -1 or 4294967297—the function attempts to convert it using beast::lexicalCast<std::uint32_t>. If the conversion fails, it silently assigns 0, which does not exist in the XRP Ledger:

    // TODO New routine for parsing ledger parameters,
    // other routines should standardize on this.
    static bool
    jvParseLedger(Json::Value& jvRequest,
        std::string const& strLedger)
    {
        if (strLedger == "current" || strLedger == "closed" ||
            strLedger == "validated")
        {
            jvRequest[jss::ledger_index] = strLedger;
        }
        else if (strLedger.length() == 64)
        {
            // YYY Could confirm this is a uint256.
            jvRequest[jss::ledger_hash] = strLedger;
        }
        else
        {
            jvRequest[jss::ledger_index] =
                beast::lexicalCast<std::uint32_t>(strLedger);
        }

        return true;
    }

This behavior results in misleading error messages when transactions or queries reference an invalid ledger, complicating debugging and user interactions. While this does not pose a direct security concern, it can lead to confusion and inefficient troubleshooting due to inaccurate error reporting.

It is recommended to explicitly validate ledger_index values, ensuring they are within the allowed range (starting from 1). If an invalid value is detected, return a descriptive error instead of defaulting to 0.

SOLVED: The Ripple team solved the issue in the specified PR.

The code checks whether a credential is expired only in the doApply() phase of certain transactions (e.g., CredentialCreate::doApply and CredentialAccept::doApply), but does not verify expiration during preclaim. As a result, transactions involving an expired credential are allowed to pass preclaim, only to fail later in doApply. This inconsistency consumes validator and node resources unnecessarily and delays rejection of invalid transactions.

An attacker (or careless user) could submit numerous transactions referencing obviously expired credentials. Because no expiration check is done in preclaim, these transactions are placed in the nodes mempools. Although each transaction will eventually fail in doApply, it still requires processing overhead, adds queue latency, and costs the transaction fee payer extra XRP.

Code Location

• Expiration Check in CredentialAccept::doApply: the check happens only at the final stage.

if (checkExpired(sleCred, view().info().parentCloseTime))
{
    JLOG(j_.trace()) << "Credential is expired: " 
        << sleCred->getText();
    // delete expired credentials even if the transaction failed
    auto const err = credentials::deleteSLE(view(),
        sleCred, j_);
    return isTesSuccess(err) ? tecEXPIRED : err;
}

• Expiration Check in CredentialCreate::doApply: the check happens only at the final stage.

if (closeTime > *optExp)
{
    JLOG(j_.trace()) << "Malformed transaction: "
                        "Expiration time is in the past.";
    return tecEXPIRED;
}

It is recommended to add an expiration check in preclaim for any transactions that depend on non-expired credentials. If the credential is expired, return tecEXPIRED (or an analogous code) immediately, preventing it from entering the transaction queue in the first place. This ensures prompt rejection of invalid transactions, saving network and node resources.

ACKNOWLEDGED: The Ripple team acknowledged this issue and stated the following:

It is because of deletion of the expired credentials (which is not possible in the preclaim).

When creating a credential (CredentialCreate), the logic checks for duplicates of the same credential type on the same subject, returning tecDUPLICATE if one already exists. However, it does not check whether the existing credential is expired. Consequently, an expired credential with the same type blocks the creation of a newer, valid credential. This can lead to a situation in which the subject is effectively unable to refresh or replace an expired credential without manual credential deletion.

Code Location

• In CredentialCreate: the function refuses to recreate a credential if it already exists, not checking if the current credential was expired.

if (ctx.view.exists(
    keylet::credential(subject, ctx.tx[sfAccount], credType)))
{
    JLOG(ctx.j.trace()) << "Credential already exists.";
    return tecDUPLICATE;
}

During the credential creation, when detecting an preexisting credential, it is recommended to verify whether it is expired, and in that case allow the new credential to overwrite or replace the old one without requiring explicit deletion.

ACKNOWLEDGED: The Ripple team acknowledged this issue and stated the following:

It is not recognized as bug, as it implemented as designed. However it seems like a feature that worth to discuss and we may return to it.