imp: allow memo strings instead of keys for transfer authorizations (…

…backport #6268) (#6289) * imp: allow memo strings instead of keys for transfer authorizations (#6268) * imp: allow memo strings instead of keys for transfer authorizations * add changelog * handle error from compact * return error * improve test * not enforce that memo strings of allowed packet data must be JSON-encoded strings * use slices contains to check if memo is allowed * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * lint --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> (cherry picked from commit 0a22b7a) # Conflicts: # CHANGELOG.md # docs/docs/02-apps/01-transfer/08-authorizations.md # modules/apps/transfer/types/transfer_authorization.go * fix conflicts * can't use slices import * Revert "can't use slices import" This reverts commit e9d9f69. * remove docs --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
cosmos · May 13, 2024 · 5470cd2 · 5470cd2
1 parent 725fe10
commit 5470cd2
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other.
 * (apps/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels.
+* (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization.
 
 ### Features
 

diff --git a/modules/apps/transfer/types/authz.pb.go b/modules/apps/transfer/types/authz.pb.go
diff --git a/modules/apps/transfer/types/keys.go b/modules/apps/transfer/types/keys.go
@@ -30,7 +30,7 @@ const (
 	// DenomPrefix is the prefix used for internal SDK coin representation.
 	DenomPrefix = "ibc"
 
-	// AllowAllPacketDataKeys holds the string key that allows all packet data keys in authz transfer messages
+	// AllowAllPacketDataKeys holds the string key that allows all memo strings in authz transfer messages
 	AllowAllPacketDataKeys = "*"
 
 	KeyTotalEscrowPrefix = "totalEscrowForDenom"

diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go
@@ -2,9 +2,8 @@ package types
 
 import (
 	"context"
-	"encoding/json"
 	"math/big"
-	"sort"
+	"slices"
 	"strings"
 
 	"github.com/cosmos/gogoproto/proto"
@@ -155,9 +154,9 @@ func isAllowedAddress(ctx sdk.Context, receiver string, allowedAddrs []string) b
 }
 
 // validateMemo returns a nil error indicating if the memo is valid for transfer.
-func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string) error {
+func validateMemo(ctx sdk.Context, memo string, allowedMemos []string) error {
 	// if the allow list is empty, then the memo must be an empty string
-	if len(allowedPacketDataList) == 0 {
+	if len(allowedMemos) == 0 {
 		if len(strings.TrimSpace(memo)) != 0 {
 			return errorsmod.Wrapf(ErrInvalidAuthorization, "memo must be empty because allowed packet data in allocation is empty")
 		}
@@ -166,36 +165,20 @@ func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string)
 	}
 
 	// if allowedPacketDataList has only 1 element and it equals AllowAllPacketDataKeys
-	// then accept all the packet data keys
-	if len(allowedPacketDataList) == 1 && allowedPacketDataList[0] == AllowAllPacketDataKeys {
+	// then accept all the memo strings
+	if len(allowedMemos) == 1 && allowedMemos[0] == AllowAllPacketDataKeys {
 		return nil
 	}
 
-	jsonObject := make(map[string]interface{})
-	err := json.Unmarshal([]byte(memo), &jsonObject)
-	if err != nil {
-		return err
-	}
-
 	gasCostPerIteration := ctx.KVGasConfig().IterNextCostFlat
-
-	for _, key := range allowedPacketDataList {
+	isMemoAllowed := slices.ContainsFunc(allowedMemos, func(allowedMemo string) bool {
 		ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")
 
-		_, ok := jsonObject[key]
-		if ok {
-			delete(jsonObject, key)
-		}
-	}
-
-	var keys []string
-	for k := range jsonObject {
-		keys = append(keys, k)
-	}
-	sort.Strings(keys)
+		return strings.TrimSpace(memo) == strings.TrimSpace(allowedMemo)
+	})
 
-	if len(jsonObject) != 0 {
-		return errorsmod.Wrapf(ErrInvalidAuthorization, "not allowed packet data keys: %s", keys)
+	if !isMemoAllowed {
+		return errorsmod.Wrapf(ErrInvalidAuthorization, "not allowed memo: %s", memo)
 	}
 
 	return nil

diff --git a/modules/apps/transfer/types/transfer_authorization_test.go b/modules/apps/transfer/types/transfer_authorization_test.go
@@ -1,6 +1,8 @@
 package types_test
 
 import (
+	"fmt"
+
 	sdkmath "cosmossdk.io/math"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -11,7 +13,10 @@ import (
 	"github.com/cosmos/ibc-go/v8/testing/mock"
 )
 
-const testMemo = `{"wasm":{"contract":"osmo1c3ljch9dfw5kf52nfwpxd2zmj2ese7agnx0p9tenkrryasrle5sqf3ftpg","msg":{"osmosis_swap":{"output_denom":"uosmo","slippage":{"twap":{"slippage_percentage":"20","window_seconds":10}},"receiver":"feeabs/feeabs1efd63aw40lxf3n4mhf7dzhjkr453axurwrhrrw","on_failed_delivery":"do_nothing"}}}}`
+const (
+	testMemo1 = `{"wasm":{"contract":"osmo1c3ljch9dfw5kf52nfwpxd2zmj2ese7agnx0p9tenkrryasrle5sqf3ftpg","msg":{"osmosis_swap":{"output_denom":"uosmo","slippage":{"twap":{"slippage_percentage":"20","window_seconds":10}},"receiver":"feeabs/feeabs1efd63aw40lxf3n4mhf7dzhjkr453axurwrhrrw","on_failed_delivery":"do_nothing"}}}}`
+	testMemo2 = `{"forward":{"channel":"channel-11","port":"transfer","receiver":"stars1twfv52yxcyykx2lcvgl42svw46hsm5dd4ww6xy","retries":2,"timeout":1712146014542131200}}`
+)
 
 func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
 	var (
@@ -122,7 +127,7 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
 			func() {
 				allowedList := []string{"*"}
 				transferAuthz.Allocations[0].AllowedPacketData = allowedList
-				msgTransfer.Memo = testMemo
+				msgTransfer.Memo = testMemo1
 			},
 			func(res authz.AcceptResponse, err error) {
 				suite.Require().NoError(err)
@@ -135,9 +140,9 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
 		{
 			"success: transfer memo allowed",
 			func() {
-				allowedList := []string{"wasm", "forward"}
+				allowedList := []string{testMemo1, testMemo2}
 				transferAuthz.Allocations[0].AllowedPacketData = allowedList
-				msgTransfer.Memo = testMemo
+				msgTransfer.Memo = testMemo1
 			},
 			func(res authz.AcceptResponse, err error) {
 				suite.Require().NoError(err)
@@ -152,7 +157,7 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
 			func() {
 				allowedList := []string{}
 				transferAuthz.Allocations[0].AllowedPacketData = allowedList
-				msgTransfer.Memo = testMemo
+				msgTransfer.Memo = testMemo1
 			},
 			func(res authz.AcceptResponse, err error) {
 				suite.Require().Error(err)
@@ -161,13 +166,13 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
 		{
 			"memo not allowed",
 			func() {
-				allowedList := []string{"forward"}
+				allowedList := []string{testMemo1}
 				transferAuthz.Allocations[0].AllowedPacketData = allowedList
-				msgTransfer.Memo = testMemo
+				msgTransfer.Memo = testMemo2
 			},
 			func(res authz.AcceptResponse, err error) {
 				suite.Require().Error(err)
-				suite.Require().ErrorContains(err, "not allowed packet data keys: [wasm]")
+				suite.Require().ErrorContains(err, fmt.Sprintf("not allowed memo: %s", testMemo2))
 			},
 		},
 		{
@@ -310,6 +315,13 @@ func (suite *TypesTestSuite) TestTransferAuthorizationValidateBasic() {
 			},
 			true,
 		},
+		{
+			"success: wildcard allowed packet data",
+			func() {
+				transferAuthz.Allocations[0].AllowedPacketData = []string{"*"}
+			},
+			true,
+		},
 		{
 			"empty allocations",
 			func() {

diff --git a/proto/ibc/applications/transfer/v1/authz.proto b/proto/ibc/applications/transfer/v1/authz.proto
@@ -19,8 +19,8 @@ message Allocation {
       [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];
   // allow list of receivers, an empty allow list permits any receiver address
   repeated string allow_list = 4;
-  // allow list of packet data keys, an empty list prohibits all packet data keys;
-  // a list only with "*" permits any packet data key
+  // allow list of memo strings, an empty list prohibits all memo strings;
+  // a list only with "*" permits any memo string
   repeated string allowed_packet_data = 5;
 }