chore: ica audit nitpicks (#483)

* updating version validation with corrections * removing unused methods from expected keeper interfaces * updating validate version error msg * update to use ErrInvalidVersion in favour of ErrInvalidAccountAddress
cosmos · Oct 13, 2021 · 4db5c83 · 4db5c83
1 parent cd263c7
commit 4db5c83
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 47 deletions.
diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go
@@ -53,8 +53,8 @@ func (k Keeper) OnChanOpenInit(
 		return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "expected %s, got %s", types.PortID, counterparty.PortId)
 	}
 
-	if err := types.ValidateVersion(version); err != nil {
-		return sdkerrors.Wrap(err, "version validation failed")
+	if version != types.VersionPrefix {
+		return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.VersionPrefix, version)
 	}
 
 	existingChannelID, found := k.GetActiveChannel(ctx, portID)
@@ -111,8 +111,8 @@ func (k Keeper) OnChanOpenTry(
 		return sdkerrors.Wrap(err, "version validation failed")
 	}
 
-	if err := types.ValidateVersion(counterpartyVersion); err != nil {
-		return sdkerrors.Wrap(err, "counterparty version validation failed")
+	if counterpartyVersion != types.VersionPrefix {
+		return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.VersionPrefix, version)
 	}
 
 	// On the host chain the capability may only be claimed during the OnChanOpenTry
@@ -129,7 +129,7 @@ func (k Keeper) OnChanOpenTry(
 	}
 
 	if parsedAddr != accAddr.String() {
-		return sdkerrors.Wrapf(types.ErrInvalidAccountAddress, "version contains invalid account address: expected %s, got %s", parsedAddr, accAddr)
+		return sdkerrors.Wrapf(types.ErrInvalidVersion, "version contains invalid account address: expected %s, got %s", parsedAddr, accAddr)
 	}
 
 	// Register interchain account if it does not already exist

diff --git a/modules/apps/27-interchain-accounts/types/expected_keepers.go b/modules/apps/27-interchain-accounts/types/expected_keepers.go
@@ -5,19 +5,13 @@ import (
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
-	connectiontypes "github.com/cosmos/ibc-go/v2/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
 	ibcexported "github.com/cosmos/ibc-go/v2/modules/core/exported"
 )
 
-type Router interface {
-	Route(ctx sdk.Context, path string) sdk.Handler
-}
-
-// AccountKeeper defines the contract required for account APIs.
+// AccountKeeper defines the expected account keeper
 type AccountKeeper interface {
 	NewAccount(ctx sdk.Context, acc authtypes.AccountI) authtypes.AccountI
-	NewAccountWithAddress(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
 	GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
 	SetAccount(ctx sdk.Context, acc authtypes.AccountI)
 	GetModuleAccount(ctx sdk.Context, name string) authtypes.ModuleAccountI
@@ -29,24 +23,11 @@ type ChannelKeeper interface {
 	GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool)
 	GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool)
 	SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error
-	ChanCloseInit(ctx sdk.Context, portID, channelID string, chanCap *capabilitytypes.Capability) error
-	ChanOpenInit(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string, portCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string) (string, *capabilitytypes.Capability, error)
 	CounterpartyHops(ctx sdk.Context, channel channeltypes.Channel) ([]string, bool)
 }
 
-// ClientKeeper defines the expected IBC client keeper
-type ClientKeeper interface {
-	GetClientState(ctx sdk.Context, clientID string) (ibcexported.ClientState, bool)
-}
-
-// ConnectionKeeper defines the expected IBC connection keeper
-type ConnectionKeeper interface {
-	GetConnection(ctx sdk.Context, connectionID string) (connection connectiontypes.ConnectionEnd, found bool)
-}
-
 // PortKeeper defines the expected IBC port keeper
 type PortKeeper interface {
 	BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability
 	IsBound(ctx sdk.Context, portID string) bool
-	LookupModuleByPort(ctx sdk.Context, portID string) (string, *capabilitytypes.Capability, error)
 }
diff --git a/modules/apps/27-interchain-accounts/types/validate.go b/modules/apps/27-interchain-accounts/types/validate.go
@@ -17,27 +17,24 @@ var IsValidAddr = regexp.MustCompile("^[a-zA-Z0-9]*$").MatchString
 // ValidateVersion performs basic validation of the provided ics27 version string.
 // An ics27 version string may include an optional account address as per [TODO: Add spec when available]
 // ValidateVersion first attempts to split the version string using the standard delimiter, then asserts a supported
-// version prefix is included, followed by additional checks which enforce constraints on the optional account address.
-// When no delimiter is present, only the version prefix is validated
+// version prefix is included, followed by additional checks which enforce constraints on the account address.
 func ValidateVersion(version string) error {
 	s := strings.Split(version, Delimiter)
 
+	if len(s) != 2 {
+		return sdkerrors.Wrapf(ErrInvalidVersion, "expected format <app-version%saccount-address>, got %s", Delimiter, version)
+	}
+
 	if s[0] != VersionPrefix {
 		return sdkerrors.Wrapf(ErrInvalidVersion, "expected %s, got %s", VersionPrefix, s[0])
 	}
 
-	if len(s) > 1 {
-		if len(s) != 2 {
-			return sdkerrors.Wrap(ErrInvalidAccountAddress, "unexpected address format")
-		}
-
-		if !IsValidAddr(s[1]) || len(s[1]) == 0 || len(s[1]) > DefaultMaxAddrLength {
-			return sdkerrors.Wrapf(
-				ErrInvalidAccountAddress,
-				"address must contain strictly alphanumeric characters, not exceeding %d characters in length",
-				DefaultMaxAddrLength,
-			)
-		}
+	if !IsValidAddr(s[1]) || len(s[1]) == 0 || len(s[1]) > DefaultMaxAddrLength {
+		return sdkerrors.Wrapf(
+			ErrInvalidAccountAddress,
+			"address must contain strictly alphanumeric characters, not exceeding %d characters in length",
+			DefaultMaxAddrLength,
+		)
 	}
 
 	return nil

diff --git a/modules/apps/27-interchain-accounts/types/validate_test.go b/modules/apps/27-interchain-accounts/types/validate_test.go
@@ -16,9 +16,14 @@ func (suite *TypesTestSuite) TestValidateVersion() {
 			true,
 		},
 		{
-			"success - version prefix only",
-			types.VersionPrefix,
-			true,
+			"unexpected version string format",
+			"invalid-version-string-format",
+			false,
+		},
+		{
+			"unexpected version string format, additional delimiter",
+			types.NewAppVersion(types.VersionPrefix, "cosmos17dtl0mjt3t77kpu.hg2edqzjpszulwhgzuj9ljs"),
+			false,
 		},
 		{
 			"invalid version",
@@ -40,11 +45,6 @@ func (suite *TypesTestSuite) TestValidateVersion() {
 			types.NewAppVersion(types.VersionPrefix, "-_-"),
 			false,
 		},
-		{
-			"invalid account address - address contains additional delimiter",
-			types.NewAppVersion(types.VersionPrefix, "cosmos17dtl0mjt3t77kpu|hg2edqzjpszulwhgzuj9ljs"),
-			false,
-		},
 	}
 
 	for _, tc := range testCases {