Unbonding period on consumers is not enforced

[andrey-kuprianov]

Surfaced from @informalsystems audit on Interchain Security, at commit 463ec20

Problem

While the documentation says that unbonding period on consumer chains should be shorter than that on the provider chain, this is not enforced by the implementation.

Closing criteria

There is an automatic check in place that ensures that every spawned consumer chains has unbonding period smaller than that on the provider chain.

Problem details

In the file interchain-security/docs/params.md it is said that:

The ConsumerUnbondingPeriod is set via the ConsumerAdditionProposal gov proposal to add a new consumer chain.
Unbonding operations (such as undelegations) MUST wait for the unbonding period on every consumer to elapse. Therefore, for an improved user experience, the ConsumerAdditionProposal on every consumer chain SHOULD be smaller than ProviderUnbondingPeriod, i.e.,

ConsumerUnbondingPeriod = ProviderUnbondingPeriod - one day

Also the ICS specification in the file ibc/spec/app/ics-028-cross-chain-validation/methods.md says, that the following postcondition should hold for the function InitGenesis(gs: ConsumerGenesisState):

consumerUnbondingPeriod is set to ComputeConsumerUnbondingPeriod(gs.providerClientState.unbondingTime)

with ComputeConsumerUnbondingPeriod being

function ComputeConsumerUnbondingPeriod(delta: Duration): Duration {
  if delta > 7*24*Hour {
    return delta - 24*Hour // one day less
  }
  else if delta >= 24*Hour {
 		return delta - Hour // one hour less
 	}
  return delta
}

But the actual implementation of the above, done in function CreateConsumerClient(), does nothing more than copying the unbonding period from the consumer chain governance proposal:

func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditionProposal) error {
    ...

	// Consumers always start out with the default unbonding period
	consumerUnbondingPeriod := prop.UnbondingPeriod

	// Create client state by getting template client from parameters and filling in zeroed fields from proposal.
	clientState := k.GetTemplateClient(ctx)
	clientState.ChainId = chainID
	clientState.LatestHeight = prop.InitialHeight

	trustPeriod, err := ccv.CalculateTrustPeriod(consumerUnbondingPeriod, k.GetTrustingPeriodFraction(ctx))
	if err != nil {
		return err
	}
	clientState.TrustingPeriod = trustPeriod
	clientState.UnbondingPeriod = consumerUnbondingPeriod

	consumerGen, validatorSetHash, err := k.MakeConsumerGenesis(ctx, prop)

Thus, nothing in the implementation really enforces the requirements stated in the specification, and the only enforcement mechanism for them is for voters on the governance proposal to check its parameters. While such a misconfiguration is unlikely to avoid detection, as consumer addition proposals will get lots of public attention, an automatic check should still be in place.

Problem Scenarios

A governance proposal could be passed with the unbonding period on the consumer chain larger than that on the provider chain. This will lead to all unbondings on the provider chain to be delayed for that period (e.g. to 5 weeks, instead of the current 3 weeks for Cosmos Hub).

Recommendation

Add enforcements mechanisms to the implementation, such that unbonding period on the consumer chains is smaller that that of the provider chain, as expected according to the specification.

TODOs

• Labels have been added for issue
• Issue has been added to the ICS project

[shaspitz]

This issue is solid, but I'll point out that the seemingly only way to enforce that a consumer chain's unbonding period is less than the provider's is to use interchain queries. We could give the consumer chain a notion of the provider's unbonding period, upon consumer genesis. But the provider's unbonding period is technically mutable, and the value stored on the consumer could become outdated.

Interchain queries would be cool to use, but AFAIK they use their own module and wouldn't necessarily be trivial to add.

Thoughts @mpoke?

[mpoke]

I think we can solve the issue by just add a warning message in case the Unbonding Period in the proposal is larger than the one on the provider. There is no security issue if it is larger (hence the SHOULD instead of MUST). It's just a recommendation for UX. As @smarshall-spitzbart pointed out, the provider cannot enforce the unbonding period on the consumers (without ICQ).

[mpoke]

@jtremback do you think we should reject proposals with the consumer unbonding larger than the provider unbonding at proposal time?

[p-offtermatt]

Would be interested in working on this issue.
As far as I can tell, since this is a SHOULD and addition proposals are screened by the community, together with the fact that the Unbonding period could be changed later by a malicious consumer anyways, a warning message seems good enough, but I would be interested to know if anyone sees it differently.