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

…backport #6268) (#6288) * 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 <[email protected]> * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: srdtrk <[email protected]> * lint --------- Co-authored-by: srdtrk <[email protected]> (cherry picked from commit 0a22b7a) # Conflicts: # docs/docs/02-apps/01-transfer/08-authorizations.md # modules/apps/transfer/types/authz.pb.go # modules/apps/transfer/types/keys.go # modules/apps/transfer/types/transfer_authorization.go # modules/apps/transfer/types/transfer_authorization_test.go # proto/ibc/applications/transfer/v1/authz.proto * fix conflicts * don't use slices * use sdkerrors * remove docs --------- Co-authored-by: Carlos Rodriguez <[email protected]>
cosmos · May 13, 2024 · a98ebca · a98ebca
1 parent 1c05fa7
commit a98ebca
Show file tree

Hide file tree

Showing 6 changed files with 210 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -51,6 +51,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (apps/27-interchain-accounts) [\#6147](https://github.com/cosmos/ibc-go/pull/6147) Emit an event signalling that the host submodule is disabled.
 * (testing) [\#6180](https://github.com/cosmos/ibc-go/pull/6180) Add version to tm abci headers in ibctesting.
 * (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/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,6 +30,9 @@ const (
 	// DenomPrefix is the prefix used for internal SDK coin representation.
 	DenomPrefix = "ibc"
 
+	// 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,6 +2,7 @@ package types
 
 import (
 	"math/big"
+	"strings"
 
 	sdkmath "cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -45,6 +46,11 @@ func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.Accep
 			return authz.AcceptResponse{}, sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "not allowed receiver address for transfer")
 		}
 
+		err := validateMemo(sdk.UnwrapSDKContext(ctx), msgTransfer.Memo, allocation.AllowedPacketData)
+		if err != nil {
+			return authz.AcceptResponse{}, err
+		}
+
 		// If the spend limit is set to the MaxUint256 sentinel value, do not subtract the amount from the spend limit.
 		if allocation.SpendLimit.AmountOf(msgTransfer.Token.Denom).Equal(UnboundedSpendLimit()) {
 			return authz.AcceptResponse{Accept: true, Delete: false, Updated: nil}, nil
@@ -140,6 +146,35 @@ func isAllowedAddress(ctx sdk.Context, receiver string, allowedAddrs []string) b
 	return false
 }
 
+// validateMemo returns a nil error indicating if the memo is valid for transfer.
+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(allowedMemos) == 0 {
+		if len(strings.TrimSpace(memo)) != 0 {
+			return sdkerrors.Wrapf(ErrInvalidAuthorization, "memo must be empty because allowed packet data in allocation is empty")
+		}
+
+		return nil
+	}
+
+	// if allowedPacketDataList has only 1 element and it equals AllowAllPacketDataKeys
+	// then accept all the memo strings
+	if len(allowedMemos) == 1 && allowedMemos[0] == AllowAllPacketDataKeys {
+		return nil
+	}
+
+	gasCostPerIteration := ctx.KVGasConfig().IterNextCostFlat
+	for _, allowedMemo := range allowedMemos {
+		ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")
+
+		if strings.TrimSpace(memo) == strings.TrimSpace(allowedMemo) {
+			return nil
+		}
+	}
+
+	return sdkerrors.Wrapf(ErrInvalidAuthorization, "not allowed memo: %s", memo)
+}
+
 // UnboundedSpendLimit returns the sentinel value that can be used
 // as the amount for a denomination's spend limit for which spend limit updating
 // should be disabled. Please note that using this sentinel value means that a grantee

diff --git a/modules/apps/transfer/types/transfer_authorization_test.go b/modules/apps/transfer/types/transfer_authorization_test.go
@@ -1,13 +1,20 @@
 package types_test
 
 import (
+	"fmt"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/authz"
 	"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 	"github.com/cosmos/ibc-go/v7/testing/mock"
 )
 
+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 (
 		msgTransfer   types.MsgTransfer
@@ -98,6 +105,73 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
 				suite.Require().Nil(res.Updated)
 			},
 		},
+		{
+			"success: empty AllowedPacketData and empty memo",
+			func() {
+				allowedList := []string{}
+				transferAuthz.Allocations[0].AllowedPacketData = allowedList
+			},
+			func(res authz.AcceptResponse, err error) {
+				suite.Require().NoError(err)
+
+				suite.Require().True(res.Accept)
+				suite.Require().True(res.Delete)
+				suite.Require().Nil(res.Updated)
+			},
+		},
+		{
+			"success: AllowedPacketData allows any packet",
+			func() {
+				allowedList := []string{"*"}
+				transferAuthz.Allocations[0].AllowedPacketData = allowedList
+				msgTransfer.Memo = testMemo1
+			},
+			func(res authz.AcceptResponse, err error) {
+				suite.Require().NoError(err)
+
+				suite.Require().True(res.Accept)
+				suite.Require().True(res.Delete)
+				suite.Require().Nil(res.Updated)
+			},
+		},
+		{
+			"success: transfer memo allowed",
+			func() {
+				allowedList := []string{testMemo1, testMemo2}
+				transferAuthz.Allocations[0].AllowedPacketData = allowedList
+				msgTransfer.Memo = testMemo1
+			},
+			func(res authz.AcceptResponse, err error) {
+				suite.Require().NoError(err)
+
+				suite.Require().True(res.Accept)
+				suite.Require().True(res.Delete)
+				suite.Require().Nil(res.Updated)
+			},
+		},
+		{
+			"empty AllowedPacketData but not empty memo",
+			func() {
+				allowedList := []string{}
+				transferAuthz.Allocations[0].AllowedPacketData = allowedList
+				msgTransfer.Memo = testMemo1
+			},
+			func(res authz.AcceptResponse, err error) {
+				suite.Require().Error(err)
+			},
+		},
+		{
+			"memo not allowed",
+			func() {
+				allowedList := []string{testMemo1}
+				transferAuthz.Allocations[0].AllowedPacketData = allowedList
+				msgTransfer.Memo = testMemo2
+			},
+			func(res authz.AcceptResponse, err error) {
+				suite.Require().Error(err)
+				suite.Require().ErrorContains(err, fmt.Sprintf("not allowed memo: %s", testMemo2))
+			},
+		},
 		{
 			"test multiple coins does not overspend",
 			func() {
@@ -236,6 +310,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,6 +19,9 @@ 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 memo strings, an empty list prohibits all memo strings;
+  // a list only with "*" permits any memo string
+  repeated string allowed_packet_data = 5;
 }
 
 // TransferAuthorization allows the grantee to spend up to spend_limit coins from