From 0a22b7a2fd8839efb6eeed8680a4d23e183503ed Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 10 May 2024 14:33:11 +0200 Subject: [PATCH] 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> --- CHANGELOG.md | 1 + .../02-apps/01-transfer/08-authorizations.md | 7 ++-- modules/apps/transfer/types/authz.pb.go | 4 +- modules/apps/transfer/types/keys.go | 2 +- .../transfer/types/transfer_authorization.go | 37 +++++-------------- .../types/transfer_authorization_test.go | 28 ++++++++++---- .../ibc/applications/transfer/v1/authz.proto | 4 +- 7 files changed, 39 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddd5a2a6210..f464e3a783a 100644 --- 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 index 9dde26c1867..575f3d7337b 100644 --- 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 index 4d015c3c58a..01928af5ac8 100644 --- a/modules/apps/transfer/types/authz.pb.go +++ b/modules/apps/transfer/types/authz.pb.go @@ -36,8 +36,8 @@ type Allocation struct { SpendLimit github_com_cosmos_cosmos_sdk_types.Coins `protobuf:"bytes,3,rep,name=spend_limit,json=spendLimit,proto3,castrepeated=github.com/cosmos/cosmos-sdk/types.Coins" json:"spend_limit"` // allow list of receivers, an empty allow list permits any receiver address AllowList []string `protobuf:"bytes,4,rep,name=allow_list,json=allowList,proto3" json:"allow_list,omitempty"` - // 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 `protobuf:"bytes,5,rep,name=allowed_packet_data,json=allowedPacketData,proto3" json:"allowed_packet_data,omitempty"` } diff --git a/modules/apps/transfer/types/keys.go b/modules/apps/transfer/types/keys.go index 0b13912cc88..b94caa797dc 100644 --- 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 index 0683166cc8b..b5e4bbe23bc 100644 --- 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 index c5d883e5198..8ef1b36bdfc 100644 --- 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 index e7561b0708b..5c4b71d3479 100644 --- 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; }