imp: add empty address check on authority field (cosmos#4713)

notional-labs · Sep 19, 2023 · 94d6b7c · 94d6b7c
1 parent 105e0bc
commit 94d6b7c
Show file tree

Hide file tree

Showing 8 changed files with 238 additions and 11 deletions.
diff --git a/modules/apps/27-interchain-accounts/controller/keeper/keeper.go b/modules/apps/27-interchain-accounts/controller/keeper/keeper.go
@@ -52,6 +52,10 @@ func NewKeeper(
 		legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable())
 	}
 
+	if strings.TrimSpace(authority) == "" {
+		panic(fmt.Errorf("authority must be non-empty"))
+	}
+
 	return Keeper{
 		storeKey:       key,
 		cdc:            cdc,

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go
@@ -8,6 +8,7 @@ import (
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 
+	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/keeper"
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
 	genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types"
 	icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
@@ -107,6 +108,58 @@ func TestKeeperTestSuite(t *testing.T) {
 	testifysuite.Run(t, new(KeeperTestSuite))
 }
 
+func (suite *KeeperTestSuite) TestNewKeeper() {
+	testCases := []struct {
+		name          string
+		instantiateFn func()
+		expPass       bool
+	}{
+		{"success", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				suite.chainA.GetSimApp().ScopedICAControllerKeeper,
+				suite.chainA.GetSimApp().MsgServiceRouter(),
+				suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
+			)
+		}, true},
+		{"failure: empty authority", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				suite.chainA.GetSimApp().ScopedICAControllerKeeper,
+				suite.chainA.GetSimApp().MsgServiceRouter(),
+				"", // authority
+			)
+		}, false},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.SetupTest()
+
+		suite.Run(tc.name, func() {
+			if tc.expPass {
+				suite.Require().NotPanics(
+					tc.instantiateFn,
+				)
+			} else {
+				suite.Require().Panics(
+					tc.instantiateFn,
+				)
+			}
+		})
+	}
+}
+
 func (suite *KeeperTestSuite) TestGetAllPorts() {
 	suite.SetupTest()
 

diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper.go b/modules/apps/27-interchain-accounts/host/keeper/keeper.go
@@ -59,6 +59,10 @@ func NewKeeper(
 		legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable())
 	}
 
+	if strings.TrimSpace(authority) == "" {
+		panic(fmt.Errorf("authority must be non-empty"))
+	}
+
 	return Keeper{
 		storeKey:       key,
 		cdc:            cdc,

diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
@@ -6,7 +6,10 @@ import (
 
 	testifysuite "github.com/stretchr/testify/suite"
 
+	authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
+
 	genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types"
+	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/keeper"
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
 	ibcfeekeeper "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/keeper"
@@ -127,6 +130,74 @@ func TestKeeperTestSuite(t *testing.T) {
 	testifysuite.Run(t, new(KeeperTestSuite))
 }
 
+func (suite *KeeperTestSuite) TestNewKeeper() {
+	testCases := []struct {
+		name          string
+		instantiateFn func()
+		expPass       bool
+	}{
+		{"success", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				suite.chainA.GetSimApp().AccountKeeper,
+				suite.chainA.GetSimApp().ScopedICAHostKeeper,
+				suite.chainA.GetSimApp().MsgServiceRouter(),
+				suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(),
+			)
+		}, true},
+		{"failure: interchain accounts module account does not exist", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				authkeeper.AccountKeeper{}, // empty account keeper
+				suite.chainA.GetSimApp().ScopedICAHostKeeper,
+				suite.chainA.GetSimApp().MsgServiceRouter(),
+				suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(),
+			)
+		}, false},
+		{"failure: empty mock staking keeper", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				suite.chainA.GetSimApp().AccountKeeper,
+				suite.chainA.GetSimApp().ScopedICAHostKeeper,
+				suite.chainA.GetSimApp().MsgServiceRouter(),
+				"", // authority
+			)
+		}, false},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.SetupTest()
+
+		suite.Run(tc.name, func() {
+			if tc.expPass {
+				suite.Require().NotPanics(
+					tc.instantiateFn,
+				)
+			} else {
+				suite.Require().Panics(
+					tc.instantiateFn,
+				)
+			}
+		})
+	}
+}
+
 func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
 	suite.SetupTest()
 

diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go
@@ -63,6 +63,10 @@ func NewKeeper(
 		legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable())
 	}
 
