Improvements to the Wallet Connect implementation for dApps on Hedera

[mgarbs]

Description:
Updates the header to match new format
Updates the technical section to match the proposed implementation
Adds text to the reference implementation

[tmctl]

I think this is a great addition and offers a non-breaking feature addition.

Currently to access multi-sig functionality of the network utilizing wallets connected through WalletConnect, a dApp would need to send multiple transactions to be signed to a wallet, resulting in a user needing to accept and sign multiple transactions of the same intent.

The main reason multiple transactions currently would need to be sent by a dApp and approved in an end user's trusted wallet is because a valid signed transaction needs to have a valid node assigned to it. I.E. a transaction sent to a node that it was not intended for will fail by design. When doing multi-sig, there's an increase priority to sign a transaction for multiple nodes. For example, a transaction could have a valid start time of 2 days from now, giving human users the opportunity to gather multiple signatures required for a multi-sig scenario over a couple of days. If a single, multi-signed transaction happens to be for a node that is no longer available during the valid transaction window (2 or 3 minutes), the transaction will not be able to be successfully sent to the network, despite the intent of the group.

The best practice in a multi-sig scenario is to reduce this risk of not being able to submit a transaction to the network at a specified time period by allowing a transaction's body to be sent to multiple nodes. The unique identifier of a transaction is the valid start timestamp in nanoseconds and the payer account id. So a transaction that only differs in node account id cannot be processes twice.

There is a security consideration of sending a list of valid transactions to a wallet and an end user to sign only once. In a naive approach, a malicious dApp could send a disguised list of transactions to a wallet that are not the same. For example, a payload could be sent to a wallet with two transactions encapsulated in a Transaction object. The first transaction could be a transfer of 1 HBAR to a valid account, and the second transaction could be assigning a delelgated spend of 1 million HBAR to a malicious account. In a naive implementation, the wallet would prompt the user to sign both transactions as 1 and the malicious dapp would submit the malicious transaction.

For this method, it is therefore a trusted wallet's duty, burden, and responsibility to verify the transaction bodies are identical in intent before allowing a user to sign. This fortunately is handled by the @hashgraph/sdk javascript SDK.

To strengthen security assurances, while providing the desired functionality and user experience proposed by this HIP, I propose we add a test in this repository or the sdk respository to verify any given SDK version throws an unrecoverable error when trying to deserialize a transaction object in which the transaction bodies do not match.

[gregscullard]

There is a security consideration of sending a list of valid transactions to a wallet and an end user to sign only once. In a naive approach, a malicious dApp could send a disguised list of transactions to a wallet that are not the same. For example, a payload could be sent to a wallet with two transactions encapsulated in a Transaction object. The first transaction could be a transfer of 1 HBAR to a valid account, and the second transaction could be assigning a delelgated spend of 1 million HBAR to a malicious account. In a naive implementation, the wallet would prompt the user to sign both transactions as 1 and the malicious dapp would submit the malicious transaction.

For this method, it is therefore a trusted wallet's duty, burden, and responsibility to verify the transaction bodies are identical in intent before allowing a user to sign. This fortunately is handled by the @hashgraph/sdk javascript SDK.

To strengthen security assurances, while providing the desired functionality and user experience proposed by this HIP, I propose we add a test in this repository or the sdk respository to verify any given SDK version throws an unrecoverable error when trying to deserialize a transaction object in which the transaction bodies do not match.

There was an assumption that the SDK does check on the transaction bodies for an exact match (except node account id) which would be necessarily different for each when a serialized Transaction is de-serialized.

This doesn't appear to be the case however, and the SDK team will investigate introducing such a check.

Note that the current SDK doesn't allow manipulation of transaction lists, it would take a rogue SDK to enable this capability.

[gregscullard]

There was an assumption that the SDK does check on the transaction bodies for an exact match (except node account id) which would be necessarily different for each when a serialized Transaction is de-serialized.

This doesn't appear to be the case however, and the SDK team will investigate introducing such a check.

