processPacket and recoverAck can fail due to Wormhole guardian change

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x390d31b6ea8525b68816e59f75b266f94439726e7849a8cc911a673e2bd75345
Severity: medium

Description:
Description
Wormhole governance can change signing guardian sets. As the application sets the incentive for the relayer to deliver the message, and gas prices can spike and remain high for a longer period of time, the relayer could be witholding the delivery of message.
If during this time the Wormhole guardian set changes the message cannot be delivered.

Attack Scenario
During the delivery of a message _verifyPacket gets called.
One of the checks is to verify the guardian set: https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/apps/wormhole/external/callworm/WormholeVerifier.sol#L56.
But Wormhole can change the guardian set at any moment: https://github.com/wormhole-foundation/wormhole/blob/main/ethereum/contracts/Governance.sol#L76-L112. After the change the existing signatures can only be used for 1 day: https://github.com/wormhole-foundation/wormhole/blob/main/ethereum/contracts/Setters.sol#L13, as expiration time is set.

If message is not delivered during this period the verification will fail, and it can no longer be delivered.

This is a problem with the whole incentive mechanism as the sending application sets the gas price but gas prices can spike and remain high for extended periods of time. During which time the relayer is not going to deliver the message.

Potential fix
The IncentivizedWormholeEscrow contract needs to have a way of reverting the state on the sending chain if the acknowledgment cannot be delivered.
Also, there should be maximum delivery time specified.

[reednaa]

I don't personally believe there is any way to timeout packages such that this can be avoided.

How would you know the package hasn't been executed on the destination chain?
Say the package has been executed on the destination chain but nobody has delivered the ack back. How would we (on the source chain) figure out the difference between an ack and a timeout?

[windhustler]

I believe this is an underlying problem of the whole incentive mechanism. It's impossible to set the incentives right.

If you have an application that favors liveness and wants to deliver the message ASAP, no matter how high it sets the gas price, it can still spike 5x within minutes.

Someone has to take this risk. With LayerZero for example the relayer is the one that takes it: https://layerzero.gitbook.io/docs/ecosystem/relayer/overview#market-risk.

Also, with most of these messaging protocols, you are going to have some expiry time for signatures. They need to have these measures in place if they get compromised.

[reednaa]

As with any generalisation fine details are lost. Some of these details include support for synchronous message, high liveness, etc.

We will be discussing timeouts internally, however, as currently presented I lean missing documentation. I have asked Wormhole what happens to old VAAs and it will form the basis of how the issue is graded.

[windhustler]

To add to the impact here:

If the _handleAck fails back on the sending chain, the user is supposed to call the recoverAck function.
As you know it is not always possible to inform all users in a timely manner on manual intervention on their side.

Depending on the application logic, this means a loss of funds for the user.

[reednaa]

We have asked Wormhole if this is a concern for the Wormhole implementation, and it is not.

Wormhole Guardians will resign old messages if asked to.

We do acknowledge that this is a manual process compared to the normal flow. We will figure out severity internally.

[windhustler]

Well, of course the Wormhole team will respond as such. The "Guardian change" governance functionality is also there to replace compromised signers.

Your design needs to handle two cases:

1. If there is a regular Guardian change there are no lost messages.
2. If there is an emergency Guardian change due to compromised signers you should be able to react as quickly as possible to minimize the damage.

This being said if there are too many trust assumptions you are not comfortable with while integrating Wormhole, I would consider other options.

[reednaa]

I don't know how to answer you here.

1. That is Wormhole's problem not ours. They explicitly tell us there is support for it. And even if there weren't, it is quite clear that support could easily be added since they are dedicated entities.
2. No...? This is an incentive integration of Wormhole, not a security overhaul of Wormhole.

[windhustler]

I understand it's hard to work around all the edge cases around external integrations that aren't immutable which is the case with Wormhole.

For 2. something as simple as pausing functionality could be added so no new messages are submitted.

I'm just trying to highlight here what are the security assumptions some other protocol needs to take into consideration if they want to use your GeneralisedIncentives contracts.

[reednaa]

2. Who would be the owner of the escrow?
   The implementation is intended to also be used by other applications than Catalyst.

How would the security assumptions be different here compared to a direct integration with Wormhole?

[windhustler]

The pausing functionality can be implemented for the whole escrow in which case you would probably need to manage it. Another option is each implementation having the possibility of pausing submitMessage only for their implementation.

In almost all cross-chain systems liveness and fast delivery of messages are prioritized. Your incentive mechanism kind of turns this upside down.

As there are a few other issues the exact fix will depend on how those are handled.

[fonstack]

Project decided to pay 100$ as a 'gift' for the effort even though is considered invalid