feat: authenticate counterparty clients by content and pin them after adoption

[giunatale]

Replaces chain-id-string trust with content verification on both sides of the
provider/consumer relationship, and pins the result.

Why

Anyone can permissionlessly create an IBC v2 client and get a relayer to route
packets through it. Previously both sides trusted the chain-id string alone:

• The provider adopted any active same-chain-id client with a counterparty
  whenever its current client was expired, frozen, or counterparty-less — so
  client expiry reopened adoption forever, to any look-alike chain.
• The consumer re-pointed its provider client to whatever same-chain-id client
  the latest validator-set update arrived over — an automatic client switch
  driven by inbound traffic.

What

Provider — content-bound adoption, then a permanent latch.
discoverActiveConsumerClient adopts a candidate only if it is an active
tendermint client of the consumer's chain id, counterparty-linked, and its
latest consensus state's NextValidatorsHash equals the CometBFT hash of the
validator set the provider itself last sent to that consumer — or of the
previous sent set, tolerating a set change whose packet is still in flight. A
chain that copies the chain-id string cannot make the provider's own validators
sign its blocks, so its consensus states cannot carry the right hash and keep
advancing. A chain-id match that fails the content check logs a warning (that
is a look-alike chain). If no candidate verifies, nothing is adopted and
discovery retries next epoch — fail closed; the liveness sweep owns a consumer
that never gets served. Once adopted, the client is latched permanently:
expiry, freezing, or counterparty loss halt traffic (packets stay queued)
rather than reopening adoption.

New state: the previous sent set's hash is retained per consumer (rotated when
the stored set is replaced) and exported/restored through a new provider
genesis field.

Consumer — the pin moves at most once, then never. The automatic re-point
is deleted. The client created from provider-produced state at genesis cannot
itself receive packets: IBC v2 routes packets only to clients whose
counterparty was registered by their creator, and a keeper-created genesis
client has no creator (verified against ibc-go v10.2.0 — RegisterCounterparty
requires the signer to equal the recorded creator, and counterparties are
write-once). So the pin moves exactly once, from that provably-unroutable
genesis client to the first client that actually delivers a validator-set
update — already light-client-proof-verified and counterparty-linked by IBC,
chain-id-gated by VAAS — and every later packet must arrive over the pinned
client or it is rejected before any state changes. The residual
trust-on-first-use window is the interval between consumer start and its first
delivered update; previously the pin was movable forever.

Recovery. The only re-key path on either side is IBC's governance client
recovery (MsgRecoverClient), which substitutes client state under the same
client id — the pin and the latch survive it. No new VAAS messages.

Relayer ergonomics. Acknowledgements and timeouts arriving for an unknown
client are now log-only instead of failing the relayer's transaction (they can
only correspond to packets sent before a consumer was removed).

The adopted client must be able to support a challenge. Downtime slashing is
falsifiable only if an accused validator can get a challenge verified, and that
verification runs against the provider's light client for the consumer — so it
must land inside the client's trusting period. Nothing enforced that: a
consumer's unbonding_period had only an upper bound, the consumer advertises
that value as its staking unbonding time precisely so relayers derive the
trusting period from it, and adoption never read the trusting period before
pinning the client permanently. A consumer could therefore hand its validators a
client whose trusting period is shorter than the window in which they would need
to defend themselves, and their slashes would execute undefended.

Adoption now rejects a candidate whose TrustingPeriod does not exceed
DowntimeEvidenceMaxAge + DowntimeChallengeWindow (logged, and fail-closed like
the content check), and consumer creation and update reject an
unbonding_period that cannot produce such a client, naming the minimum. The
consumer-chain query additionally reports the adopted client's trusting period
alongside the challengeable interval, so an operator can answer "are challenges
possible for this consumer?" in one call.

Testing

• Unit: forged client (same chain id, wrong valset hash) rejected and logged;
  correct client adopted; previous-set tolerance; latch holds across
  expiry/frozen/counterparty loss; fail-closed retry then adopt;
  highest-verified-height tie-break; prev-hash rotation; hash equivalence
  checked against an independently computed CometBFT hash; consumer bootstrap
  adoption then latch; rejection over a non-pinned client leaves valset, pin,
  staleness clock, debt, staged params, and the dedup watermark untouched;
  missing pin rejects; genesis round-trip of the new field.
