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

…osmos#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 <[email protected]> * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <[email protected]> * lint --------- Co-authored-by: srdtrk <[email protected]>
notional-labs · May 10, 2024 · 0a22b7a · 0a22b7a
1 parent 5279120
commit 0a22b7a
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 44 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -60,6 +60,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able.
 * (core/02-client, core/03-connection, apps/27-interchain-accounts) [\#6256](https://github.com/cosmos/ibc-go/pull/6256) Add length checking of array fields in messages.
 * (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/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/docs/docs/02-apps/01-transfer/08-authorizations.md b/docs/docs/02-apps/01-transfer/08-authorizations.md
@@ -22,7 +22,7 @@ It takes:
 
 - an `AllowList` list that specifies the list of addresses that are allowed to receive funds. If this list is empty, then all addresses are allowed to receive funds from the `TransferAuthorization`.
 
-- an `AllowedPacketData` list that specifies the list of memo packet data keys that are allowed to send the packet. If this list is empty, then only an empty memo is allowed (a `memo` field with non-empty content will be denied). If this list includes a single element equal to `"*"`, then any content in `memo` field will be allowed.
+- an `AllowedPacketData` list that specifies the list of memo strings that are allowed to be included in the memo field of the packet. If this list is empty, then only an empty memo is allowed (a `memo` field with non-empty content will be denied). If this list includes a single element equal to `"*"`, then any content in `memo` field will be allowed.
 
 Setting a `TransferAuthorization` is expected to fail if:
 
@@ -51,9 +51,8 @@ type Allocation struct {
   SpendLimit sdk.Coins  
   // allow list of receivers, an empty allow list permits any receiver address
   AllowList []string 
-  // 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
   AllowedPacketData []string 
 }
-
 ```
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)
-		}
-	}
-
-	keys := make([]string, 0, len(jsonObject))
-	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;
 }