Skip to content

NestedMultiSign #675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RichardAH wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

NestedMultiSign #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
393 changes: 275 additions & 118 deletions src/ripple/app/tx/impl/Transactor.cpp
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 90;
static constexpr std::size_t numFeatures = 91;

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -378,6 +378,7 @@ extern uint256 const fixInvalidTxFlags;
extern uint256 const featureExtendedHookState;
extern uint256 const fixCronStacking;
extern uint256 const fixHookAPI20251128;
extern uint256 const featureNestedMultiSign;
} // namespace ripple

#endif
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ REGISTER_FIX (fixInvalidTxFlags, Supported::yes, VoteBehavior::De
REGISTER_FEATURE(ExtendedHookState, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixCronStacking, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixHookAPI20251128, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(NestedMultiSign, Supported::yes, VoteBehavior::DefaultNo);

// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.
Expand Down
5 changes: 3 additions & 2 deletions src/ripple/protocol/impl/InnerObjectFormats.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ InnerObjectFormats::InnerObjectFormats()
sfSigner.getCode(),
{
{sfAccount, soeREQUIRED},
{sfSigningPubKey, soeREQUIRED},
{sfTxnSignature, soeREQUIRED},
{sfSigningPubKey, soeOPTIONAL},
Copy link

@shortthefomo shortthefomo Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making sfSigningPubKey and sfTxnSignature optional changes validation for ALL signers, not just nested
The change in InnerObjectFormats.cpp:47-49 from soeREQUIRED to soeOPTIONAL for sfSigningPubKey and sfTxnSignature in the sfSigner inner object format is a global change that affects all signer deserialization, not just when featureNestedMultiSign is enabled. This means that even without the amendment enabled, a malformed signer object missing sfSigningPubKey or sfTxnSignature will now successfully deserialize (whereas before it would throw during applyTemplate). The runtime checks in STTx::checkMultiSign (STTx.cpp:445-449) and Transactor::checkMultiSign (Transactor.cpp:1101-1107) do validate that leaf signers have these fields, so this is caught later. However, the test in STTx_test.cpp:1797-1803 (Test case 2) had to be commented out because it tested that deserialization itself would reject a signer without sfSigningPubKey. This is a defense-in-depth regression—invalid data that was previously rejected at the serialization layer now passes through to application-level validation.

{sfTxnSignature, soeOPTIONAL},
{sfSigners, soeOPTIONAL},
});

add(sfMajority.jsonName.c_str(),
Expand Down
148 changes: 104 additions & 44 deletions src/ripple/protocol/impl/STTx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,64 +369,124 @@ STTx::checkMultiSign(
bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) ||
(requireCanonicalSig == RequireFullyCanonicalSig::yes);

// Signers must be in sorted order by AccountID.
AccountID lastAccountID(beast::zero);

bool const isWildcardNetwork =
isFieldPresent(sfNetworkID) && getFieldU32(sfNetworkID) == 65535;

for (auto const& signer : signers)
{
auto const accountID = signer.getAccountID(sfAccount);
// Set max depth based on feature flag
int const maxDepth = rules.enabled(featureNestedMultiSign) ? 4 : 1;

// The account owner may not multisign for themselves.
if (accountID == txnAccountID)
return Unexpected("Invalid multisigner.");
// Define recursive lambda for checking signatures at any depth
std::function<Expected<void, std::string>(
STArray const&, AccountID const&, int)>
checkSignersArray;

// No duplicate signers allowed.
if (lastAccountID == accountID)
return Unexpected("Duplicate Signers not allowed.");
checkSignersArray = [&](STArray const& signersArray,
AccountID const& parentAccountID,
Copy link
Contributor Author

@RichardAH RichardAH Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused parentAccuontID

int depth) -> Expected<void, std::string> {
// Check depth limit
if (depth > maxDepth)
return Unexpected("Multi-signing depth limit exceeded.");

// Accounts must be in order by account ID. No duplicates allowed.
if (lastAccountID > accountID)
return Unexpected("Unsorted Signers array.");
// There are well known bounds that the number of signers must be
// within.
if (signersArray.size() < minMultiSigners ||
signersArray.size() > maxMultiSigners(&rules))
return Unexpected("Invalid Signers array size.");

// The next signature must be greater than this one.
lastAccountID = accountID;
// Signers must be in sorted order by AccountID.
AccountID lastAccountID(beast::zero);

// Verify the signature.
bool validSig = false;
try
for (auto const& signer : signersArray)
{
Serializer s = dataStart;
finishMultiSigningData(accountID, s);
auto const accountID = signer.getAccountID(sfAccount);

// The account owner may not multisign for themselves.
Copy link

@shortthefomo shortthefomo Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested signer matching parent account is not rejected in STTx::checkMultiSign
In the recursive checkSignersArray lambda in STTx::checkMultiSign, the check accountID == txnAccountID only compares against the top-level transaction account ID, not the parentAccountID at each recursion level. The parentAccountID parameter is passed into the lambda but never used.

Copy link

@shortthefomo shortthefomo Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But parentAccountID (the account being signed for at the current nesting level) is never checked. This means if account B's signer list includes B itself, and B is a nested signer for account A, the recursive call would pass parentAccountID = B but never reject B appearing in its own nested signer array.
In the original non-recursive code, this wasn't an issue because there was only one level. Now with nesting, a signer at depth N could list themselves as a sub-signer at depth N+1, which should be rejected but isn't.
Note: Transactor::checkMultiSign has cycle detection via the ancestors set which would catch this at the preclaim stage, but STTx::checkMultiSign (the preflight signature verification) does not, allowing malformed transactions to pass signature verification and consume more processing resources than necessary.

if (accountID == txnAccountID)
return Unexpected("Invalid multisigner.");

// No duplicate signers allowed.
if (lastAccountID == accountID)
return Unexpected("Duplicate Signers not allowed.");

// Accounts must be in order by account ID. No duplicates allowed.
if (lastAccountID > accountID)
return Unexpected("Unsorted Signers array.");

auto spk = signer.getFieldVL(sfSigningPubKey);
// The next signature must be greater than this one.
lastAccountID = accountID;

if (publicKeyType(makeSlice(spk)))
// Check if this signer has nested signers
if (signer.isFieldPresent(sfSigners))
{
Blob const signature = signer.getFieldVL(sfTxnSignature);

// wildcard network gets a free pass
validSig = isWildcardNetwork ||
verify(PublicKey(makeSlice(spk)),
s.slice(),
makeSlice(signature),
fullyCanonical);
// This is a nested multi-signer
if (maxDepth == 1)
{
// amendment is not enabled, this is an error
return Unexpected("FeatureNestedMultiSign is disabled");
}

// Ensure it doesn't also have signature fields
if (signer.isFieldPresent(sfSigningPubKey) ||
signer.isFieldPresent(sfTxnSignature))
return Unexpected(
"Signer cannot have both nested signers and signature "
"fields.");

// Recursively check nested signers
STArray const& nestedSigners = signer.getFieldArray(sfSigners);
auto result =
checkSignersArray(nestedSigners, accountID, depth + 1);
if (!result)
return result;
}
else
{
// This is a leaf node - must have signature
if (!signer.isFieldPresent(sfSigningPubKey) ||
!signer.isFieldPresent(sfTxnSignature))
return Unexpected(
"Leaf signer must have SigningPubKey and "
"TxnSignature.");

// Verify the signature
bool validSig = false;
try
{
Serializer s = dataStart;
finishMultiSigningData(accountID, s);

auto spk = signer.getFieldVL(sfSigningPubKey);

if (publicKeyType(makeSlice(spk)))
{
Blob const signature =
signer.getFieldVL(sfTxnSignature);

// wildcard network gets a free pass
validSig = isWildcardNetwork ||
verify(PublicKey(makeSlice(spk)),
s.slice(),
makeSlice(signature),
fullyCanonical);
}
}
catch (std::exception const&)
{
// We assume any problem lies with the signature.
validSig = false;
}
if (!validSig)
return Unexpected(
std::string("Invalid signature on account ") +
toBase58(accountID) + ".");
}
}
catch (std::exception const&)
{
// We assume any problem lies with the signature.
validSig = false;
}
if (!validSig)
return Unexpected(
std::string("Invalid signature on account ") +
toBase58(accountID) + ".");
}
// All signatures verified.
return {};

return {};
};

// Start the recursive check at depth 1
return checkSignersArray(signers, txnAccountID, 1);
}

//------------------------------------------------------------------------------
Expand Down
21 changes: 15 additions & 6 deletions src/ripple/rpc/impl/TransactionSign.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,12 +1183,21 @@ transactionSubmitMultiSigned(
// The Signers array may only contain Signer objects.
if (std::find_if_not(
signers.begin(), signers.end(), [](STObject const& obj) {
return (
// A Signer object always contains these fields and no
// others.
obj.isFieldPresent(sfAccount) &&
obj.isFieldPresent(sfSigningPubKey) &&
obj.isFieldPresent(sfTxnSignature) && obj.getCount() == 3);
if (obj.getCount() != 4 || !obj.isFieldPresent(sfAccount))
Copy link

@shortthefomo shortthefomo Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCount() != 4 check in TransactionSign.cpp is correct but non-obvious
The check obj.getCount() != 4 at TransactionSign.cpp:1186 initially appears wrong since a leaf signer has 3 explicit fields and a nested signer has 2. However, this is correct because STObject::applyTemplate (in STObject.cpp:116-153) adds nonPresentObject entries for all optional fields not present in the source data. Since the sfSigner template now has 4 fields (sfAccount required, sfSigningPubKey optional, sfTxnSignature optional, sfSigners optional), getCount() always returns 4 regardless of which optional fields are actually set. The old code used getCount() == 3 because the old template had exactly 3 fields (all required). This is fragile coupling between the validation logic and the template definition—if the template changes again, this hardcoded 4 would silently break.

return false;
Comment on lines -1186 to +1187
Copy link
Contributor Author

@RichardAH RichardAH Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove 4 here

Copy link
Contributor Author

@RichardAH RichardAH Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

        if (!obj.isFieldPresent(sfAccount))
            return false;

        // leaf signer
        if (obj.isFieldPresent(sfSigningPubKey) &&
            obj.isFieldPresent(sfTxnSignature) &&
            !obj.isFieldPresent(sfSigners))
            return obj.getCount() == 3;

        // nested signer
        if (!obj.isFieldPresent(sfSigningPubKey) &&
            !obj.isFieldPresent(sfTxnSignature) &&
            obj.isFieldPresent(sfSigners))
            return obj.getCount == 2;

// leaf signer
if (obj.isFieldPresent(sfSigningPubKey) &&
obj.isFieldPresent(sfTxnSignature) &&
!obj.isFieldPresent(sfSigners))
return true;

// nested signer
if (!obj.isFieldPresent(sfSigningPubKey) &&
!obj.isFieldPresent(sfTxnSignature) &&
obj.isFieldPresent(sfSigners))
return true;

return false;
}) != signers.end())
{
return RPC::make_param_error(
Expand Down
Loading
Loading