Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that client state is zeroed out for ibc client upgrade proposals #1529

2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Bug Fixes

* (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. 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

Expand Down
7 changes: 6 additions & 1 deletion modules/core/02-client/types/proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"fmt"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
Expand Down Expand Up @@ -111,11 +112,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(ErrInvalidUpgradeProposal, "upgraded client state is not zeroed out")
}

return nil
}

Expand Down
17 changes: 14 additions & 3 deletions modules/core/02-client/types/proposal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Contributor Author

@chatton chatton Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous tests did not have zeroed out state.

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,
},
Expand Down