ibc: internal audit part 1 (cosmos#7704)

* ibc: internal audit part 1

* Update x/ibc/core/02-client/keeper/client.go

* gofmt

* fix tests

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Colin Axner <colinaxner@berkeley.edu>

x/ibc/core/02-client/abci_test.go

@@ -8,6 +8,7 @@ import (
 	client "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
+	localhosttypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/09-localhost/types"
 	ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
 )
 
@@ -25,6 +26,13 @@ func (suite *ClientTestSuite) SetupTest() {
 
 	suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
 	suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
+
+	// set localhost client
+	version := types.ParseChainID(suite.chainA.GetContext().ChainID())
+	localHostClient := localhosttypes.NewClientState(
+		suite.chainA.GetContext().ChainID(), types.NewHeight(version, uint64(suite.chainA.GetContext().BlockHeight())),
+	)
+	suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), exported.Localhost, localHostClient)
 }
 
 func TestClientTestSuite(t *testing.T) {

x/ibc/core/02-client/keeper/client.go

@@ -22,6 +22,7 @@ func (k Keeper) CreateClient(
 		return sdkerrors.Wrapf(types.ErrClientExists, "cannot create client with ID %s", clientID)
 	}
 
+	// check if consensus state is nil in case the created client is Localhost
 	if consensusState != nil {
 		k.SetClientConsensusState(ctx, clientID, clientState.GetLatestHeight(), consensusState)
 	}
@@ -52,20 +53,15 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
 	}
 
-	var (
-		consensusState  exported.ConsensusState
-		consensusHeight exported.Height
-		err             error
-	)
-
-	clientState, consensusState, err = clientState.CheckHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), header)
-
+	clientState, consensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), header)
 	if err != nil {
 		return sdkerrors.Wrapf(err, "cannot update client with ID %s", clientID)
 	}
 
 	k.SetClientState(ctx, clientID, clientState)
 
+	var consensusHeight exported.Height
+
 	// we don't set consensus state for localhost client
 	if header != nil && clientID != exported.Localhost {
 		k.SetClientConsensusState(ctx, clientID, header.GetHeight(), consensusState)
@@ -116,7 +112,7 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
 
 	err := clientState.VerifyUpgrade(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, upgradeHeight, proofUpgrade)
 	if err != nil {
-		return sdkerrors.Wrapf(err, "cannot upgrade client with ID: %s", clientID)
+		return sdkerrors.Wrapf(err, "cannot upgrade client with ID %s", clientID)
 	}
 
 	k.SetClientState(ctx, clientID, upgradedClient)
@@ -155,6 +151,10 @@ func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, misbehaviour ex
 		return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID())
 	}
 
+	if clientState.IsFrozen() && clientState.GetFrozenHeight().LTE(misbehaviour.GetHeight()) {
+		return sdkerrors.Wrapf(types.ErrInvalidMisbehaviour, "client is already frozen at height ≤ misbehaviour height (%s ≤ %s)", clientState.GetFrozenHeight(), misbehaviour.GetHeight())
+	}
+
 	clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, misbehaviour.GetClientID()), misbehaviour)
 	if err != nil {
 		return err

x/ibc/core/02-client/keeper/client_test.go

@@ -222,14 +222,14 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
 }
 
 func (suite *KeeperTestSuite) TestUpdateClientLocalhost() {
-	var localhostClient exported.ClientState = localhosttypes.NewClientState(suite.header.Header.GetChainID(), types.NewHeight(0, uint64(suite.ctx.BlockHeight())))
+	version := types.ParseChainID(suite.chainA.ChainID)
+	var localhostClient exported.ClientState = localhosttypes.NewClientState(suite.chainA.ChainID, types.NewHeight(version, uint64(suite.chainA.GetContext().BlockHeight())))
 
-	suite.ctx = suite.ctx.WithBlockHeight(suite.ctx.BlockHeight() + 1)
-
-	err := suite.keeper.UpdateClient(suite.ctx, exported.Localhost, nil)
-	suite.Require().NoError(err, err)
+	ctx := suite.chainA.GetContext().WithBlockHeight(suite.chainA.GetContext().BlockHeight() + 1)
+	err := suite.chainA.App.IBCKeeper.ClientKeeper.UpdateClient(ctx, exported.Localhost, nil)
+	suite.Require().NoError(err)
 
-	clientState, found := suite.keeper.GetClientState(suite.ctx, exported.Localhost)
+	clientState, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(ctx, exported.Localhost)
 	suite.Require().True(found)
 	suite.Require().Equal(localhostClient.GetLatestHeight().(types.Height).Increment(), clientState.GetLatestHeight())
 }