Note that the current SDK doesn't allow manipulation of transaction lists, it would take a rogue SDK to enable this capability.

After checking with the JS SDK team, such a check is in fact in place, see here:
https://github.com/hiero-ledger/hiero-sdk-js/blob/main/src/transaction/Transaction.js#L471-L487
This is executed whenever a transaction is de-serialized with fromBytes

[bugbytesinc]

There was an assumption that the SDK does check on the transaction bodies for an exact match (except node account id) which would be necessarily different for each when a serialized Transaction is de-serialized.
This doesn't appear to be the case however, and the SDK team will investigate introducing such a check.
Note that the current SDK doesn't allow manipulation of transaction lists, it would take a rogue SDK to enable this capability.

After checking with the JS SDK team, such a check is in fact in place, see here: https://github.com/hiero-ledger/hiero-sdk-js/blob/main/src/transaction/Transaction.js#L471-L487 This is executed whenever a transaction is de-serialized with fromBytes

I just need to raise my hand here and point out that this conversation itself should be noted as a red flag on the design. "Assumption" in how the SDK behaves. I've raised my objections to this design artifact of the SDKs from the beginning. A few small accidental changes in SDK internals could result in an exploit that could be overlooked for a number of versions. I am now at peace because I've said my piece (again).

[gregscullard]

I just need to raise my hand here and point out that this conversation itself should be noted as a red flag on the design. "Assumption" in how the SDK behaves. I've raised my objections to this design artifact of the SDKs from the beginning. A few small accidental changes in SDK internals could result in an exploit that could be overlooked for a number of versions. I am now at peace because I've said my piece (again).

@bugbytesinc Thanks and appreciate your concern, note that the assumption has been replaced with a verification that the check is in place in the SDK.

An alternative might be for the SDK to accept a serialized transaction for a singular node (or no node at all) and construct a list of transactions for random nodes, sign and return the signatures.
This would alleviate the need for the check which as you say, could fail or be inadvertently removed in the future.

It may be then, that we can either:

1. Change the hedera_signTransaction method to accept an optional "multinode" parameter, it would default to false to be backwards compatible and if set to true, the wallet would construct an array of transactions, however the response in this event the parameter is set to true would have to be different since it would need to include a SignedTransaction and public key.

2. Convey through this HIP that not having a node_account_id signifies to the wallet that it should create an array of transactions, sign them. This introduces the same need for a different response to an existing method which could be problematic to existing implementation.

Choosing 1 or 2 could remove the need for the new method proposed in this HIP altogether.

Note that hedera_signAndExecuteTransaction which was part of HIP 820 accepts a transactionList which could equally be open to exploit as you describe.

[bugbytesinc]

Note that hedera_signAndExecuteTransaction which was part of HIP 820 accepts a transactionList which could equally be open to exploit as you describe.

Yes indeed. I objected to that back then for the same reasons as well, but "didn't have the votes" as you might say. transactionList is very problematic from the start, it solves a problem at a level in the tech stack that, in my opinion, should be left to the application layer to decide. However, on the other hand, you're basically stuck with it unless you get the SDK teams to re-think their approach, which is why HIP-820 had to bend to the SDK team because the SDK team, at the time, was unwilling to change anything to facilitate HIP-820 aligning more closely to the HAPI wire protocol itself.

[gregscullard]

transactionList is very problematic from the start, it solves a problem at a level in the tech stack that, in my opinion, should be left to the application layer to decide.

The application layer is free to choose not to leverage TransactionList and enforce a single item in the list, the drawbacks are that the application layer needs to handle potentially more frequent node accessibility failures and ask the user to re-sign transactions.
Back then, TransactionList was seen as a convenience feature for both developers and users to abstract Hedera's unique "no mem pool" approach to transaction submission.

Here's a proposal in code which alleviates your concern which we could implement either in this new proposed method, or ( being mindful of backwards compatibility) as an alternative flow for the existing methods

