ICS4: fix off by one error in non-crossing-hello case (#5722)

* fix off by one error in non-crossing-hello case * Update modules/core/04-channel/keeper/upgrade.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * chore: formatting for linter ci * Update modules/core/04-channel/keeper/upgrade.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * lint --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
cosmos · Jan 25, 2024 · bddbe49 · bddbe49
1 parent bb6c7d8
commit bddbe49
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 20 deletions.
diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go
@@ -123,38 +123,45 @@ func (k Keeper) ChanUpgradeTry(
 	}
 
 	var (
-		err             error
-		upgrade         types.Upgrade
-		isCrossingHello bool
+		err                     error
+		upgrade                 types.Upgrade
+		isCrossingHello         bool
+		expectedUpgradeSequence uint64
 	)
 
 	upgrade, isCrossingHello = k.GetUpgrade(ctx, portID, channelID)
-	if counterpartyUpgradeSequence < channel.UpgradeSequence {
+	if isCrossingHello {
+		expectedUpgradeSequence = channel.UpgradeSequence
+	} else {
+		// at the end of the TRY step, the current upgrade sequence will be incremented in the non-crossing
+		// hello case due to calling chanUpgradeInit, we should use this expected upgrade sequence for
+		// sequence mismatch comparison
+		expectedUpgradeSequence = channel.UpgradeSequence + 1
+	}
+	if counterpartyUpgradeSequence < expectedUpgradeSequence {
 		// In this case, the counterparty upgrade is outdated. We want to force the counterparty
 		// to abort their upgrade and come back to sync with our own upgrade sequence.
-		var upgradeSequence uint64
-		if isCrossingHello {
-			// In the crossing hello case, we already have an upgrade but it is at a higher sequence than the counterparty.
-			// Thus, our upgrade should take priority. We force the counterparty to abort their upgrade by invalidating all counterparty
-			// upgrade attempts below our own sequence by setting errorReceipt to upgradeSequence - 1.
-			// The upgrade handshake may then proceed on the counterparty with our sequence
-			upgradeSequence = channel.UpgradeSequence - 1
-		} else {
-			// In the case, that we are in a non-crossing hello (i.e. upgrade does not exist on our side),
-			// the sequence on both sides should move to a fresh sequence on the next upgrade attempt.
-			// Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty
-			// to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either
-			// side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1)
-			upgradeSequence = channel.UpgradeSequence
-		}
+
+		// In the crossing hello case, we already have an upgrade but it is at a higher sequence than the counterparty.
+		// Thus, our upgrade should take priority. We force the counterparty to abort their upgrade by invalidating all counterparty
+		// upgrade attempts below our own sequence by setting errorReceipt to upgradeSequence - 1.
+		// Since our channel upgrade sequence was incremented in the previous step, the counterparty will be forced to abort their upgrade
+		// and we will be able to proceed with our own upgrade.
+		// The upgrade handshake may then proceed on the counterparty with our sequence
+		// In the non-crossing hello (i.e. upgrade does not exist on our side),
+		// the sequence on both sides should move to a fresh sequence on the next upgrade attempt.
+		// Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty
+		// to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either
+		// side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1).
+		// Note that expectedUpgradeSequence - 1 == channel.UpgradeSequence in the non-crossing hello case.
 
 		// NOTE: Two possible outcomes may occur in this scenario.
 		// The ChanUpgradeCancel datagram may reach the counterparty first, which will cause the counterparty to cancel. The counterparty
 		// may then receive a TRY with our channel upgrade sequence and correctly increment their sequence to become synced with our upgrade attempt.
 		// The ChanUpgradeTry message may arrive first, in this case, **IF** the upgrade fields are mutually compatible; the counterparty will simply
 		// fast forward their sequence to our own and continue the upgrade. The following ChanUpgradeCancel message will be rejected as it is below the current sequence.
 
-		return channel, upgrade, types.NewUpgradeError(upgradeSequence, errorsmod.Wrapf(
+		return channel, upgrade, types.NewUpgradeError(expectedUpgradeSequence-1, errorsmod.Wrapf(
 			types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence < current upgrade sequence (%d < %d)", counterpartyUpgradeSequence, channel.UpgradeSequence,
 		))
 	}

diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go
@@ -237,6 +237,30 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() {
 			// channel sequence will be returned so that counterparty inits on completely fresh sequence for both sides
 			types.NewUpgradeError(5, types.ErrInvalidUpgradeSequence),
 		},
+		{
+			"fails due to mismatch in upgrade sequences: chainB is on incremented sequence without an upgrade indicating it has already processed upgrade at this sequence.",
+			func() {
+				channel := path.EndpointB.GetChannel()
+				channel.UpgradeSequence = 1
+				errorReceipt := types.NewUpgradeError(1, types.ErrInvalidUpgrade)
+				suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.WriteErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)
+				path.EndpointB.SetChannel(channel)
+			},
+			types.NewUpgradeError(1, types.ErrInvalidUpgradeSequence),
+		},
+		{
+			"fails due to mismatch in upgrade sequences, crossing hello with the TRY chain having a higher sequence",
+			func() {
+				channel := path.EndpointB.GetChannel()
+				channel.UpgradeSequence = 4
+				path.EndpointB.SetChannel(channel)
+
+				// upgrade sequence is 5 after this call
+				err := path.EndpointB.ChanUpgradeInit()
+				suite.Require().NoError(err)
+			},
+			types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence),
+		},
 		{
 			// ChainA(Sequence: 0, mock-version-v2), ChainB(Sequence: 0, mock-version-v3)
 			// ChainA.INIT(Sequence: 1)