x/ibc/core/02-client/keeper/keeper_test.go

@@ -113,6 +113,13 @@ func (suite *KeeperTestSuite) SetupTest() {
 		app.StakingKeeper.SetHistoricalInfo(suite.ctx, int64(i), &hi)
 	}
 
+	// add localhost client
+	version := types.ParseChainID(suite.chainA.ChainID)
+	localHostClient := localhosttypes.NewClientState(
+		suite.chainA.ChainID, types.NewHeight(version, uint64(suite.chainA.GetContext().BlockHeight())),
+	)
+	suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), exported.Localhost, localHostClient)
+
 	queryHelper := baseapp.NewQueryServerTestHelper(suite.ctx, app.InterfaceRegistry())
 	types.RegisterQueryServer(queryHelper, app.IBCKeeper.ClientKeeper)
 	suite.queryClient = types.NewQueryClient(queryHelper)
@@ -245,15 +252,15 @@ func (suite KeeperTestSuite) TestGetAllClients() {
 	}
 
 	for i := range expClients {
-		suite.keeper.SetClientState(suite.ctx, clientIDs[i], expClients[i])
+		suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientIDs[i], expClients[i])
 	}
 
 	// add localhost client
-	localHostClient, found := suite.keeper.GetClientState(suite.ctx, exported.Localhost)
+	localHostClient, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), exported.Localhost)
 	suite.Require().True(found)
 	expClients = append(expClients, localHostClient)
 
-	clients := suite.keeper.GetAllClients(suite.ctx)
+	clients := suite.chainA.App.IBCKeeper.ClientKeeper.GetAllClients(suite.chainA.GetContext())
 	suite.Require().Len(clients, len(expClients))
 	suite.Require().Equal(expClients, clients)
 }
@@ -271,16 +278,16 @@ func (suite KeeperTestSuite) TestGetAllGenesisClients() {
 	expGenClients := make([]types.IdentifiedClientState, len(expClients))
 
 	for i := range expClients {
-		suite.keeper.SetClientState(suite.ctx, clientIDs[i], expClients[i])
+		suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientIDs[i], expClients[i])
 		expGenClients[i] = types.NewIdentifiedClientState(clientIDs[i], expClients[i])
 	}
 
 	// add localhost client
-	localHostClient, found := suite.keeper.GetClientState(suite.ctx, exported.Localhost)
+	localHostClient, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), exported.Localhost)
 	suite.Require().True(found)
 	expGenClients = append(expGenClients, types.NewIdentifiedClientState(exported.Localhost, localHostClient))
 
-	genClients := suite.keeper.GetAllGenesisClients(suite.ctx)
+	genClients := suite.chainA.App.IBCKeeper.ClientKeeper.GetAllGenesisClients(suite.chainA.GetContext())
 
 	suite.Require().Equal(expGenClients, genClients)
 }

x/ibc/core/02-client/types/genesis.go

@@ -80,7 +80,7 @@ func DefaultGenesisState() GenesisState {
 	return GenesisState{
 		Clients:          []IdentifiedClientState{},
 		ClientsConsensus: ClientsConsensusStates{},
-		CreateLocalhost:  true,
+		CreateLocalhost:  false,
 	}
 }
 