Application

    // creates a transaction
    let tx = new TransferTransaction()
        .addHbarTransfer(senderAccountId, -10)
        .addHbarTransfer(recipientAccountId, 10);
    // serialize and ship to wallet over wallet connect
    let bytes = tx.toBytes();

Wallet

    // sets up client
    let client = Client.forTestnet().setOperator(
        AccountId.fromString(process.env.OPERATOR_ID),
        PrivateKey.fromString(process.env.OPERATOR_KEY)
    );
    //  de-serializes transaction
    let tx2 = Transaction.fromBytes(bytes);
    // assigns 5 nodes randomly to the transaction
    const nodeList = getRandomNodes(client._network._nodes, 5);
    tx2.setNodeAccountIds(nodeList);
    // signs the transaction
    await tx2.freezeWith(client);
    // serializes 
    const txBytes2 = tx2.toBytes()
    client.close();
    // returns signed transaction bytes
    return txBytes2;

getRandomNodes is a function I wrote, we could have that added to the SDK for convenience and make it a method on the transaction (e.g. .setRandomNodes(numberOfNodes))

function getRandomNodes(nodes, count) {
    const scrambled = nodes.sort(() => Math.random() - 0.5);
    const shortList = scrambled.slice(0, count);
    let nodeList = [];
    for (let node in shortList) {
        nodeList.push(shortList[node]._accountId);
    }
    return nodeList;
}

Would this approach satisfy your concerns over TransactionList ?

[Reccetech]

@gregscullard What is the path forward on this HIP? Move forward? Stale? Withdrawn?
Thanks!

[gregscullard]

@Reccetech Move forwards, definitely not stale or withdrawn

[Reccetech]

@gregscullard Great. Let us know when your ready to move it into review for better visibility to the community to gather additional feedback.

[mgarbs]

@bugbytesinc does @gregscullard explanation satisfy the concerns you were having ? I have marked this hip as last call. After this it will go into active state if we do not hear from you.

[bugbytesinc]

@bugbytesinc does @gregscullard explanation satisfy the concerns you were having ? I have marked this hip as last call. After this it will go into active state if we do not hear from you.

Returning a Transaction which also includes inside of it a SignedTransaction is potentially inviting problems and opportunities for mistakes and malicious man-in the middle attacks. Specifically sending the body bytes to the controller and then having the controller send them back can result in lazy implementations that re-hydrate the "sent back" bytes and don't check that they are exactly the same as the ones that were originally sent out. It would be safer to only return a list of signature maps matching the TransactionList and force the consumer to do due dilligance in this regard.

[gregscullard]

@bugbytesinc thanks for your comments, I think this could work.

[mgarbs]

Security Feedback Addressed

Updated the specification to address @bugbytesinc's security concerns regarding transaction body integrity.

Changes Made:

1. Clarified Return Value: Explicitly stated that hedera_signTransactions returns only signature maps, not full signed transactions

2. Added Security Rationale: Added an "Important Security Note" in the Returns section explaining why signature-only return is safer

3. Enhanced Example Code: Updated the example to show the complete flow:

   • Creating the original transaction
   • Sending for signing
   • Receiving only signature maps
   • Adding signatures to the ORIGINAL transaction object
   • This ensures body bytes cannot be tampered with

4. Security Implications Section: Added "Transaction Body Integrity" subsection explaining:

   • How signature-only return prevents man-in-the-middle attacks
   • Why lazy implementations that re-hydrate returned bytes are prevented
   • The security benefits of forcing dApps to maintain their original transaction

This design ensures that dApp implementations must perform proper verification and cannot blindly trust returned bytes, preventing potential exploits where transaction body bytes might be modified in transit.

[mgarbs]

@bugbytesinc please review this commit e9484f9

[AdrianKBL]

LGTM

[gregscullard]

Minor update to the contents which was missing text relating to the return value in the description for signTransactions
9b54031

[Reccetech]

Approved.

[teacoat]

+1 lets goooo