Wormhole upgrade affects messages that can never be delivered

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x61e756f17fc91d930f9df3ec4a25684a5b2a43e82dfd18cb1b236b601afee914
Severity: high

Description:
Description
Describe the context and the effect of the vulnerability.

Description
Wormhole governance can change the chainId: https://github.com/wormhole-foundation/wormhole/blob/a048c9ac83129c25dffa4919d07931208e83d866/ethereum/contracts/Governance.sol#L146.

_verifyPacket reads the vm.emitterChainId,
and passes it to internal functions inside the processPacket.

This is used to fetch the expected sourceImplementationHash from the implementation mapping: https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L285.

If there is no hash found the message is sent back with the NO_AUTHENTICATION flag.

This is the case with _handleMessage flow.

With _handleAck the message couldn't even be delivered: https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L372.

Scenario
If we take the example of CatalystChainInterface and the above scenario has occurred, the ack message would be received back on the sending chain.
As the first byte would be different than 0x00:

Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/evm/src/CatalystChainInterface.sol

Line 463 in 27b4d0a

if (swapStatus != 0x00) {

.
The execution would continue inside the _onPacketFailure function: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystChainInterface.sol#L421C14-L421C30.

It could never be delivered as context is neither CTX0_ASSET_SWAP nor CTX1_LIQUIDITY_SWAP.

This would mean permanent loss of funds for the user.

[reednaa]

This function is only supposed to be called whenever a chain is forked. I don't understand how we would mitigrate against this or really how we are supposed to do anything about this.

I also disagree about the ack, the first byte is not included:

Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/evm/src/CatalystChainInterface.sol

Line 465 in 27b4d0a

return _onPacketFailure(destinationIdentifier, acknowledgement[1:]);

But yes, if the change is made on the sending side the ack wouldn't be deliverable. But this change wouldn't be worse than Wormhole Guardians creating wrong VAAs.

[windhustler]

Thanks for the correction, still the outcome is the same as you said the ack wouldn't be deliverable.

I understand the preconditions for the Wormhole function to be called, but if that occurs your system gets broken.

I need to think about the proposed fix. My biggest concern is how you lock in the configuration inside your contracts while the underlying messaging protocol can change it.

[windhustler]

Proposed fix:

Each time someone sends a packet you query the messageFee(). This is done as the underlying messageFee can be changed.
While deploying IncentivizedWormholeEscrow save the evmChainId and chainId and compare them to the values stored inside the Storage.WormholeState struct, i.e. provider.chainId and evmChainId.
If they don't match revert();.
This will ensure no damage can be done, and a proper fix can be devised.

[reednaa]

In case of a chain fork, it is expected that the minority chain is the one forking. We would recommend users to not have inflight swaps while there is a chance of forks.

The main chain will continue functioning as intended.

Furthermore, the audit competition details exclude administration "exploits".