From d51c228058cb456b757a9a5ef787a6b798f8b310 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 13 Jun 2022 12:32:52 +0100 Subject: [PATCH 1/5] wip: Added check for zeroed out custom fields in ValidateBasic --- modules/core/02-client/types/proposal.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/modules/core/02-client/types/proposal.go b/modules/core/02-client/types/proposal.go index 75c9778e8c9..e60e1a4fc0a 100644 --- a/modules/core/02-client/types/proposal.go +++ b/modules/core/02-client/types/proposal.go @@ -2,11 +2,11 @@ package types import ( "fmt" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" + "reflect" "github.com/cosmos/ibc-go/v3/modules/core/exported" ) @@ -111,11 +111,15 @@ func (up *UpgradeProposal) ValidateBasic() error { return sdkerrors.Wrap(ErrInvalidUpgradeProposal, "upgraded client state cannot be nil") } - _, err := UnpackClientState(up.UpgradedClientState) + 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") + } + return nil } From 2e614a9846b3f2faad1280b38365c66b5fd21b71 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 13 Jun 2022 13:23:10 +0100 Subject: [PATCH 2/5] test: Added test for validate basic with non-zeroed fields. --- modules/core/02-client/types/proposal.go | 2 +- modules/core/02-client/types/proposal_test.go | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/modules/core/02-client/types/proposal.go b/modules/core/02-client/types/proposal.go index e60e1a4fc0a..5c518df178b 100644 --- a/modules/core/02-client/types/proposal.go +++ b/modules/core/02-client/types/proposal.go @@ -117,7 +117,7 @@ func (up *UpgradeProposal) ValidateBasic() error { } if !reflect.DeepEqual(clientState, clientState.ZeroCustomFields()) { - return sdkerrors.Wrap(err, "upgraded client state is not zeroed out") + return sdkerrors.Wrap(ErrInvalidUpgradeProposal, "upgraded client state is not zeroed out") } return nil diff --git a/modules/core/02-client/types/proposal_test.go b/modules/core/02-client/types/proposal_test.go index d1a6f52cf71..c322dfdfed9 100644 --- a/modules/core/02-client/types/proposal_test.go +++ b/modules/core/02-client/types/proposal_test.go @@ -109,20 +109,31 @@ func (suite *TypesTestSuite) TestUpgradeProposalValidateBasic() { }{ { "success", func() { - proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, plan, cs) + proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, plan, cs.ZeroCustomFields()) suite.Require().NoError(err) }, true, }, { "fails validate abstract - empty title", func() { - proposal, err = types.NewUpgradeProposal("", ibctesting.Description, plan, cs) + proposal, err = types.NewUpgradeProposal("", ibctesting.Description, plan, cs.ZeroCustomFields()) suite.Require().NoError(err) }, false, }, + { + "non zeroed fields", func() { + proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, plan, &ibctmtypes.ClientState{ + FrozenHeight: types.Height{ + RevisionHeight: 10, + }, + }) + suite.Require().NoError(err) + + }, false, + }, { "plan height is zero", func() { invalidPlan := upgradetypes.Plan{Name: "ibc upgrade", Height: 0} - proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, invalidPlan, cs) + proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, invalidPlan, cs.ZeroCustomFields()) suite.Require().NoError(err) }, false, }, From fd0f4574fc4fa269167cec3fc1ee4289473678b3 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 13 Jun 2022 13:27:33 +0100 Subject: [PATCH 3/5] chore: Adding CHANGELOG.md entry. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff0d73923e3..2c9684e9e1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/core/04-channel) [\#1130](https://github.com/cosmos/ibc-go/pull/1130) Call `packet.GetSequence()` rather than passing func in `WriteAcknowledgement` log output * (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries. * (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure. +* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) Client state must be zeroed out for `UpgradeProposals` to pass validation. ## [v3.0.0](https://github.com/cosmos/ibc-go/releases/tag/v3.0.0) - 2022-03-15 From 6d8f3d580e9805497171f52feafed4951b5eea2d Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 20 Jun 2022 09:49:03 +0100 Subject: [PATCH 4/5] chore: addressed PR feedback, updated CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a4a482352a..90a27f8501a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,7 +70,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries. * (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure. -* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) Client state must be zeroed out for `UpgradeProposals` to pass validation. +* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) ClientState must be zeroed out for `UpgradeProposals` to pass validation. ## [v3.1.0](https://github.com/cosmos/ibc-go/releases/tag/v3.1.0) - 2022-04-16 From b1a32031f4596eab96223864ecb92b618d4a6d00 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Thu, 7 Jul 2022 14:39:43 +0100 Subject: [PATCH 5/5] chore: addressing PR feedback --- CHANGELOG.md | 2 +- modules/core/02-client/types/proposal.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66d5a2987a1..5788fc57aa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,7 +77,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries. * (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure. -* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) ClientState must be zeroed out for `UpgradeProposals` to pass validation. +* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) ClientState must be zeroed out for `UpgradeProposals` to pass validation. This prevents the proposal containing information governance is not actually voting on. ## [v3.1.0](https://github.com/cosmos/ibc-go/releases/tag/v3.1.0) - 2022-04-16 diff --git a/modules/core/02-client/types/proposal.go b/modules/core/02-client/types/proposal.go index 88638cb0d25..cac355d8144 100644 --- a/modules/core/02-client/types/proposal.go +++ b/modules/core/02-client/types/proposal.go @@ -2,11 +2,12 @@ package types import ( "fmt" + "reflect" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" - "reflect" "github.com/cosmos/ibc-go/v4/modules/core/exported" )