fix: forbid negative values for trusting period, unbonding period and max clock drift

CHANGELOG.md

@@ -69,6 +69,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### State Machine Breaking
 
 * (transfer) [\#2377](https://github.com/cosmos/ibc-go/pull/2377) Adding `sequence` to `MsgTransferResponse`.
+* (light-clients/07-tendermint) [\#2554](https://github.com/cosmos/ibc-go/pull/2554) Forbid negative values for `TrustingPeriod`, `UnbondingPeriod` and `MaxClockDrift` (as specified in ICS-07).
 
 ### Improvements
 

modules/light-clients/07-tendermint/client_state.go

@@ -125,14 +125,14 @@ func (cs ClientState) Validate() error {
 	if err := light.ValidateTrustLevel(cs.TrustLevel.ToTendermint()); err != nil {
 		return err
 	}
-	if cs.TrustingPeriod == 0 {
-		return sdkerrors.Wrap(ErrInvalidTrustingPeriod, "trusting period cannot be zero")
+	if cs.TrustingPeriod <= 0 {
+		return sdkerrors.Wrap(ErrInvalidTrustingPeriod, "trusting period must be greater than zero")
 	}
-	if cs.UnbondingPeriod == 0 {
-		return sdkerrors.Wrap(ErrInvalidUnbondingPeriod, "unbonding period cannot be zero")
+	if cs.UnbondingPeriod <= 0 {
+		return sdkerrors.Wrap(ErrInvalidUnbondingPeriod, "unbonding period must be greater than zero")
 	}
-	if cs.MaxClockDrift == 0 {
-		return sdkerrors.Wrap(ErrInvalidMaxClockDrift, "max clock drift cannot be zero")
+	if cs.MaxClockDrift <= 0 {
+		return sdkerrors.Wrap(ErrInvalidMaxClockDrift, "max clock drift must be greater than zero")
 	}
 
 	// the latest height revision number must match the chain id revision number

modules/light-clients/07-tendermint/client_state_test.go

@@ -108,20 +108,35 @@ func (suite *TendermintTestSuite) TestValidate() {
 			expPass:     false,
 		},
 		{
-			name:        "invalid trusting period",
+			name:        "invalid zero trusting period",
 			clientState: ibctm.NewClientState(chainID, ibctm.DefaultTrustLevel, 0, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath),
 			expPass:     false,
 		},
 		{
-			name:        "invalid unbonding period",
+			name:        "invalid negative trusting period",
+			clientState: ibctm.NewClientState(chainID, ibctm.DefaultTrustLevel, -1, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath),
+			expPass:     false,
+		},
+		{
+			name:        "invalid zero unbonding period",
 			clientState: ibctm.NewClientState(chainID, ibctm.DefaultTrustLevel, trustingPeriod, 0, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath),
 			expPass:     false,
 		},
 		{
-			name:        "invalid max clock drift",
+			name:        "invalid negative unbonding period",
+			clientState: ibctm.NewClientState(chainID, ibctm.DefaultTrustLevel, trustingPeriod, -1, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath),
+			expPass:     false,
+		},
+		{
+			name:        "invalid zero max clock drift",
 			clientState: ibctm.NewClientState(chainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, 0, height, commitmenttypes.GetSDKSpecs(), upgradePath),
 			expPass:     false,
 		},
+		{
+			name:        "invalid negative max clock drift",
+			clientState: ibctm.NewClientState(chainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, -1, height, commitmenttypes.GetSDKSpecs(), upgradePath),
+			expPass:     false,
+		},
 		{
 			name:        "invalid revision number",
 			clientState: ibctm.NewClientState(chainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.NewHeight(1, 1), commitmenttypes.GetSDKSpecs(), upgradePath),