Solana: Multiple Emitter Registrations Per Chain Are Allowed

[evan-gray]

Expected Behavior

There should only be one contract per network which is trusted to emit token / nft bridge messages. Only one emitter per chain should be registered.

Current Behavior

The derivation for an endpoint, the account for which existence signifies that the emitter chain and address is registered, includes both emitter chain and emitter address.

wormhole/solana/modules/token_bridge/program/src/api/governance.rs

Line 134 in de02c20

emitter_address: accs.vaa.endpoint_address,

While the same emitter chain and address combination cannot be registered twice, due to the AccountState::Uninitialized requirement, nothing prevents two different addresses on the same chain from being registered. This is not an immediate concern as registration VAAs can only be created manually through governance, but it deviates from how the other implementations work and could lead to poor maintainer assumptions in the future.

Possible Solution

Add an additional account to RegisterChain which is derived only on emitter chain, must be uninitialized, and gets initialized in this call. This should block the same chain from having multiple registrations. The use of endpoint should not be changed nor should this new account be added to other methods, e.g. CompleteTransfer, as this could break existing integrators. A breaking change to RegisterChain should be acceptable given its limited use only by the Guardians / maintainers and isn't expected to be called by any contracts.

Context (Environment)

This was encountered when needing to update a registration in testnet and I found that other contracts required a migration but Solana just accepted a different registration for the same chain without modification 😅

[kcsongor]

The proposed solution does block re-registrations, but does not cover a scenario where the registration needs to be updated for whatever reason. For other chains there is a way to do this via an upgrade governance, so I think we should at least outline a strategy for that here too.

Firstly, the newly proposed account derived from the chain should contain the current emitter's address. Then the registration upgrade strategy could be as follows:

1. add a new endpoint UpgradeRegistration that takes three accounts: the old emitter, the new emitter, and the chain-based emitter PDA
2. ensure that the old emitter account matches the chain-based emitter address
3. deallocate the old emitter account, and initialise the new one, and put the new address into the chain-based PDA

I'm not suggesting we should implement this strategy right now, but we should document it somewhere in case a registration needs to be updated again (like in the context of the current issue)

[claudijd]

My first reaction in reading the initial description was the likely eventual need to upgrade to a new endpoint for whatever reason. I agree with the sentiment that there should only be one active registration per chain and I @kcsongor approach to migrating to new changes when we are ready for it. No objection here, agree with the proposal.

[kcsongor]

We need a playbook for when/if that happens. e.g. when brining up new chains, the old registrations VAAs still need to be replayed (in order) to ensure that they can't be replayed maliciously.

For solana, worm could first check if a registration already exists, and call out to the appropriate endpoint (i.e. RegisterChain or UpgradeRegistration).

[claudijd]

We need a playbook for when/if that happens. e.g. when brining up new chains, the old registrations VAAs still need to be replayed (in order) to ensure that they can't be replayed maliciously.

I think this could be described in WH Notion (or in a markdown doc under a playbooks folder). I tend to like the idea of it living in the repo to start building out playbooks for operational tasks.