From e1b16cc73088b8053517d71baf5c995a17c2fabc 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> (cherry picked from commit 0a22b7a2fd8839efb6eeed8680a4d23e183503ed) # 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 --- CHANGELOG.md | 1 + .../02-apps/01-transfer/08-authorizations.md | 58 ++++++++++++ modules/apps/transfer/types/authz.pb.go | 6 ++ modules/apps/transfer/types/keys.go | 6 ++ .../transfer/types/transfer_authorization.go | 41 +++++++++ .../types/transfer_authorization_test.go | 92 +++++++++++++++++++ .../ibc/applications/transfer/v1/authz.proto | 6 ++ 7 files changed, 210 insertions(+) create mode 100644 docs/docs/02-apps/01-transfer/08-authorizations.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ca36a43c73..79a2e4c6497 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,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/docs/docs/02-apps/01-transfer/08-authorizations.md b/docs/docs/02-apps/01-transfer/08-authorizations.md new file mode 100644 index 00000000000..575f3d7337b --- /dev/null +++ b/docs/docs/02-apps/01-transfer/08-authorizations.md @@ -0,0 +1,58 @@ +--- +title: Authorizations +sidebar_label: Authorizations +sidebar_position: 8 +slug: /apps/transfer/authorizations +--- +# `TransferAuthorization` + +`TransferAuthorization` implements the `Authorization` interface for `ibc.applications.transfer.v1.MsgTransfer`. It allows a granter to grant a grantee the privilege to submit `MsgTransfer` on its behalf. Please see the [Cosmos SDK docs](https://docs.cosmos.network/v0.47/modules/authz) for more details on granting privileges via the `x/authz` module. + +More specifically, the granter allows the grantee to transfer funds that belong to the granter over a specified channel. + +For the specified channel, the granter must be able to specify a spend limit of a specific denomination they wish to allow the grantee to be able to transfer. + +The granter may be able to specify the list of addresses that they allow to receive funds. If empty, then all addresses are allowed. + +It takes: + +- a `SourcePort` and a `SourceChannel` which together comprise the unique transfer channel identifier over which authorized funds can be transferred. + +- a `SpendLimit` that specifies the maximum amount of tokens the grantee can transfer. The `SpendLimit` is updated as the tokens are transferred, unless the sentinel value of the maximum value for a 256-bit unsigned integer (i.e. 2^256 - 1) is used for the amount, in which case the `SpendLimit` will not be updated (please be aware that using this sentinel value will grant the grantee the privilege to transfer **all** the tokens of a given denomination available at the granter's account). The helper function `UnboundedSpendLimit` in the `types` package of the `transfer` module provides the sentinel value that can be used. This `SpendLimit` may also be updated to increase or decrease the limit as the granter wishes. + +- 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 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: + +- the spend limit is nil +- the denomination of the spend limit is an invalid coin type +- the source port ID is invalid +- the source channel ID is invalid +- there are duplicate entries in the `AllowList` +- the `memo` field is not allowed by `AllowedPacketData` + +Below is the `TransferAuthorization` message: + +```go +func NewTransferAuthorization(allocations ...Allocation) *TransferAuthorization { + return &TransferAuthorization{ + Allocations: allocations, + } +} + +type Allocation struct { + // the port on which the packet will be sent + SourcePort string + // the channel by which the packet will be sent + SourceChannel string + // spend limitation on the channel + SpendLimit sdk.Coins + // allow list of receivers, an empty allow list permits any receiver address + AllowList []string + // 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 95f24eca332..3027d603392 100644 --- a/modules/apps/transfer/types/authz.pb.go +++ b/modules/apps/transfer/types/authz.pb.go @@ -36,6 +36,12 @@ 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"` +<<<<<<< HEAD +======= + // 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"` +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) } func (m *Allocation) Reset() { *m = Allocation{} } diff --git a/modules/apps/transfer/types/keys.go b/modules/apps/transfer/types/keys.go index ec4c76cf596..ab5bee8357a 100644 --- a/modules/apps/transfer/types/keys.go +++ b/modules/apps/transfer/types/keys.go @@ -30,6 +30,12 @@ const ( // DenomPrefix is the prefix used for internal SDK coin representation. DenomPrefix = "ibc" +<<<<<<< HEAD +======= + // AllowAllPacketDataKeys holds the string key that allows all memo strings in authz transfer messages + AllowAllPacketDataKeys = "*" + +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) KeyTotalEscrowPrefix = "totalEscrowForDenom" ) diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go index f8a7ef18be2..904667cee50 100644 --- a/modules/apps/transfer/types/transfer_authorization.go +++ b/modules/apps/transfer/types/transfer_authorization.go @@ -1,7 +1,14 @@ package types import ( +<<<<<<< HEAD "math/big" +======= + "context" + "math/big" + "slices" + "strings" +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) sdkmath "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" @@ -140,6 +147,40 @@ func isAllowedAddress(ctx sdk.Context, receiver string, allowedAddrs []string) b return false } +<<<<<<< HEAD +======= +// 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 errorsmod.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 + isMemoAllowed := slices.ContainsFunc(allowedMemos, func(allowedMemo string) bool { + ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization") + + return strings.TrimSpace(memo) == strings.TrimSpace(allowedMemo) + }) + + if !isMemoAllowed { + return errorsmod.Wrapf(ErrInvalidAuthorization, "not allowed memo: %s", memo) + } + + return nil +} + +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) // 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 index a549bce954e..08d6b02314c 100644 --- a/modules/apps/transfer/types/transfer_authorization_test.go +++ b/modules/apps/transfer/types/transfer_authorization_test.go @@ -1,6 +1,13 @@ package types_test import ( +<<<<<<< HEAD +======= + "fmt" + + sdkmath "cosmossdk.io/math" + +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/authz" "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" @@ -8,6 +15,14 @@ import ( "github.com/cosmos/ibc-go/v7/testing/mock" ) +<<<<<<< HEAD +======= +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}}` +) + +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { var ( msgTransfer types.MsgTransfer @@ -99,6 +114,76 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { }, }, { +<<<<<<< HEAD +======= + "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)) + }, + }, + { +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) "test multiple coins does not overspend", func() { transferAuthz.Allocations[0].SpendLimit = transferAuthz.Allocations[0].SpendLimit.Add( @@ -236,6 +321,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 8b27ac9cf7d..e8b1820dcfb 100644 --- a/proto/ibc/applications/transfer/v1/authz.proto +++ b/proto/ibc/applications/transfer/v1/authz.proto @@ -19,6 +19,12 @@ 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; +<<<<<<< HEAD +======= + // 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; +>>>>>>> 0a22b7a2 (imp: allow memo strings instead of keys for transfer authorizations (#6268)) } // TransferAuthorization allows the grantee to spend up to spend_limit coins from