x/ibc/core/02-client/types/proposal.go

@@ -2,6 +2,7 @@ package types
 
 import (
 	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+	host "github.com/cosmos/cosmos-sdk/x/ibc/core/24-host"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
 )
 
@@ -33,7 +34,7 @@ func (cup *ClientUpdateProposal) GetTitle() string { return cup.Title }
 // GetDescription returns the description of a client update proposal.
 func (cup *ClientUpdateProposal) GetDescription() string { return cup.Description }
 
-// GetDescription returns the routing key of a client update proposal.
+// ProposalRoute returns the routing key of a client update proposal.
 func (cup *ClientUpdateProposal) ProposalRoute() string { return RouterKey }
 
 // ProposalType returns the type of a client update proposal.
@@ -46,6 +47,10 @@ func (cup *ClientUpdateProposal) ValidateBasic() error {
 		return err
 	}
 
+	if err := host.ClientIdentifierValidator(cup.ClientId); err != nil {
+		return err
+	}
+
 	header, err := UnpackHeader(cup.Header)
 	if err != nil {
 		return err

x/ibc/core/24-host/keys.go

@@ -39,11 +39,6 @@ const (
 	KeyPacketReceiptPrefix     = "receipts"
 )
 
-// KeyPrefixBytes return the key prefix bytes from a URL string format
-func KeyPrefixBytes(prefix int) []byte {
-	return []byte(fmt.Sprintf("%d/", prefix))
-}
-
 // FullClientPath returns the full path of a specific client path in the format:
 // "clients/{clientID}/{path}" as a string.
 func FullClientPath(clientID string, path string) string {
@@ -87,15 +82,15 @@ func KeyConsensusState(height exported.Height) []byte {
 
 // ClientConnectionsPath defines a reverse mapping from clients to a set of connections
 func ClientConnectionsPath(clientID string) string {
-	return fmt.Sprintf("clients/%s/connections", clientID)
+	return fmt.Sprintf("%s/%s/connections", KeyClientStorePrefix, clientID)
 }
 
 // ConnectionPath defines the path under which connection paths are stored
 func ConnectionPath(connectionID string) string {
 	return fmt.Sprintf("%s/%s", KeyConnectionPrefix, connectionID)
 }
 
-// KeyClientConnections returns the store key for the connectios of a given client
+// KeyClientConnections returns the store key for the connections of a given client
 func KeyClientConnections(clientID string) []byte {
 	return []byte(ClientConnectionsPath(clientID))
 }
@@ -108,11 +103,6 @@ func KeyConnection(connectionID string) []byte {
 // ICS04
 // The following paths are the keys to the store as defined in https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#store-paths
 
-// GetChannelPortsKeysPrefix returns the prefix bytes for ICS04 and ICS05 iterators
-func GetChannelPortsKeysPrefix(prefix int) []byte {
-	return []byte(fmt.Sprintf("%d/ports/", prefix))
-}
-
 // ChannelPath defines the path under which channels are stored
 func ChannelPath(portID, channelID string) string {
 	return fmt.Sprintf("%s/", KeyChannelPrefix) + channelPath(portID, channelID)

x/ibc/core/24-host/parse.go

@@ -6,16 +6,6 @@ import (
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
-// RemovePath is an util function to remove a path from a set.
-func RemovePath(paths []string, path string) ([]string, bool) {
-	for i, p := range paths {
-		if p == path {
-			return append(paths[:i], paths[i+1:]...), true
-		}
-	}
-	return paths, false
-}
-
 // ParseConnectionPath returns the connection ID from a full path. It returns
 // an error if the provided path is invalid.
 func ParseConnectionPath(path string) (string, error) {

x/ibc/core/24-host/validate.go

@@ -56,29 +56,29 @@ func defaultIdentifierValidator(id string, min, max int) error { //nolint:unpara
 }
 
 // ClientIdentifierValidator is the default validator function for Client identifiers.
-// A valid Identifier must be between 9-20 characters and only contain lowercase
-// alphabetic characters,
+// A valid Identifier must be between 9-64 characters and only contain lowercase
+// alphabetic characters.
 func ClientIdentifierValidator(id string) error {
 	return defaultIdentifierValidator(id, 9, DefaultMaxCharacterLength)
 }
 
 // ConnectionIdentifierValidator is the default validator function for Connection identifiers.
-// A valid Identifier must be between 10-20 characters and only contain lowercase
-// alphabetic characters,
+// A valid Identifier must be between 10-64 characters and only contain lowercase
+// alphabetic characters.
 func ConnectionIdentifierValidator(id string) error {
 	return defaultIdentifierValidator(id, 10, DefaultMaxCharacterLength)
 }
 
 // ChannelIdentifierValidator is the default validator function for Channel identifiers.
-// A valid Identifier must be between 10-20 characters and only contain lowercase
-// alphabetic characters,
+// A valid Identifier must be between 10-64 characters and only contain lowercase
+// alphabetic characters.
 func ChannelIdentifierValidator(id string) error {
 	return defaultIdentifierValidator(id, 10, DefaultMaxCharacterLength)
 }
 
 // PortIdentifierValidator is the default validator function for Port identifiers.
-// A valid Identifier must be between 2-20 characters and only contain lowercase
-// alphabetic characters,
+// A valid Identifier must be between 2-64 characters and only contain lowercase
+// alphabetic characters.
 func PortIdentifierValidator(id string) error {
 	return defaultIdentifierValidator(id, 2, DefaultMaxCharacterLength)
 }

x/ibc/core/handler.go

@@ -24,6 +24,10 @@ func NewHandler(k keeper.Keeper) sdk.Handler {
 			res, err := k.UpdateClient(sdk.WrapSDKContext(ctx), msg)
 			return sdk.WrapServiceResult(ctx, res, err)
 
+		case *clienttypes.MsgUpgradeClient:
+			res, err := k.UpgradeClient(sdk.WrapSDKContext(ctx), msg)
+			return sdk.WrapServiceResult(ctx, res, err)
+
 		case *clienttypes.MsgSubmitMisbehaviour:
 			res, err := k.SubmitMisbehaviour(sdk.WrapSDKContext(ctx), msg)
 			return sdk.WrapServiceResult(ctx, res, err)

x/ibc/core/keeper/msg_server.go

@@ -84,10 +84,6 @@ func (k Keeper) UpgradeClient(goCtx context.Context, msg *clienttypes.MsgUpgrade
 		return nil, err
 	}
 
-	if err := upgradedClient.Validate(); err != nil {
-		return nil, err
-	}
-
 	if err = k.ClientKeeper.UpgradeClient(ctx, msg.ClientId, upgradedClient, msg.UpgradeHeight, msg.ProofUpgrade); err != nil {
 		return nil, err
 	}

x/ibc/core/keeper/msg_server_test.go

@@ -688,34 +688,6 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			},
 			expPass: false,
 		},
-		{
-			name: "invalid clientstate",
-			setup: func() {
-
-				upgradedClient = ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, ibctesting.DefaultConsensusParams, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
-
-				// upgrade Height is at next block
-				upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))
-
-				// zero custom fields and store in upgrade store
-				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetVersionHeight()), upgradedClient)
-
-				// commit upgrade store changes and update clients
-
-				suite.coordinator.CommitBlock(suite.chainB)
-				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
-				suite.Require().NoError(err)
-
-				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
-				suite.Require().True(found)
-
-				proofUpgrade, _ := suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight())
-
-				msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, upgradeHeight, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
-				suite.Require().NoError(err)
-			},
-			expPass: false,
-		},
 		{
 			name: "VerifyUpgrade fails",
 			setup: func() {