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

fix: check that MsgChannelUpgradeInit is signed by authority #4773

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/golangci-feature.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ jobs:
steps:
- uses: actions/setup-go@v4
with:
go-version: '1.20'
- uses: actions/checkout@v3
go-version: '1.21'
- uses: actions/checkout@v4
- name: golangci-lint
uses: golangci/golangci-lint-action@v3.6.0
uses: golangci/golangci-lint-action@v3.7.0
with:
version: v1.53.1
args: --timeout 5m --exclude unused
version: v1.54.2
args: --timeout 10m
10 changes: 5 additions & 5 deletions modules/apps/callbacks/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,27 +364,27 @@ func (im IBCMiddleware) OnChanCloseConfirm(ctx sdk.Context, portID, channelID st
}

// OnChanUpgradeInit implements the IBCModule interface
func (im IBCMiddleware) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) {
func (IBCMiddleware) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) {
panic("implement me")
}

// OnChanUpgradeTry implements the IBCModule interface
func (im IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string) (string, error) {
func (IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string) (string, error) {
panic("implement me")
}

// OnChanUpgradeAck implements the IBCModule interface
func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
func (IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
panic("implement me")
}

// OnChanUpgradeOpen implements the IBCModule interface
func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) {
func (IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) {
panic("implement me")
}

// OnChanUpgradeRestore implements the IBCModule interface
func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {
func (IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {
panic("implement me")
}

Expand Down
1 change: 0 additions & 1 deletion modules/apps/transfer/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,6 @@ func (suite *TransferTestSuite) TestOnChanUpgradeInit() {
expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)

upgrade := path.EndpointA.GetChannelUpgrade()
suite.Require().Equal(upgradePath.EndpointA.ConnectionID, upgrade.Fields.ConnectionHops[0])
} else {
Expand Down
4 changes: 4 additions & 0 deletions modules/core/04-channel/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ var (
_ sdk.HasValidateBasic = (*MsgAcknowledgement)(nil)
_ sdk.HasValidateBasic = (*MsgTimeout)(nil)
_ sdk.HasValidateBasic = (*MsgTimeoutOnClose)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeInit)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeTry)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeAck)(nil)
_ sdk.HasValidateBasic = (*MsgChannelUpgradeConfirm)(nil)
)

// NewMsgChannelOpenInit creates a new MsgChannelOpenInit. It sets the counterparty channel
Expand Down
4 changes: 4 additions & 0 deletions modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,10 @@ func (k Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAckn
func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeInit) (*channeltypes.MsgChannelUpgradeInitResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

if k.GetAuthority() != msg.Signer {
return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "expected signer address to be the same as the authority address %s, got %s", k.GetAuthority(), msg.Signer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should have a consistent error message for this check?

return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "expected %s, got %s", k.GetAuthority(), msg.Signer)

Copy link
Contributor Author

@charleenfei charleenfei Oct 4, 2023

Choose a reason for hiding this comment

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

actually, it looks like theres an invalid signer error in govtypes. maybe we can update all of these to use that error return, looks like that's what the 08-wasm client returns in StoreCode. i can open a code hygiene issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, maybe we open an issue and address once all the feature branches are merged to main?

}

module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId)
if err != nil {
ctx.Logger().Error("channel upgrade init failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id"))
Expand Down
69 changes: 68 additions & 1 deletion modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,72 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
}
}

func (suite *KeeperTestSuite) TestChannelUpgradeInit() {
var (
path *ibctesting.Path
msg *channeltypes.MsgChannelUpgradeInit
)

cases := []struct {
name string
malleate func()
expResult func(res *channeltypes.MsgChannelUpgradeInitResponse, err error)
}{
{
"success",
func() {
msg = channeltypes.NewMsgChannelUpgradeInit(
path.EndpointA.ChannelConfig.PortID,
path.EndpointA.ChannelID,
path.EndpointA.GetProposedUpgrade().Fields,
path.EndpointA.Chain.GetSimApp().IBCKeeper.GetAuthority(),
)
},
func(res *channeltypes.MsgChannelUpgradeInitResponse, err error) {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(uint64(1), res.UpgradeSequence)
},
},
{
"authority is not signer of the upgrade init msg",
func() {
msg = channeltypes.NewMsgChannelUpgradeInit(
path.EndpointA.ChannelConfig.PortID,
path.EndpointA.ChannelID,
path.EndpointA.GetProposedUpgrade().Fields,
path.EndpointA.Chain.SenderAccount.String(),
)
},
func(res *channeltypes.MsgChannelUpgradeInitResponse, err error) {
suite.Require().Error(err)
suite.Require().ErrorContains(err, ibcerrors.ErrUnauthorized.Error())
suite.Require().Nil(res)
},
},
}

for _, tc := range cases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()

path = ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)

// configure the channel upgrade version on testing endpoints
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion

tc.malleate()

