From c49ff25c2e1db8b266c05f68e2bb773fbe6d7518 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 8 May 2024 14:51:13 +0200 Subject: [PATCH 1/4] fix: noop on UpdateState for invalid misbehaviour --- modules/light-clients/07-tendermint/update.go | 3 ++- modules/light-clients/07-tendermint/update_test.go | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/modules/light-clients/07-tendermint/update.go b/modules/light-clients/07-tendermint/update.go index e01786a79d1..abb11ff31fc 100644 --- a/modules/light-clients/07-tendermint/update.go +++ b/modules/light-clients/07-tendermint/update.go @@ -134,7 +134,8 @@ func (cs *ClientState) verifyHeader( func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore storetypes.KVStore, clientMsg exported.ClientMessage) []exported.Height { header, ok := clientMsg.(*Header) if !ok { - panic(fmt.Errorf("expected type %T, got %T", &Header{}, clientMsg)) + // clientMsg is invalid Misbehaviour and handler should noop + return []exported.Height{} } cs.pruneOldestConsensusState(ctx, cdc, clientStore) diff --git a/modules/light-clients/07-tendermint/update_test.go b/modules/light-clients/07-tendermint/update_test.go index b28b928babb..93ef5638e7e 100644 --- a/modules/light-clients/07-tendermint/update_test.go +++ b/modules/light-clients/07-tendermint/update_test.go @@ -546,9 +546,12 @@ func (suite *TendermintTestSuite) TestUpdateState() { suite.Require().Equal(expConsensusState, updatedConsensusState) } else { - suite.Require().Panics(func() { - clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, clientMessage) - }) + consensusHeights = clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, clientMessage) + suite.Require().Empty(consensusHeights) + + consensusState, found := suite.chainA.GetSimApp().GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()))) + suite.Require().False(found) + suite.Require().Nil(consensusState) } // perform custom checks From b777fee81a5b47a2f8877f35af3805e5dfd689d0 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 8 May 2024 15:06:13 +0200 Subject: [PATCH 2/4] godoc: update godoc for UpdateState --- modules/light-clients/07-tendermint/update.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/light-clients/07-tendermint/update.go b/modules/light-clients/07-tendermint/update.go index abb11ff31fc..5b9d1ebe5bc 100644 --- a/modules/light-clients/07-tendermint/update.go +++ b/modules/light-clients/07-tendermint/update.go @@ -131,6 +131,7 @@ func (cs *ClientState) verifyHeader( // UpdateState must only be used to update within a single revision, thus header revision number and trusted height's revision // number must be the same. To update to a new revision, use a separate upgrade path // UpdateState will prune the oldest consensus state if it is expired. +// If the provided clientMsg is not of type of Header then the handler will noop and empty slice is returned. func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore storetypes.KVStore, clientMsg exported.ClientMessage) []exported.Height { header, ok := clientMsg.(*Header) if !ok { From e57504eee3cc96fdaae2e86ef68a031bcd0aca8d Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 8 May 2024 15:39:05 +0200 Subject: [PATCH 3/4] Update modules/light-clients/07-tendermint/update.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- modules/light-clients/07-tendermint/update.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/light-clients/07-tendermint/update.go b/modules/light-clients/07-tendermint/update.go index 5b9d1ebe5bc..1a88eee5859 100644 --- a/modules/light-clients/07-tendermint/update.go +++ b/modules/light-clients/07-tendermint/update.go @@ -135,7 +135,7 @@ func (cs *ClientState) verifyHeader( func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore storetypes.KVStore, clientMsg exported.ClientMessage) []exported.Height { header, ok := clientMsg.(*Header) if !ok { - // clientMsg is invalid Misbehaviour and handler should noop + // clientMsg is invalid Misbehaviour, no update necessary return []exported.Height{} } From 9c410533342db7f3694c8d85d1b23fb0ac22807b Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 8 May 2024 16:11:02 +0200 Subject: [PATCH 4/4] chore: add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddd5a2a6210..a27ef681518 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking +* (light-clients/07-tendermint) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions. + ### Improvements * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able.