From 3321cbea6ae05d9b9706ebe9d56c75ad8966025d Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Thu, 12 Nov 2020 16:35:25 +0000
Subject: [PATCH 1/4] add cap and name checks

---
 x/capability/genesis.go                       |  2 +-
 x/capability/keeper/keeper.go                 | 45 ++++++++++++++++---
 x/capability/keeper/keeper_test.go            | 18 ++++++++
 x/capability/types/errors.go                  | 12 ++---
 .../types/misbehaviour_handle.go              |  2 +-
 5 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/x/capability/genesis.go b/x/capability/genesis.go
index 9bcca42a35f0..ba8e09dcd375 100644
--- a/x/capability/genesis.go
+++ b/x/capability/genesis.go
@@ -9,7 +9,7 @@ import (
 // InitGenesis initializes the capability module's state from a provided genesis
 // state.
 func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) {
-	if err := k.SetIndex(ctx, genState.Index); err != nil {
+	if err := k.InitializeIndex(ctx, genState.Index); err != nil {
 		panic(err)
 	}
 
diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go
index 492d56c91591..6bbe87118371 100644
--- a/x/capability/keeper/keeper.go
+++ b/x/capability/keeper/keeper.go
@@ -49,6 +49,8 @@ type (
 	}
 )
 
+// NewKeeper constructs a new CapabilityKeeper instance and initializes maps
+// for capability map and scopedModules map.
 func NewKeeper(cdc codec.BinaryMarshaler, storeKey, memKey sdk.StoreKey) *Keeper {
 	return &Keeper{
 		cdc:           cdc,
@@ -67,6 +69,9 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper {
 	if k.sealed {
 		panic("cannot scope to module via a sealed capability keeper")
 	}
+	if moduleName == "" {
+		panic("cannot scope to module name")
+	}
 
 	if _, ok := k.scopedModules[moduleName]; ok {
 		panic(fmt.Sprintf("cannot create multiple scoped keepers for the same module name: %s", moduleName))
@@ -117,10 +122,10 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) {
 	k.sealed = true
 }
 
-// SetIndex sets the index to one (or greater) in InitChain according
+// InitializeIndex sets the index to one (or greater) in InitChain according
 // to the GenesisState. It must only be called once.
 // It will panic if the provided index is 0, or if the index is already set.
-func (k Keeper) SetIndex(ctx sdk.Context, index uint64) error {
+func (k Keeper) InitializeIndex(ctx sdk.Context, index uint64) error {
 	if index == 0 {
 		panic("SetIndex requires index > 0")
 	}
@@ -200,6 +205,9 @@ func (k Keeper) InitializeCapability(ctx sdk.Context, index uint64, owners types
 // Note, namespacing is completely local, which is safe since records are prefixed
 // with the module name and no two ScopedKeeper can have the same module name.
 func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capability, error) {
+	if name == "" {
+		return nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty")
+	}
 	store := ctx.KVStore(sk.storeKey)
 
 	if _, ok := sk.GetCapability(ctx, name); ok {
@@ -247,6 +255,9 @@ func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capab
 // Note, the capability's forward mapping is indexed by a string which should
 // contain its unique memory reference.
 func (sk ScopedKeeper) AuthenticateCapability(ctx sdk.Context, cap *types.Capability, name string) bool {
+	if name == "" || cap == nil {
+		return false
+	}
 	return sk.GetCapabilityName(ctx, cap) == name
 }
 
@@ -256,6 +267,12 @@ func (sk ScopedKeeper) AuthenticateCapability(ctx sdk.Context, cap *types.Capabi
 // index. If the owner already exists, it will return an error. Otherwise, it will
 // also set a forward and reverse index for the capability and capability name.
 func (sk ScopedKeeper) ClaimCapability(ctx sdk.Context, cap *types.Capability, name string) error {
+	if cap == nil {
+		return sdkerrors.Wrap(types.ErrNilCapability, "cannot claim nil capability")
+	}
+	if name == "" {
+		return sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty")
+	}
 	// update capability owner set
 	if err := sk.addOwner(ctx, cap, name); err != nil {
 		return err
@@ -282,6 +299,9 @@ func (sk ScopedKeeper) ClaimCapability(ctx sdk.Context, cap *types.Capability, n
 // previously claimed or created. After releasing the capability, if no more
 // owners exist, the capability will be globally removed.
 func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability) error {
+	if cap == nil {
+		return sdkerrors.Wrap(types.ErrNilCapability, "cannot release nil capability")
+	}
 	name := sk.GetCapabilityName(ctx, cap)
 	if len(name) == 0 {
 		return sdkerrors.Wrap(types.ErrCapabilityNotOwned, sk.module)
@@ -321,6 +341,9 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability)
 // by name. The module is not allowed to retrieve capabilities which it does not
 // own.
 func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capability, bool) {
+	if name == "" {
+		return nil, false
+	}
 	memStore := ctx.KVStore(sk.memKey)
 
 	key := types.RevCapabilityKey(sk.module, name)
@@ -332,15 +355,14 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab
 		// to still have the capability in the go map since changes to
 		// go map do not automatically get reverted on tx failure,
 		// so we delete here to remove unnecessary values in map
-		delete(sk.capMap, index)
+		// TODO: Delete index correctly from capMap by storing some reverse lookup
+		// in-memory map. Issue: https://github.com/cosmos/cosmos-sdk/issues/7805
 		return nil, false
 	}
 
 	cap := sk.capMap[index]
 	if cap == nil {
-		// delete key from store to remove unnecessary mapping
-		memStore.Delete(key)
-		return nil, false
+		panic("capability found in memstore is missing from map")
 	}
 
 	return cap, true
@@ -349,14 +371,20 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab
 // GetCapabilityName allows a module to retrieve the name under which it stored a given
 // capability given the capability
 func (sk ScopedKeeper) GetCapabilityName(ctx sdk.Context, cap *types.Capability) string {
+	if cap == nil {
+		return ""
+	}
 	memStore := ctx.KVStore(sk.memKey)
 
 	return string(memStore.Get(types.FwdCapabilityKey(sk.module, cap)))
 }
 
-// Get all the Owners that own the capability associated with the name this ScopedKeeper uses
+// GetOwners all the Owners that own the capability associated with the name this ScopedKeeper uses
 // to refer to the capability
 func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.CapabilityOwners, bool) {
+	if name == "" {
+		return nil, false
+	}
 	cap, ok := sk.GetCapability(ctx, name)
 	if !ok {
 		return nil, false
@@ -382,6 +410,9 @@ func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.Capabilit
 // The method returns an error if either the capability or the owners cannot be
 // retreived from the memstore.
 func (sk ScopedKeeper) LookupModules(ctx sdk.Context, name string) ([]string, *types.Capability, error) {
+	if name == "" {
+		return nil, nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "cannot lookup modules with empty capability name")
+	}
 	cap, ok := sk.GetCapability(ctx, name)
 	if !ok {
 		return nil, nil, sdkerrors.Wrap(types.ErrCapabilityNotFound, name)
diff --git a/x/capability/keeper/keeper_test.go b/x/capability/keeper/keeper_test.go
index c6f1d174bdb9..d29b44e1e58b 100644
--- a/x/capability/keeper/keeper_test.go
+++ b/x/capability/keeper/keeper_test.go
@@ -38,6 +38,9 @@ func (suite *KeeperTestSuite) SetupTest() {
 
 func (suite *KeeperTestSuite) TestInitializeAndSeal() {
 	sk := suite.keeper.ScopeToModule(banktypes.ModuleName)
+	suite.Require().Panics(func() {
+		suite.keeper.ScopeToModule("")
+	})
 
 	caps := make([]*types.Capability, 5)
 	// Get Latest Index before creating new ones to sychronize indices correctly
@@ -105,6 +108,10 @@ func (suite *KeeperTestSuite) TestNewCapability() {
 	suite.Require().True(ok)
 	suite.Require().Equal(cap, got)
 	suite.Require().True(cap == got, "expected memory addresses to be equal")
+
+	cap, err = sk.NewCapability(suite.ctx, "")
+	suite.Require().Error(err)
+	suite.Require().Nil(cap)
 }
 
 func (suite *KeeperTestSuite) TestOriginalCapabilityKeeper() {
@@ -151,11 +158,15 @@ func (suite *KeeperTestSuite) TestAuthenticateCapability() {
 	badCap := types.NewCapability(100)
 	suite.Require().False(sk1.AuthenticateCapability(suite.ctx, badCap, "transfer"))
 	suite.Require().False(sk2.AuthenticateCapability(suite.ctx, badCap, "bond"))
+
+	suite.Require().False(sk1.AuthenticateCapability(suite.ctx, cap1, ""))
+	suite.Require().False(sk1.AuthenticateCapability(suite.ctx, nil, "transfer"))
 }
 
 func (suite *KeeperTestSuite) TestClaimCapability() {
 	sk1 := suite.keeper.ScopeToModule(banktypes.ModuleName)
 	sk2 := suite.keeper.ScopeToModule(stakingtypes.ModuleName)
+	sk3 := suite.keeper.ScopeToModule("foo")
 
 	cap, err := sk1.NewCapability(suite.ctx, "transfer")
 	suite.Require().NoError(err)
@@ -171,6 +182,9 @@ func (suite *KeeperTestSuite) TestClaimCapability() {
 	got, ok = sk2.GetCapability(suite.ctx, "transfer")
 	suite.Require().True(ok)
 	suite.Require().Equal(cap, got)
+
+	suite.Require().Error(sk3.ClaimCapability(suite.ctx, cap, ""))
+	suite.Require().Error(sk3.ClaimCapability(suite.ctx, nil, "transfer"))
 }
 
 func (suite *KeeperTestSuite) TestGetOwners() {
@@ -237,6 +251,8 @@ func (suite *KeeperTestSuite) TestGetOwners() {
 		}
 	}
 
+	_, ok := sk1.GetOwners(suite.ctx, "")
+	suite.Require().False(ok, "got owners from empty capability name")
 }
 
 func (suite *KeeperTestSuite) TestReleaseCapability() {
@@ -264,6 +280,8 @@ func (suite *KeeperTestSuite) TestReleaseCapability() {
 	got, ok = sk1.GetCapability(suite.ctx, "transfer")
 	suite.Require().False(ok)
 	suite.Require().Nil(got)
+
+	suite.Require().Error(sk1.ReleaseCapability(suite.ctx, nil))
 }
 
 func (suite KeeperTestSuite) TestRevertCapability() {
diff --git a/x/capability/types/errors.go b/x/capability/types/errors.go
index 93f18542608a..7c582ccbb0a5 100644
--- a/x/capability/types/errors.go
+++ b/x/capability/types/errors.go
@@ -8,9 +8,11 @@ import (
 
 // x/capability module sentinel errors
 var (
-	ErrCapabilityTaken          = sdkerrors.Register(ModuleName, 2, "capability name already taken")
-	ErrOwnerClaimed             = sdkerrors.Register(ModuleName, 3, "given owner already claimed capability")
-	ErrCapabilityNotOwned       = sdkerrors.Register(ModuleName, 4, "capability not owned by module")
-	ErrCapabilityNotFound       = sdkerrors.Register(ModuleName, 5, "capability not found")
-	ErrCapabilityOwnersNotFound = sdkerrors.Register(ModuleName, 6, "owners not found for capability")
+	ErrInvalidCapabilityName    = sdkerrors.Register(ModuleName, 2, "capability name not valid")
+	ErrNilCapability            = sdkerrors.Register(ModuleName, 3, "provided capability is nil")
+	ErrCapabilityTaken          = sdkerrors.Register(ModuleName, 4, "capability name already taken")
+	ErrOwnerClaimed             = sdkerrors.Register(ModuleName, 5, "given owner already claimed capability")
+	ErrCapabilityNotOwned       = sdkerrors.Register(ModuleName, 6, "capability not owned by module")
+	ErrCapabilityNotFound       = sdkerrors.Register(ModuleName, 7, "capability not found")
+	ErrCapabilityOwnersNotFound = sdkerrors.Register(ModuleName, 8, "owners not found for capability")
 )
diff --git a/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go b/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go
index e3793ee86383..b27cc06a45d7 100644
--- a/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go
+++ b/x/ibc/light-clients/07-tendermint/types/misbehaviour_handle.go
@@ -108,7 +108,7 @@ func checkMisbehaviourHeader(
 		chainID, _ = clienttypes.SetVersionNumber(chainID, header.GetHeight().GetVersionNumber())
 	}
 
-	// - ValidatorSet must have 2/3 similarity with trusted FromValidatorSet
+	// - ValidatorSet must have TrustLevel similarity with trusted FromValidatorSet
 	// - ValidatorSets on both headers are valid given the last trusted ValidatorSet
 	if err := tmTrustedValset.VerifyCommitLightTrusting(
 		chainID, tmCommit, clientState.TrustLevel.ToTendermint(),

From 8106fe78aef49ce533ce6cd37badeab8cc32e057 Mon Sep 17 00:00:00 2001
From: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Date: Thu, 12 Nov 2020 17:49:52 +0100
Subject: [PATCH 2/4] Update x/capability/keeper/keeper.go

---
 x/capability/keeper/keeper.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go
index 6bbe87118371..eb5a14109ba7 100644
--- a/x/capability/keeper/keeper.go
+++ b/x/capability/keeper/keeper.go
@@ -70,7 +70,7 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper {
 		panic("cannot scope to module via a sealed capability keeper")
 	}
 	if moduleName == "" {
-		panic("cannot scope to module name")
+		panic("cannot scope to an empty module name")
 	}
 
 	if _, ok := k.scopedModules[moduleName]; ok {

From 993a51c6b010e175147bad98a3897d36c744ad44 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 13 Nov 2020 10:51:15 +0000
Subject: [PATCH 3/4] address reviews

---
 CHANGELOG.md                       |  3 +++
 x/capability/keeper/keeper.go      | 13 +++++++------
 x/capability/keeper/keeper_test.go | 10 +++++-----
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index bbe06901e1ee..efaf660f3def 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,6 +65,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
   * `server/types.AppExporter` requires extra argument: `AppOptions`.
   * `server.AddCommands` requires extra argument: `addStartFlags types.ModuleInitFlags`
   * `x/crisis.NewAppModule` has a new attribute: `skipGenesisInvariants`. [PR](https://github.com/cosmos/cosmos-sdk/pull/7764)
+* [#7918](https://github.com/cosmos/cosmos-sdk/pull/7918) Add x/capability safety checks:
+  * All outward facing APIs will now check that capability is not nil and name is not empty before performing any state-machine changes
+  * `SetIndex` has been renamed to `InitializeIndex`
 
 ### Features
 
diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go
index 6bbe87118371..f0bbee73f68f 100644
--- a/x/capability/keeper/keeper.go
+++ b/x/capability/keeper/keeper.go
@@ -2,6 +2,7 @@ package keeper
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/tendermint/tendermint/libs/log"
 
@@ -69,7 +70,7 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper {
 	if k.sealed {
 		panic("cannot scope to module via a sealed capability keeper")
 	}
-	if moduleName == "" {
+	if strings.TrimSpace(moduleName) == "" {
 		panic("cannot scope to module name")
 	}
 
@@ -205,7 +206,7 @@ func (k Keeper) InitializeCapability(ctx sdk.Context, index uint64, owners types
 // Note, namespacing is completely local, which is safe since records are prefixed
 // with the module name and no two ScopedKeeper can have the same module name.
 func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capability, error) {
-	if name == "" {
+	if strings.TrimSpace(name) == "" {
 		return nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty")
 	}
 	store := ctx.KVStore(sk.storeKey)
@@ -270,7 +271,7 @@ func (sk ScopedKeeper) ClaimCapability(ctx sdk.Context, cap *types.Capability, n
 	if cap == nil {
 		return sdkerrors.Wrap(types.ErrNilCapability, "cannot claim nil capability")
 	}
-	if name == "" {
+	if strings.TrimSpace(name) == "" {
 		return sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty")
 	}
 	// update capability owner set
@@ -341,7 +342,7 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability)
 // by name. The module is not allowed to retrieve capabilities which it does not
 // own.
 func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capability, bool) {
-	if name == "" {
+	if strings.TrimSpace(name) == "" {
 		return nil, false
 	}
 	memStore := ctx.KVStore(sk.memKey)
@@ -382,7 +383,7 @@ func (sk ScopedKeeper) GetCapabilityName(ctx sdk.Context, cap *types.Capability)
 // GetOwners all the Owners that own the capability associated with the name this ScopedKeeper uses
 // to refer to the capability
 func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.CapabilityOwners, bool) {
-	if name == "" {
+	if strings.TrimSpace(name) == "" {
 		return nil, false
 	}
 	cap, ok := sk.GetCapability(ctx, name)
@@ -410,7 +411,7 @@ func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.Capabilit
 // The method returns an error if either the capability or the owners cannot be
 // retreived from the memstore.
 func (sk ScopedKeeper) LookupModules(ctx sdk.Context, name string) ([]string, *types.Capability, error) {
-	if name == "" {
+	if strings.TrimSpace(name) == "" {
 		return nil, nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "cannot lookup modules with empty capability name")
 	}
 	cap, ok := sk.GetCapability(ctx, name)
diff --git a/x/capability/keeper/keeper_test.go b/x/capability/keeper/keeper_test.go
index d29b44e1e58b..e62176a72463 100644
--- a/x/capability/keeper/keeper_test.go
+++ b/x/capability/keeper/keeper_test.go
@@ -39,7 +39,7 @@ func (suite *KeeperTestSuite) SetupTest() {
 func (suite *KeeperTestSuite) TestInitializeAndSeal() {
 	sk := suite.keeper.ScopeToModule(banktypes.ModuleName)
 	suite.Require().Panics(func() {
-		suite.keeper.ScopeToModule("")
+		suite.keeper.ScopeToModule("  ")
 	})
 
 	caps := make([]*types.Capability, 5)
@@ -109,7 +109,7 @@ func (suite *KeeperTestSuite) TestNewCapability() {
 	suite.Require().Equal(cap, got)
 	suite.Require().True(cap == got, "expected memory addresses to be equal")
 
-	cap, err = sk.NewCapability(suite.ctx, "")
+	cap, err = sk.NewCapability(suite.ctx, "   ")
 	suite.Require().Error(err)
 	suite.Require().Nil(cap)
 }
@@ -159,7 +159,7 @@ func (suite *KeeperTestSuite) TestAuthenticateCapability() {
 	suite.Require().False(sk1.AuthenticateCapability(suite.ctx, badCap, "transfer"))
 	suite.Require().False(sk2.AuthenticateCapability(suite.ctx, badCap, "bond"))
 
-	suite.Require().False(sk1.AuthenticateCapability(suite.ctx, cap1, ""))
+	suite.Require().False(sk1.AuthenticateCapability(suite.ctx, cap1, "  "))
 	suite.Require().False(sk1.AuthenticateCapability(suite.ctx, nil, "transfer"))
 }
 
@@ -183,7 +183,7 @@ func (suite *KeeperTestSuite) TestClaimCapability() {
 	suite.Require().True(ok)
 	suite.Require().Equal(cap, got)
 
-	suite.Require().Error(sk3.ClaimCapability(suite.ctx, cap, ""))
+	suite.Require().Error(sk3.ClaimCapability(suite.ctx, cap, "  "))
 	suite.Require().Error(sk3.ClaimCapability(suite.ctx, nil, "transfer"))
 }
 
@@ -251,7 +251,7 @@ func (suite *KeeperTestSuite) TestGetOwners() {
 		}
 	}
 
-	_, ok := sk1.GetOwners(suite.ctx, "")
+	_, ok := sk1.GetOwners(suite.ctx, "  ")
 	suite.Require().False(ok, "got owners from empty capability name")
 }
 

From 86fdc31379a10e1ec9436c01cbe71effde7f1b11 Mon Sep 17 00:00:00 2001
From: Aditya Sripal <adityasripal@gmail.com>
Date: Fri, 13 Nov 2020 10:55:51 +0000
Subject: [PATCH 4/4] fix missing trim

---
 x/capability/keeper/keeper.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x/capability/keeper/keeper.go b/x/capability/keeper/keeper.go
index 9df8c545341e..61de0338991c 100644
--- a/x/capability/keeper/keeper.go
+++ b/x/capability/keeper/keeper.go
@@ -256,7 +256,7 @@ func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capab
 // Note, the capability's forward mapping is indexed by a string which should
 // contain its unique memory reference.
 func (sk ScopedKeeper) AuthenticateCapability(ctx sdk.Context, cap *types.Capability, name string) bool {
-	if name == "" || cap == nil {
+	if strings.TrimSpace(name) == "" || cap == nil {
 		return false
 	}
 	return sk.GetCapabilityName(ctx, cap) == name