res, err := keeper.Keeper.ChannelUpgradeInit(*suite.chainA.App.GetIBCKeeper(), suite.chainA.GetContext(), msg)

tc.expResult(res, err)
})
}
}

func (suite *KeeperTestSuite) TestChannelUpgradeTry() {
var (
path *ibctesting.Path
Expand Down Expand Up @@ -1511,7 +1577,8 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() {
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion

suite.Require().NoError(path.EndpointA.ChanUpgradeInit())
err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

// cause the upgrade to fail on chain b so an error receipt is written.
// if the counterparty (chain A) upgrade sequence is less than the current sequence, (chain B)
Expand Down
58 changes: 56 additions & 2 deletions testing/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@ package ibctesting

import (
"fmt"
"strconv"
"strings"

"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/baseapp"
sdk "github.com/cosmos/cosmos-sdk/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov"
govtypesv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"

abci "github.com/cometbft/cometbft/abci/types"

Expand Down Expand Up @@ -587,18 +591,68 @@ func (endpoint *Endpoint) QueryChannelUpgradeProof() ([]byte, []byte, clienttype

// ChanUpgradeInit sends a MsgChannelUpgradeInit on the associated endpoint.
// A default upgrade proposal is used with overrides from the ProposedUpgrade
// in the channel config.
// in the channel config, and submitted via governance proposal
func (endpoint *Endpoint) ChanUpgradeInit() error {
upgrade := endpoint.GetProposedUpgrade()

// create upgrade init message via gov proposal and submit the proposal
msg := channeltypes.NewMsgChannelUpgradeInit(
endpoint.ChannelConfig.PortID,
endpoint.ChannelID,
upgrade.Fields,
endpoint.Chain.GetSimApp().IBCKeeper.GetAuthority(),
)

proposal, err := govtypesv1.NewMsgSubmitProposal(
[]sdk.Msg{msg},
sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, govtypesv1.DefaultMinDepositTokens)),
endpoint.Chain.SenderAccount.GetAddress().String(),
endpoint.ChannelID,
"upgrade-init",
fmt.Sprintf("gov proposal for initialising channel upgrade: %s", endpoint.ChannelID),
false,
)
require.NoError(endpoint.Chain.TB, err)

return endpoint.Chain.sendMsgs(msg)
var proposalID int
res, err := endpoint.Chain.SendMsgs(proposal)
if err != nil {
return err
}

events := res.Events
for _, event := range events {
for _, attribute := range event.Attributes {
if attribute.Key == "proposal_id" {
proposalID, err = strconv.Atoi(attribute.Value)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(endpoint.Chain.TB, err)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

With this approach we would need to convert the any into a MsgSubmitProposalResponse there is some code in the e2e tests that does this already and might be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah that's annoying. Opened #4803, happy to fix later

charleenfei marked this conversation as resolved.
Show resolved Hide resolved

// vote on proposal
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
ctx := endpoint.Chain.GetContext()
require.NoError(endpoint.Chain.TB, endpoint.Chain.GetSimApp().GovKeeper.AddVote(ctx, uint64(proposalID), endpoint.Chain.SenderAccount.GetAddress(), govtypesv1.NewNonSplitVoteOption(govtypesv1.OptionYes), ""))
require.NoError(endpoint.Chain.TB, endpoint.Chain.GetSimApp().GovKeeper.AddVote(ctx, uint64(proposalID), endpoint.Chain.SenderAccount.GetAddress(), govtypesv1.NewNonSplitVoteOption(govtypesv1.OptionYes), ""))
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

// fast forward the chain context to end the voting period
params, err := endpoint.Chain.GetSimApp().GovKeeper.Params.Get(ctx)
require.NoError(endpoint.Chain.TB, err)

newHeader := endpoint.Chain.GetContext().BlockHeader()
newHeader.Time = endpoint.Chain.GetContext().BlockHeader().Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod)
ctx = ctx.WithBlockHeader(newHeader)
require.NoError(endpoint.Chain.TB, govtypes.EndBlocker(ctx, &endpoint.Chain.GetSimApp().GovKeeper))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// check if proposal passed or failed on msg execution
p, err := endpoint.Chain.GetSimApp().GovKeeper.Proposals.Get(ctx, 1)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(endpoint.Chain.TB, err)
if p.Status != govtypesv1.StatusPassed {
return fmt.Errorf("proposal failed: %s", p.FailedReason)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to return the error? Maybe we can just do an assertion here?

require.Equalf(endpoint.Chain.TB, govtypesv1.StatusPassed, p.Status, "proposal failed: %s", p.FailedReason)

Copy link
Contributor Author

@charleenfei charleenfei Oct 4, 2023

Choose a reason for hiding this comment

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

I wanted to return the error here so that I could do an assertion in the test itself, since this is a util function.


endpoint.Chain.Coordinator.CommitBlock(endpoint.Chain)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

// ChanUpgradeTry sends a MsgChannelUpgradeTry on the associated endpoint.
Expand Down