In the event of a hardfork, IncentivizedMockImplementation is susceptible to cross-chain signature replay

[hats-bug-reporter]

Github username: @PlamenTSV
Twitter username: @p_tsanev
Submission hash (on-chain): 0xa099d3d13dfa2619ab793bf91a79614b1f71b35c5be5b169264a17ed98f580bb
Severity: medium

Description:
Description
Both the incentivized mock and wormhole implementation implement methods to send, sign and verify packets of messages. Messages which, upon submission, get signed by the owner/signer of the implementation against their identifiers which include: block number, destination identifier and the message itself, as well as a chainId(on the wormhole) or a SOURCE_IDENTIFIER(on the mock). Due to the SOURCE_IDENTIFIER being hardcoded, with no method to alter it, a hardfork of the chain would leave the contract open for cross-chain replay of old messages.

Attack Scenario
A hardfork/split in the word software and blockchains is described as "...a protocol software upgrade that permanently splits a blockchain network into two separate chains. It occurs when nodes on the newest version of the protocol fail to accept the older version of the blockchain."
It an unusual occurence that leads to the seperation of the chain at a given block, producing 2 different block-chains.
The Wormhole implementation would suffer no change, because it uses chainId() to encode identifiers, which changes with forks. Unlike it however, the SOURCE_IDENTIFIER does not change, so identical messages on the 2 different chains would be signed with the same parameters, leading to the same metadata, that later decoded would approve the signature on both chains, allowing the messages to be replayed cross-chain.
Hardforks are often unpredictable events, and their occurence/likelihood could either low or medium depending on the future EIPs, but the impact is definitely high, thus - MEDIUM

More on previous hardforks, solidifying their unpredictiveness and inevitable occurence with more incoming EIPs: https://coinloan.io/blog/history-of-ethereum-hard-forks/
Historical issues with signatures:
https://solodit.xyz/issues/m-05-replay-attack-in-case-of-hard-fork-code4rena-golom-golom-contest-git
https://solodit.xyz/issues/m-24-oracles-are-vulnerable-to-cross-chain-replay-attacks-sherlock-none-gmx-git

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation: add an admin function for changing the SOURCE_IDENTIFIER in the mock or just use chainId() like on wormhole, as it is foulproof.

[reednaa]

Cross-chain applications aren't durable for hardforks.

It is assumed that only 1 instance of cross-chain application will continue working, whatever instance that the AMB determines to be canonical. When that is done, it is very likely that the non-canonical chain will freeze: Messages won't be submitted, checks against chain.id freezes, and much more.

We could try to build defences into the application but it is much simpler to ignore the new fork. If it becomes relevant, then everything can be redeployed to ensure the deployment isn't accidently broken.

So far, I have yet to see a sustainable hard fork.

Cons associated with the proposed fix: Verifying the message becomes 2000 gas more expensive. As such, hard coding the chainid is (from my perspective) a better fix.

[PlamenTSV]

The new chain instance that continues to work is fine, but chain support after a hardfork does not go away instantly for the old one either (check out ETH vs ETC).
If the canonical chain for the application splits to the new chain, they would share the same SOURCE_IDENTIFIER.
The first half of the fix to introduce an additional function seems fine if you do not want 2000 more gas.
Imo even LOW would be fine for a known behavior were not notified not to look for, that is a real scenario.

[reednaa]

We have decided to classify this issue as won't fix. Our decision is based on the following arguments:

• It is expected that cross-chain applications break on hard forks.
• The proposed fix increases gas costs OR makes the integration between evm and non-evm significantly more complicated.
• Mock is a strictly auth based scheme and the operator would be able to provide appropiate hot fixes.

According to these arguments, the issue is won’t fix.

[PlamenTSV]

I still sit behind my argument in #72 that non-EVM chains do not sufficiently cover the concept of chainId. I asked around to be sure:

2 chains can have the same ID, thus messages signed on one of them is replayable on the other one.
A fix for this issue is instead of using chainId, thus avoiding any overlap between EVM and non-EVM which allows replay, use the wormhole constants from the docs: https://docs.wormhole.com/wormhole/reference/constants
This is for both wormhole and mock.
I believe this is at least low still, due to the plans to go beyond EVM.

[reednaa]

I don't understand this argument. Could you please propose a fix / PoC or similar?

[PlamenTSV]

The fix is straightforward, replace the UNIQUE_SOURCE_IDENTFIER and chainId parameters with the built-in wormhole constants for different EVM and non-EVM chains (since the protocol intends to support both).
The reason for this is that non-EVM chains can interpret the chainId specification differently and overlap with an already existing id of an EVM chain (This also protects against hardforks).
Thus if we try a message from a non-EVM chain X to EVM chain Y it will be fine. But if we try to relay the same message from chain Z to Y, it will pass through when X and Z have the same id.

Normally the difference between 2 identical messages on 2 chains would be the chainId().
Here it is not present. The low likelihood of hardforks and uncertainty in non-EVM chains leads me to LOW.

[reednaa]

We have classified this as won't fix.
The reason being is that the Mock implementation is an auth implementation and generally on forks we expect to redeploy. This is not an issue on normal operation.