+	if strings.TrimSpace(authority) == "" {
+		panic(fmt.Errorf("authority must be non-empty"))
+	}
+
 	return Keeper{
 		cdc:            cdc,
 		storeKey:       key,

diff --git a/modules/apps/transfer/keeper/keeper_test.go b/modules/apps/transfer/keeper/keeper_test.go
@@ -12,7 +12,9 @@ import (
 	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
 
+	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper"
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
 	channelkeeper "github.com/cosmos/ibc-go/v8/modules/core/04-channel/keeper"
 	ibctesting "github.com/cosmos/ibc-go/v8/testing"
@@ -43,6 +45,74 @@ func TestKeeperTestSuite(t *testing.T) {
 	testifysuite.Run(t, new(KeeperTestSuite))
 }
 
+func (suite *KeeperTestSuite) TestNewKeeper() {
+	testCases := []struct {
+		name          string
+		instantiateFn func()
+		expPass       bool
+	}{
+		{"success", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.ModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				suite.chainA.GetSimApp().AccountKeeper,
+				suite.chainA.GetSimApp().BankKeeper,
+				suite.chainA.GetSimApp().ScopedTransferKeeper,
+				suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
+			)
+		}, true},
+		{"failure: transfer module account does not exist", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.ModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				authkeeper.AccountKeeper{}, // empty account keeper
+				suite.chainA.GetSimApp().BankKeeper,
+				suite.chainA.GetSimApp().ScopedTransferKeeper,
+				suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
+			)
+		}, false},
+		{"failure: empty authority", func() {
+			keeper.NewKeeper(
+				suite.chainA.GetSimApp().AppCodec(),
+				suite.chainA.GetSimApp().GetKey(types.StoreKey),
+				suite.chainA.GetSimApp().GetSubspace(types.ModuleName),
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
+				&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
+				suite.chainA.GetSimApp().AccountKeeper,
+				suite.chainA.GetSimApp().BankKeeper,
+				suite.chainA.GetSimApp().ScopedTransferKeeper,
+				"", // authority
+			)
+		}, false},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.SetupTest()
+
+		suite.Run(tc.name, func() {
+			if tc.expPass {
+				suite.Require().NotPanics(
+					tc.instantiateFn,
+				)
+			} else {
+				suite.Require().Panics(
+					tc.instantiateFn,
+				)
+			}
+		})
+	}
+}
+
 func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() {
 	const denom = "atom"
 	var expAmount sdkmath.Int

diff --git a/modules/core/keeper/keeper.go b/modules/core/keeper/keeper.go
@@ -3,6 +3,7 @@ package keeper
 import (
 	"fmt"
 	"reflect"
+	"strings"
 
 	storetypes "cosmossdk.io/store/types"
 
@@ -64,6 +65,10 @@ func NewKeeper(
 		panic(fmt.Errorf("cannot initialize IBC keeper: empty scoped keeper"))
 	}
 
+	if strings.TrimSpace(authority) == "" {
+		panic(fmt.Errorf("authority must be non-empty"))
+	}
+
 	clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper)
 	connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, clientKeeper)
 	portKeeper := portkeeper.NewKeeper(scopedKeeper)

diff --git a/modules/core/keeper/keeper_test.go b/modules/core/keeper/keeper_test.go
@@ -64,17 +64,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
 		stakingKeeper  clienttypes.StakingKeeper
 		upgradeKeeper  clienttypes.UpgradeKeeper
 		scopedKeeper   capabilitykeeper.ScopedKeeper
-		newIBCKeeperFn = func() {
-			ibckeeper.NewKeeper(
-				suite.chainA.GetSimApp().AppCodec(),
-				suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey),
-				suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName),
-				stakingKeeper,
-				upgradeKeeper,
-				scopedKeeper,
-				suite.chainA.App.GetIBCKeeper().GetAuthority(),
-			)
-		}
+		newIBCKeeperFn func()
 	)
 
 	testCases := []struct {
@@ -113,6 +103,19 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
 
 			scopedKeeper = emptyScopedKeeper
 		}, false},
+		{"failure: empty authority", func() {
+			newIBCKeeperFn = func() {
+				ibckeeper.NewKeeper(
+					suite.chainA.GetSimApp().AppCodec(),
+					suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey),
+					suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName),
+					stakingKeeper,
+					upgradeKeeper,
+					scopedKeeper,
+					"", // authority
+				)
+			}
+		}, false},
 		{"success: replace stakingKeeper with non-empty MockStakingKeeper", func() {
 			// use a different implementation of clienttypes.StakingKeeper
 			mockStakingKeeper := MockStakingKeeper{"not empty"}
@@ -126,6 +129,19 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
 		suite.SetupTest()
 
 		suite.Run(tc.name, func() {
+			// set default behaviour
+			newIBCKeeperFn = func() {
+				ibckeeper.NewKeeper(
+					suite.chainA.GetSimApp().AppCodec(),
+					suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey),
+					suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName),
+					stakingKeeper,
+					upgradeKeeper,
+					scopedKeeper,
+					suite.chainA.App.GetIBCKeeper().GetAuthority(),
+				)
+			}
+
 			stakingKeeper = suite.chainA.GetSimApp().StakingKeeper
 			upgradeKeeper = suite.chainA.GetSimApp().UpgradeKeeper
 			scopedKeeper = suite.chainA.GetSimApp().ScopedIBCKeeper