Snowbridge V2 Audit Fixes

[claravanstaden]

This PR contains the following fixes after the Snowbridge V2 audit issue were found.

Failure to advance nonce on error allows repeated execution of failed events without relayer incentives (issue 1)

Severity: Critical

In bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs:216, the submit extrinsic allows submission of inbound events originating from Ethereum. These events are decoded into messages and processed by the process_message function.

However, if the send_xcm operation fails, the transaction reverts without storing the nonce or registering rewards. Consequently, the same inbound message remains unmarked and eligible for re-execution.

This creates multiple risks: the same message can be replayed multiple times, potentially in varying orders and at a specific timing decided by the relayer under diff erent conditions, potentially causing unintended state transitions.

Additionally, since rewards are only registered upon successful execution, relayers are incentivized to reorder messages to maximize successful transactions, potentially at the expense of the user and system fairness.

Recommendation
We recommend storing the nonce and reward relayers also in case the send_xcm operation fails.

Snowbridge notes:
We dispute this finding, because the inbound-queue can never replay messages because it checks the nonce. Also, by allowing send_xcm to fail without reverting the whole transaction, we are essentially killing the message and preventing it from being retried at a later time. This will lead to a loss of user funds. Message sends can fail for a number of reasons, like if the HRMP queues between parachains are full, and that's something we don't have control over. So we need to allow processing a message again if the send XCM fails. We did however move setting the nonce to before the send XCM method, even though it does not make a difference functionally, it is better semantically.

Reward misallocation due to ignored reward_address field (issue 5)

Severity: Major

In bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs:374, the process_delivery_receipt function processes a DeliveryReceipt parameter.

However, while the function correctly validates the gateway and nonce fields, it disregards the reward_address field contained within the DeliveryReceipt structure.

As a result, delivery rewards are always assigned to the transaction sender rather than to the beneficiary specified by the reward_address.

Recommendation
We recommend validating and utilizing all fields of the DeliveryReceipt structure to correctly allocate rewards.

Non-sequential call indexes in systemv2 pallet (issue 15)

Severity: Informational

In bridges/snowbridge/pallets/system-v2/src/lib.rs, the call indexes for extrinsics are inconsistently assigned.

Specifically, while indexes 0, 3, and 4 are implemented, indexes 1 and 2 remain undefined.

Although this does not currently pose a functional or security risk, the non-sequential indexing undermines the clarity and consistency of the codebase. In the context of a newly developed pallet, maintaining orderly call indexes aids in readability, developer experience, and future maintainability.

Recommendation:
We recommend assigning sequential call indexes to all implemented extrinsics within the systemv2 pallet.

---

Incorrect NatSpec in register_token function (issue 17)

Severity: Informational

In bridges/snowbridge/pallets/system-v2/src/lib.rs:182, the register_token function is documented to include a fee parameter, but no such parameter is actually used in the function.

Recommendation:
We recommend updating the NatSpec documentation to accurately reflect the function parameters and behavior, removing any reference to a non-existent fee.

---

Potentially confusing message hashing implementation (issue 21)

Severity: Informational

In bridges/snowbridge/pallets/system-v2/src/lib.rs:230-235, the Message struct includes an id field, which is intended to store the blake2_256 hash of the message itself.

However, during the hash computation, the id field is first initialized with its default value. The hash is then calculated over the struct containing this default id, and only afterward is the computed hash assigned back to the id field.

This approach, while functional, introduces complexity for external systems or tools attempting to verify the message. Verifiers must replicate this specific sequence: extract the id value from the message, reset the id field in the struct to its default value, compute the hash, and then compare it against the originally extracted id.

Recommendation:
We recommend implementing a wrapper data structure over the Message that would contain its hash as a separate member, not part of the Message itself.

Snowbridge notes:
We used unique_id to generate an ID instead.

---

Optimize SparseBitmap (issue 22)

Severity: Informational

In bridges/snowbridge/primitives/core/src/sparse_bitmap.rs:9, the mapping from buckets to masks is declared, used to define the SparseBitmap structure. The type of keys in this mapping is u128.

Then, within lines 22-27, the function compute_bucket_and_mask is defined, which accepts a parameter index of type u128. This function is called both from get and set functions, which are called only with nonces of type u64. The type of the parameter index can be replaced by u64.

The function compute_bucket_and_mask computes the index of the corresponding bucket by dividing the index by 128, so the value range of the output is less than the value range of the input. Hence, the return type of the function, as well as the type of keys in the mapping declared on line 9, can be declared as u64.

Currently, the Rust implementation of SparseBitmap utilizes 128 times more space than it could.

Recommendation:
We recommend optimizing the storage consumption by adopting u64 as the types of keys in the internal mappings of both Rust and Solidity implementations of SparseBitmap.

---

Misleading implementation details (issue 25)

Severity: Informational

In bridges/snowbridge/primitives/inbound-queue/src/lib.rs:23, the commentary states that the structure EthereumLocationsConverterFor is deprecated. However, it is still used in the V2 source file: bridges/snowbridge/primitives/inbound-queue/src/v2/converter.rs.

Recommendation:
We recommend resolving the aforementioned misleading implementation details.

---

[yrong]

Deduplicate some tests in claravanstaden#25

[paritytech-release-backport-bot]

Created backport PR for stable2503:

• [stable2503] Backport #8240 #8432 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-8240-to-stable2503
git worktree add --checkout .worktree/backport-8240-to-stable2503 backport-8240-to-stable2503
cd .worktree/backport-8240-to-stable2503
git reset --hard HEAD^
git cherry-pick -x 632eed96b4560a45ce41c6f6bedec9a4c745a604
git push --force-with-lease