• Both Docker e2e suites pass end to end (19 subtests): the single real relayer
  client content-verifies and is adopted, the consumer bootstrap-pins it, and
  every downstream scenario (valset sync, debt, downtime slash, fee pool,
  removal, provider genesis round-trip including the new field) is unaffected.

[giunatale]

Branched from giunatale/feat/consensus-key-rotation (#64) (hard dependency: needs atomone-sdk v0.500.2 and the rotation hooks). Opened against that branch; will retarget to main once it lands.

[julienrbrt]

im not super sure about this tbf, @tbruyelle input would be useful

[tbruyelle]

producing a header carrying them requires the very validators the provider put in charge of the consumer to have signed it

This is not true for a consensus state written by MsgCreateClient, because unlike the consensus states in MsgUpdateClient, this one is not verified. So an attacker just needs to send the message below to steal the seat of an honest unadopted consumer chain:

MsgCreateClient{
	client_state: {
		chain_id: "<consumer chain id>",
		// provide absurd height to outrank any honest candidate 
        // at the `height > bestHeight` check
		latest_height: {<consumer revision id>, 999999999},  
		... 
	},
	consensus_state: {
		next_validators_hash: <hash of the provider's current consumer valset>,
		root:                 <attacker chain's root hash>, 
		timestamp:            now
	},
}

Then the attacker sends the MsgRegisterCounterParty, this is permitted since signer == creator and it's ready to be discovered by this function.

Legit consumer chain can be rejected from discovery

This one concerns the valset hashes check in clientCarriesExpectedValSetHash: it can simply reject an honest consumer from being adopted.

Until a client is adopted, the consumer is stuck on the valset the provider wrote into its genesis, no VSC packet can reach it without a client. Meanwhile the provider recomputes and re-stores the consumer valset every epoch. So any 2 changes in that computed set evict the genesis valset hash from the accepted window. From that point the legitimate client is refused at every epoch with nothing but a warn log, no VSC packet is ever delivered, lastAck never advances and finally SweepUnresponsiveConsumers stops and the deletes the consumer chain. The chain has to be registered from scratch.

Suggested fix

Instead of trying to infer which IBC client belongs to a consumer, let the consumer's owner do it manually, using MsgUpdateConsumer (only the owner can submit this message), with the addition of an optional client_id field.

It adds a manual launch step, but this allows to remove the complex discovery entirely, and so discards the related attack.

With this change a malicious owner can harm the provider valset: he could point the provider at a chain they control and forge downtime evidence against provider validators. So we need to be restrictive with its usage: it can only be used once. MsgUpdateConsumer can only set the client id if it's empty. We still need MsgRecoverClient to replace expired/frozen client.

So there's still a possible attack with this change but it is limited to the consumer chain owner.

MsgRecoverClient is bound

Something that I've discovered during the review is that MsgRecoverClient is bound. If we try to switch an existing client to another without preserving the client id, it destroys downtime history. On consumer side it's not really better, such switch will change the expected denom in the photon fee decorator, making all previously tranfered photons unusable and requiring people to transfer some other photons.

[tbruyelle]

The PR introduces a clear improvement of the client id healing process, but there's still possibility to front-run the client id pin, with the consequences of having to restart the consumer bootstrap.

Added 2 comments, one for consumer and one for provider client id healing, both avocate for an extra manual step instead of the self-healing.

[tbruyelle]

producing a header carrying them requires the very validators the provider put in charge of the consumer to have signed it

This is not true for a consensus state written by MsgCreateClient, because unlike the consensus states in MsgUpdateClient, this one is not verified. So an attacker just needs to send the message below to steal the seat of an honest unadopted consumer chain:

MsgCreateClient{
	client_state: {
		chain_id: "<consumer chain id>",
		// provide absurd height to outrank any honest candidate 
        // at the `height > bestHeight` check
		latest_height: {<consumer revision id>, 999999999},  
		... 
	},
	consensus_state: {
		next_validators_hash: <hash of the provider's current consumer valset>,
		root:                 <attacker chain's root hash>, 
		timestamp:            now
	},
}

Then the attacker sends the MsgRegisterCounterParty, this is permitted since signer == creator and it's ready to be discovered by this function.

Legit consumer chain can be rejected from discovery

This one concerns the valset hashes check in clientCarriesExpectedValSetHash: it can simply reject an honest consumer from being adopted.

Until a client is adopted, the consumer is stuck on the valset the provider wrote into its genesis, no VSC packet can reach it without a client. Meanwhile the provider recomputes and re-stores the consumer valset every epoch. So any 2 changes in that computed set evict the genesis valset hash from the accepted window. From that point the legitimate client is refused at every epoch with nothing but a warn log, no VSC packet is ever delivered, lastAck never advances and finally SweepUnresponsiveConsumers stops and the deletes the consumer chain. The chain has to be registered from scratch.

Suggested fix

Instead of trying to infer which IBC client belongs to a consumer, let the consumer's owner do it manually, using MsgUpdateConsumer (only the owner can submit this message), with the addition of an optional client_id field.

It adds a manual launch step, but this allows to remove the complex discovery entirely, and so discards the related attack.

With this change a malicious owner can harm the provider valset: he could point the provider at a chain they control and forge downtime evidence against provider validators. So we need to be restrictive with its usage: it can only be used once. MsgUpdateConsumer can only set the client id if it's empty. We still need MsgRecoverClient to replace expired/frozen client.

So there's still a possible attack with this change but it is limited to the consumer chain owner.

MsgRecoverClient is bound

Something that I've discovered during the review is that MsgRecoverClient is bound. If we try to switch an existing client to another without preserving the client id, it destroys downtime history. On consumer side it's not really better, such switch will change the expected denom in the photon fee decorator, making all previously tranfered photons unusable and requiring people to transfer some other photons.

[tbruyelle]

Similarly to https://github.com/allinbits/vaas/pull/65/changes#r3770838508 (maybe read first), I'd advocate simplifying the client id healing here.

The vuln

Just like the provider version, this version can be front-run. The first VSC packet received by a consumer chain will pin the clientID, there's no guarantee that this packet comes from the legit provider chain, it can be any chain with the same chain id.

Due to epoch boundaries, a legit VSC packet can take time to arrive on the consumer chain after client creation. This gives even more time for an attacker to front-run. We've seen this kind of attack in the past in Cosmos for the Quicksilver launch for example (although not technically related, this demonstrates that quick and easy malicious behaviour made just to bother has been done in the past).

First, remove client creation from genesis

Client creation in genesis is more a IBC v1 leftover than a real requirement, and creates friction with v2. Because IBC v2 requires the counterparty-registration signer to be the client creator, no counterparty can ever be registered for it, so it is permanently unroutable. Note that genesis could bind a creator by calling clientKeeper.SetClientCreator, but that requires knowing in advance the relayer address and I don't like it.

Client creation in genesis also creates friction in this function because it requires the code to distinguish the unroutable genesis client from others. For instance, the check line 198 tries to identify this unroutable client by checking if it has a counterparty or not, but this can be true for other clients too, so it's kind of weak.

Hence I would remove client creation from genesis.

Then, declare manually the client instead of self-healing

Like I suggested in the provider, let's stop client id self-healing, because it has too much attack-surface. Let's register an address in the consumer genesis params, the owner again, seed by the provider as usual. This address is authored to submit a MsgSetProviderClient (maybe gov module address can too), only if the client is absent (bootstrap case).

For the reason mentioned in the provider section's comment, MsgSetProviderClient can be used only at bootstrap, swaping the client id after bootstrap has bad consequences in the consumer, like changing the fee IBC denom.

Then this function enforcePinnedProviderClient only needs to reject if there's no client id registered, which can be probably inlined in the call location instead of a having a specific function for it.

To be clear, the permanent latch this PR introduces is right and should stay, only the bootstrap adoption needs replacing.