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.

Sign up for GitHub

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: allow error receipt to be nil in case of the msg sender is the authority #5262

Merged
merged 3 commits into from
Dec 4, 2023
Merged

fix: allow error receipt to be nil in case of the msg sender is the authority #5262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90f2755
fix + tests
charleenfei Nov 30, 2023
a09c160
shuffle about logic ordering
charleenfei Nov 30, 2023
fafb42d
Merge branch '04-channel-upgrades' into charly/issue#5260-allow-error…
charleenfei Dec 4, 2023
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
21 changes: 12 additions & 9 deletions modules/core/04-channel/keeper/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,22 +569,25 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err
return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

if errorReceipt.Sequence < channel.UpgradeSequence {
return errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "error receipt sequence (%d) must be greater than or equal to current upgrade sequence (%d)", errorReceipt.Sequence, channel.UpgradeSequence)
}

// an error receipt proof must be provided if the sender is not authorized to cancel upgrades.
if !isAuthority && len(errorReceiptProof) == 0 {
return errorsmod.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty error receipt proof unless the sender is authorized to cancel upgrades")
}

// if the msgSender is authorized to make and cancel upgrades AND the current channel has not already reached FLUSHCOMPLETE
// then we can restore immediately without any additional checks
// otherwise, we can only cancel if the counterparty wrote an error receipt during the upgrade handshake
if isAuthority && channel.State != types.FLUSHCOMPLETE {
return nil
}

// an error receipt proof must be provided if the sender is not authorized to cancel upgrades.
// the error receipt should also have a sequence greater than or equal to the current upgrade sequence.
if !isAuthority {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bug? As per comment above:

// if the msgSender is authorized to make and cancel upgrades AND the current channel has not already reached FLUSHCOMPLETE
// then we can restore immediately without any additional checks
// otherwise, we can only cancel if the counterparty wrote an error receipt during the upgrade handshake

If the channel state is in FLUSHCOMPLETE, regardless of whether the authority called this function, we must prove that the counterparty wrote an error receipt for a valid upgrade sequence. Otherwise the authority could cancel an upgrade while the counterparty simultaneously is provided historical proof to upgrade to open. In this case, the authority would be harming itself, but no harm in ensuring this isn't possible

if len(errorReceiptProof) == 0 {
return errorsmod.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty error receipt proof unless the sender is authorized to cancel upgrades")
}

if errorReceipt.Sequence < channel.UpgradeSequence {
return errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "error receipt sequence (%d) must be greater than or equal to current upgrade sequence (%d)", errorReceipt.Sequence, channel.UpgradeSequence)
}
}

// get underlying connection for proof verification
connection, found := k.connectionKeeper.GetConnection(ctx, channel.ConnectionHops[0])
if !found {
Expand Down
24 changes: 23 additions & 1 deletion modules/core/04-channel/keeper/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,28 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
},
expError: nil,
},
{
name: "sender is authority, upgrade can be cancelled even with invalid error receipt upgrade sequence",
malleate: func() {
var ok bool
errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

errorReceipt.Sequence = path.EndpointA.GetChannel().UpgradeSequence - 1

suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)

suite.coordinator.CommitBlock(suite.chainB)

suite.Require().NoError(path.EndpointA.UpdateClient())

upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
errorReceiptProof, proofHeight = suite.chainB.QueryProof(upgradeErrorReceiptKey)

isAuthority = true
},
expError: nil,
},
{
name: "sender is authority, channel in flushing, upgrade can be cancelled even with invalid error receipt",
malleate: func() {
Expand Down Expand Up @@ -1260,7 +1282,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
expError: types.ErrUpgradeNotFound,
},
{
name: "error receipt sequence less than channel upgrade sequence",
name: "sender is not authority, error receipt sequence less than channel upgrade sequence",
malleate: func() {
var ok bool
errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
Expand Down
4 changes: 0 additions & 4 deletions modules/core/04-channel/types/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,10 +847,6 @@ func (msg MsgChannelUpgradeCancel) ValidateBasic() error {
return ErrInvalidChannelIdentifier
}

if msg.ErrorReceipt.Sequence == 0 {
return errorsmod.Wrap(ibcerrors.ErrInvalidSequence, "upgrade sequence cannot be 0")
}

_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down
7 changes: 0 additions & 7 deletions modules/core/04-channel/types/msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,13 +967,6 @@ func (suite *TypesTestSuite) TestMsgChannelUpgradeCancelValidateBasic() {
},
true,
},
{
"invalid error receipt sequence",
func() {
msg.ErrorReceipt.Sequence = 0
},
false,
},
{
"missing signer address",
func() {
Expand Down
Loading