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).