Check that client state is zeroed out for IBC client upgrade proposals

[colin-axner]

Summary

proposal.ValidateBasic() should ensure the client submitted is zeroed out. Not a security concern but prevents the proposal from containing information governance is not actually voting on

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[alizahidraja]

Hey!
I wanted to ask, in what cases is the client state not needed? And also want to confirm that we’re talking about the client state associated with UpgradeProposal, right?

[crodriguezvega]

• Add utility function hasZeroedCustomFields to exported.ClientState.
• For 07-tendermint implement hasZeroedCustomFields in client_state.go and check that TrustLevel, TrustingPeriod, MaxClockDrift, FrozenHeight, AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour have all default values.
• For 06-solomachine implement hasZeroedCustomFields in client_state.go and check that FrozenSequence and AllowUpdateAfterProposal have both default values.
• Use hasZeroedCustomFields in UpgradeProposal.ValidateBasic and return error if custom fields are not zeroed out.

[colin-axner]

Hey! I wanted to ask, in what cases is the client state not needed? And also want to confirm that we’re talking about the client state associated with UpgradeProposal, right?

Yes we are talking about the client state in the upgrade proposal. The client is always needed. "Zeroing" out the client state just means removing all relayer set fields from the client state (setting them to nil/zero)

[colin-axner]

• Add utility function hasZeroedCustomFields to exported.ClientState.

  • For 07-tendermint implement hasZeroedCustomFields in client_state.go and check that TrustLevel, TrustingPeriod, MaxClockDrift, FrozenHeight, AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour have all default values.

  • For 06-solomachine implement hasZeroedCustomFields in client_state.go and check that FrozenSequence and AllowUpdateAfterProposal have both default values.

  • Use hasZeroedCustomFields in UpgradeProposal.ValidateBasic and return error if custom fields are not zeroed out.

No, we don't want to add another function to the exported.ClientState. We can simply check that reflect.DeepEqual(up.UpgradedClientState, up.UpgradedClientState.ZeroCustomFields()) ie that the custom fields have already been zeroed out on the provided upgraded client state

[crodriguezvega]

No, we don't want to add another function to the exported.ClientState. We can simply check that reflect.DeepEqual(up.UpgradedClientState, up.UpgradedClientState.ZeroCustomFields()) ie that the custom fields have already been zeroed out on the provided upgraded client state

I think we need to first unpack, right? So then we just need to do something like this?

clientState, err := UnpackClientState(up.UpgradedClientState)
if err != nil {
        return sdkerrors.Wrap(err, "failed to unpack upgraded client state")
}

if !reflect.DeepEqual(clientState, clientState.ZeroCustomFields()) {
	return sdkerrors.Wrap(err, "upgraded client state is not zeroed out")
}

[colin-axner]

I think we need to first unpack, right?

Yup, but the unpacking is already being performed in proposal.ValidateBasic()

[colin-axner]

closed by #1676