fix(nexus)!: validate lockable asset denom (#2198)

x/nexus/keeper/lockable_asset.go

@@ -22,7 +22,7 @@ func (k Keeper) NewLockableAsset(ctx sdk.Context, ibc types.IBCKeeper, bank type
 
 // lockableAsset provides functionality to lock and release coins
 type lockableAsset struct {
-	sdk.Coin
+	asset    sdk.Coin
 	coinType types.CoinType
 	nexus    types.Nexus
 	ibc      types.IBCKeeper
@@ -31,26 +31,13 @@ type lockableAsset struct {
 
 // newLockableAsset creates a coin struct, assign a coin type and normalize the denom if it's a ICS20 token
 func newLockableAsset(ctx sdk.Context, nexus types.Nexus, ibc types.IBCKeeper, bank types.BankKeeper, coin sdk.Coin) (lockableAsset, error) {
-	denom := coin.GetDenom()
-
-	coinType, err := getCoinType(ctx, nexus, ibc, denom)
+	asset, coinType, err := toAssetAndType(ctx, nexus, ibc, coin)
 	if err != nil {
 		return lockableAsset{}, err
 	}
 
-	// If coin type is ICS20, we need to normalize it to convert from 'ibc/{hash}'
-	// to native asset denom so that nexus could recognize it
-	if coinType == types.ICS20 && isIBCDenom(denom) {
-		denomTrace, err := ibc.ParseIBCDenom(ctx, denom)
-		if err != nil {
-			return lockableAsset{}, err
-		}
-
-		coin = sdk.NewCoin(denomTrace.GetBaseDenom(), coin.Amount)
-	}
-
 	c := lockableAsset{
-		Coin:     coin,
+		asset:    asset,
 		coinType: coinType,
 		nexus:    nexus,
 		ibc:      ibc,
@@ -66,7 +53,7 @@ func newLockableAsset(ctx sdk.Context, nexus types.Nexus, ibc types.IBCKeeper, b
 
 // GetAsset returns a sdk.Coin using the nexus registered asset as the denom
 func (c lockableAsset) GetAsset() sdk.Coin {
-	return c.Coin
+	return c.asset
 }
 
 // GetCoin returns a sdk.Coin with the actual denom used by x/bank (e.g. ICS20 coins)
@@ -106,36 +93,42 @@ func (c lockableAsset) UnlockTo(ctx sdk.Context, toAddr sdk.AccAddress) error {
 func (c lockableAsset) getCoin(ctx sdk.Context) (sdk.Coin, error) {
 	switch c.coinType {
 	case types.ICS20:
-		return c.toICS20(ctx)
+		return toICS20(ctx, c.nexus, c.ibc, c.asset)
 	case types.Native, types.External:
-		return c.Coin, nil
+		return c.asset, nil
 	default:
 		return sdk.Coin{}, fmt.Errorf("unrecognized coin type %d", c.coinType)
 	}
 }
 
-func (c lockableAsset) toICS20(ctx sdk.Context) (sdk.Coin, error) {
-	if c.coinType != types.ICS20 {
-		return sdk.Coin{}, fmt.Errorf("%s is not ICS20 token", c.GetDenom())
-	}
-
+func getIBCPath(ctx sdk.Context, nexus types.Nexus, ibc types.IBCKeeper, asset string) (string, error) {
 	// check if the asset registered with a path
-	chain, ok := c.nexus.GetChainByNativeAsset(ctx, c.GetDenom())
+	chain, ok := nexus.GetChainByNativeAsset(ctx, asset)
 	if !ok {
-		return sdk.Coin{}, fmt.Errorf("asset %s is not linked to a cosmos chain", c.GetDenom())
+		return "", fmt.Errorf("asset %s is not linked to a cosmos chain", asset)
 	}
 
-	path, ok := c.ibc.GetIBCPath(ctx, chain.Name)
+	path, ok := ibc.GetIBCPath(ctx, chain.Name)
 	if !ok {
-		return sdk.Coin{}, fmt.Errorf("path not found for chain %s", chain.Name)
+		return "", fmt.Errorf("path not found for chain %s", chain.Name)
+	}
+
+	return path, nil
+}
+
+// toICS20 converts a registered asset originating from an external cosmos chain to an ICS20 coin
+func toICS20(ctx sdk.Context, nexus types.Nexus, ibc types.IBCKeeper, coin sdk.Coin) (sdk.Coin, error) {
+	path, err := getIBCPath(ctx, nexus, ibc, coin.GetDenom())
+	if err != nil {
+		return sdk.Coin{}, err
 	}
 
 	trace := ibctypes.DenomTrace{
 		Path:      path,
-		BaseDenom: c.GetDenom(),
+		BaseDenom: coin.GetDenom(),
 	}
 
-	return sdk.NewCoin(trace.IBCDenom(), c.Amount), nil
+	return sdk.NewCoin(trace.IBCDenom(), coin.Amount), nil
 }
 
 func lock(ctx sdk.Context, bank types.BankKeeper, fromAddr sdk.AccAddress, coin sdk.Coin) error {
@@ -174,38 +167,53 @@ func mint(ctx sdk.Context, bank types.BankKeeper, toAddr sdk.AccAddress, coin sd
 	return nil
 }
 
-func getCoinType(ctx sdk.Context, nexus types.Nexus, ibc types.IBCKeeper, denom string) (types.CoinType, error) {
+// toAssetAndType normalizes the given denom to the corresponding registered asset and returns the coin type
+func toAssetAndType(ctx sdk.Context, nexus types.Nexus, ibc types.IBCKeeper, coin sdk.Coin) (sdk.Coin, types.CoinType, error) {
+	denom := coin.GetDenom()
+
 	switch {
-	// check if the format of token denomination is 'ibc/{hash}'
-	case isIBCDenom(denom):
-		return types.ICS20, nil
+	// check if the format of token denomination is 'ibc/{hash}' and it's a registered asset
+	case isICS20Denom(denom):
+		{
+			denomTrace, err := ibc.ParseIBCDenom(ctx, denom)
+			if err != nil {
+				return sdk.Coin{}, types.Unrecognized, err
+			}
+
+			path, err := getIBCPath(ctx, nexus, ibc, denomTrace.GetBaseDenom())
+			if err != nil {
+				return sdk.Coin{}, types.Unrecognized, err
+			}
+			if path != denomTrace.GetPath() {
+				return sdk.Coin{}, types.Unrecognized, fmt.Errorf("expected ics20 coin IBC path %s, got %s", path, denomTrace.GetPath())
+			}
+
+			asset := sdk.NewCoin(denomTrace.GetBaseDenom(), coin.Amount)
+
+			return asset, types.ICS20, nil
+		}
 	// check if the denom is the registered asset name for an ICS20 coin from a cosmos chain
 	case isFromExternalCosmosChain(ctx, nexus, ibc, denom):
-		return types.ICS20, nil
+		return coin, types.ICS20, nil
 	case isNativeAssetOnAxelarnet(ctx, nexus, denom):
-		return types.Native, nil
+		return coin, types.Native, nil
 	case nexus.IsAssetRegistered(ctx, axelarnet.Axelarnet, denom):
-		return types.External, nil
+		return coin, types.External, nil
 	default:
-		return types.Unrecognized, fmt.Errorf("unrecognized coin %s", denom)
+		return sdk.Coin{}, types.Unrecognized, fmt.Errorf("unrecognized coin %s", denom)
 	}
 }
 
 // isFromExternalCosmosChain returns true if the denom is a nexus-registered
 // asset name for an ICS20 coin originating from a cosmos chain
 func isFromExternalCosmosChain(ctx sdk.Context, nexus types.Nexus, ibc types.IBCKeeper, denom string) bool {
-	chain, ok := nexus.GetChainByNativeAsset(ctx, denom)
-	if !ok {
-		return false
-	}
-
-	_, ok = ibc.GetIBCPath(ctx, chain.Name)
+	_, err := getIBCPath(ctx, nexus, ibc, denom)
 
-	return ok
+	return err == nil
 }
 
-// isIBCDenom validates that the given denomination is a valid ICS token representation (ibc/{hash})
-func isIBCDenom(denom string) bool {
+// isICS20Denom validates that the given denomination is a valid ICS token representation (ibc/{hash})
+func isICS20Denom(denom string) bool {
 	if err := sdk.ValidateDenom(denom); err != nil {
 		return false
 	}

x/nexus/keeper/lockable_asset_test.go

@@ -128,6 +128,23 @@ func TestLockableAsset(t *testing.T) {
 			}).
 			Run(t)
 
+		givenKeeper.
+			When2(whenCoinIsICS20).
+			When("registered ibc path differs from ics20 trace", func() {
+				registeredPath := testutils.RandomIBCPath()
+				ibc.GetIBCPathFunc = func(ctx sdk.Context, chain exported.ChainName) (string, bool) {
+					if chain == axelarnet.Axelarnet.Name {
+						return registeredPath, true
+					}
+					return "", false
+				}
+			}).
+			Then("should fail to create a new lockable asset", func(t *testing.T) {
+				_, err := newLockableAsset(ctx, nexus, ibc, bank, coin)
+				assert.ErrorContains(t, err, "expected ics20 coin IBC path")
+			}).
+			Run(t)
+
 		givenKeeper.
 			When2(whenCoinIsICS20FromExternalCosmosChain).
 			Then("should create a new lockable asset of type ICS20", func(t *testing.T) {