feat!: add protection against accidental downgrades

CHANGELOG.md

@@ -57,6 +57,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (deps) [\#10210](https://github.com/cosmos/cosmos-sdk/pull/10210) Bump Tendermint to [v0.35.0](https://github.com/tendermint/tendermint/releases/tag/v0.35.0).
 * [\#10486](https://github.com/cosmos/cosmos-sdk/pull/10486) store/cachekv's `Store.Write` conservatively looks up keys, but also uses the [map clearing idiom](https://bencher.orijtech.com/perfclinic/mapclearing/) to reduce the RAM usage, CPU time usage, and garbage collection pressure from clearing maps, instead of allocating new maps.
+* [\#10407](https://github.com/cosmos/cosmos-sdk/pull/10407) Add validation to `x/upgrade` module's `BeginBlock` to check accidental binary downgrades
 
 ### API Breaking Changes
 

x/upgrade/abci.go

@@ -12,6 +12,16 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
+// downgradeVerified is a variable that tells if we've already sanity checked that this binary version isn't being used against an old state.
+// If so, we dont run that check every beginblock.
+var downgradeVerified = false
+
+// ResetDowngradeVerified resets downgradeVerified.
+// Note: It should be use only for testing purposes.
+func ResetDowngradeVerified() {
+	downgradeVerified = false
+}
+
 // BeginBlock will check if there is a scheduled plan and if it is ready to be executed.
 // If the current height is in the provided set of heights to skip, it will skip and clear the upgrade plan.
 // If it is ready, it will execute it if the handler is installed, and panic/abort otherwise.
@@ -22,7 +32,24 @@ import (
 // skipUpgradeHeightArray is a set of block heights for which the upgrade must be skipped
 func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) {
 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)
+
 	plan, found := k.GetUpgradePlan(ctx)
+
+	if !downgradeVerified {
+		downgradeVerified = true
+		lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx)
+		// This check will make sure that we are using a valid binary.
+		// It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade.
+		// 1. If there is no scheduled upgrade.
+		// 2. If the plan is not ready.
+		// 3. If the plan is ready and skip upgrade height is set for current height.
+		if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) {
+			if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) {
+				panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", ctx.ConsensusParams().Version.AppVersion, lastAppliedPlan))
+			}
+		}
+	}
+
 	if !found {
 		return
 	}

x/upgrade/abci_test.go

@@ -410,3 +410,94 @@ func TestDumpUpgradeInfoToFile(t *testing.T) {
 	err = os.Remove(upgradeInfoFilePath)
 	require.Nil(err)
 }
+
+func TestBinaryVersion(t *testing.T) {
+	var skipHeight int64 = 15
+	s := setupTest(t, 10, map[int64]bool{skipHeight: true})
+
+	testCases := []struct {
+		name        string
+		preRun      func() (sdk.Context, abci.RequestBeginBlock)
+		expectPanic bool
+	}{
+		{
+			"test not panic: no scheduled upgrade or applied upgrade is present",
+			func() (sdk.Context, abci.RequestBeginBlock) {
+				req := abci.RequestBeginBlock{Header: s.ctx.BlockHeader()}
+				return s.ctx, req
+			},
+			false,
+		},
+		{
+			"test not panic: upgrade handler is present for last applied upgrade",
+			func() (sdk.Context, abci.RequestBeginBlock) {
+				s.keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+					return vm, nil
+				})
+
+				err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}})
+				require.Nil(t, err)
+
+				newCtx := s.ctx.WithBlockHeight(12)
+				s.keeper.ApplyUpgrade(newCtx, types.Plan{
+					Name:   "test0",
+					Height: 12,
+				})
+
+				req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
+				return newCtx, req
+			},
+			false,
+		},
+		{
+			"test panic: no upgrade handler is present for last upgrade",
+			func() (sdk.Context, abci.RequestBeginBlock) {
+
+				newCtx := s.ctx.WithBlockHeight(13)
+				req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
+				upgrade.ResetDowngradeVerified()
+
+				// delete last applied upgrade
+				s.keeper.DeleteUpgradeHandler("test0")
+				return newCtx, req
+			},
+			true,
+		},
+		{
+			"test panic: no upgrade handler is present for last upgrade with skip upgrade",
+			func() (sdk.Context, abci.RequestBeginBlock) {
+				upgrade.ResetDowngradeVerified()
+
+				newCtx := s.ctx.WithBlockHeight(15)
+				req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
+				return newCtx, req
+			},
+			true,
+		},
+		{
+			"test panic: upgrade needed",
+			func() (sdk.Context, abci.RequestBeginBlock) {
+				err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: 13}})
+				require.Nil(t, err)
+
+				newCtx := s.ctx.WithBlockHeight(13)
+				req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
+				return newCtx, req
+			},
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		ctx, req := tc.preRun()
+		if tc.expectPanic {
+			require.Panics(t, func() {
+				s.module.BeginBlock(ctx, req)
+			})
+		} else {
+			require.NotPanics(t, func() {
+				s.module.BeginBlock(ctx, req)
+			})
+		}
+	}
+}

