IBC: wrong use of UnbondingPeriod in Tendermint client

[josef-widder]

Surfaced from Informal Systems IBC Audit of cosmos-sdk hash 6344d62.

In the code we observe the following line:

if currentTimestamp.Sub(consState.Timestamp) >= clientState.UnbondingPeriod {

It should ensure that the consensus state that is used to verify one of the headers that constitute misbehavior is not too old.

According to the Tendermint Security model, validators in NextValidators of a block (header) with time t need to behave correctly until t + TrustingPeriod. After that time, they may behave arbitrarily (given that they do not appear in as NextValidator in a later block.). However here we are checking against the UnbondingPeriod, not the TrustingPeriod, where UnbondingPeriod > TrustingPeriod. As a result, this check allows nodes that are outside the fault assumption to shut down the client.

Remark. The implemented misbehavior treatment in the Tendermint Client is not specified in ICS 07.

Problem Scenarios

If the age of the consState is between TrustingPeriod and UnbondingPeriod the header will be accepted as base to verify one of the conflicting headers that constitutes misbehavior.

During this period, the validators in consState.NextValidators are not considered trustworthy anymore. As we must assume that they behave arbitrary, they can forge the header that is part of misbehavior (there is no incentive not to do that). As a result adversarial former validators may shut down the client without risking anything.

Recommendation

• Document and specify the misbehavior treatment in ICS 07, and make explicit timing assumptions.
• Change the code to

  if currentTimestamp.Sub(consState.Timestamp) >= clientState.TrustingPeriod {

[colin-axner]

cc @cwgoes this needs a spec fix

[cwgoes]

Thanks for the catch. Upstream spec fixed in cosmos/ibc@a8fb92b.