x/upgrade/keeper/keeper.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/cosmos/cosmos-sdk/types/kv"
 	"github.com/cosmos/cosmos-sdk/types/module"
 	xp "github.com/cosmos/cosmos-sdk/x/upgrade/exported"
 	"github.com/cosmos/cosmos-sdk/x/upgrade/types"
@@ -58,6 +59,11 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl
 	k.upgradeHandlers[name] = upgradeHandler
 }
 
+// DeleteUpgradeHandler removes an UpgradeHandler for the given upgrade name
+func (k Keeper) DeleteUpgradeHandler(name string) {
+	delete(k.upgradeHandlers, name)
+}
+
 // setProtocolVersion sets the protocol version to state
 func (k Keeper) setProtocolVersion(ctx sdk.Context, v uint64) {
 	store := ctx.KVStore(k.storeKey)
@@ -227,6 +233,23 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([]
 	return bz, true
 }
 
+// GetLastCompletedUpgrade returns the last applied upgrade name and height.
+func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) {
+	iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte})
+	defer iter.Close()
+	if iter.Valid() {
+		return parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(iter.Value()))
+	}
+
+	return "", 0
+}
+
+// parseDoneKey - split upgrade name from the done key
+func parseDoneKey(key []byte) string {
+	kv.AssertKeyAtLeastLength(key, 2)
+	return string(key[1:])
+}
+
 // GetDoneHeight returns the height at which the given upgrade was executed
 func (k Keeper) GetDoneHeight(ctx sdk.Context, name string) int64 {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte{types.DoneByte})

x/upgrade/keeper/keeper_test.go

@@ -232,6 +232,45 @@ func (s *KeeperTestSuite) TestMigrations() {
 	s.Require().Equal(vmBefore["bank"]+1, vm["bank"])
 }
 
+func (s *KeeperTestSuite) TestLastCompletedUpgrade() {
+	keeper := s.app.UpgradeKeeper
+	require := s.Require()
+
+	s.T().Log("verify empty name if applied upgrades are empty")
+	name, height := keeper.GetLastCompletedUpgrade(s.ctx)
+	require.Equal("", name)
+	require.Equal(int64(0), height)
+
+	keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+		return vm, nil
+	})
+
+	keeper.ApplyUpgrade(s.ctx, types.Plan{
+		Name:   "test0",
+		Height: 10,
+	})
+
+	s.T().Log("verify valid upgrade name and height")
+	name, height = keeper.GetLastCompletedUpgrade(s.ctx)
+	require.Equal("test0", name)
+	require.Equal(int64(10), height)
+
+	keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+		return vm, nil
+	})
+
+	newCtx := s.ctx.WithBlockHeight(15)
+	keeper.ApplyUpgrade(newCtx, types.Plan{
+		Name:   "test1",
+		Height: 15,
+	})
+
+	s.T().Log("verify valid upgrade name and height with multiple upgrades")
+	name, height = keeper.GetLastCompletedUpgrade(newCtx)
+	require.Equal("test1", name)
+	require.Equal(int64(15), height)
+}
+
 func TestKeeperTestSuite(t *testing.T) {
 	suite.Run(t, new(KeeperTestSuite))
 }