Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add list of allowed packet data keys to Allocation of TransferAuthorization #5280

Merged
merged 45 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
134df14
add keys allow packet
GNaD13 Nov 30, 2023
254b0b5
add allow packet data list
GNaD13 Nov 30, 2023
0543a9b
add func check allowed packet data
GNaD13 Nov 30, 2023
26a55d6
add check allow packet
GNaD13 Nov 30, 2023
cc6abf7
add test
GNaD13 Nov 30, 2023
695152a
update docs
GNaD13 Nov 30, 2023
ddec47c
const the memo
GNaD13 Nov 30, 2023
180e419
Update modules/apps/transfer/types/transfer_authorization.go
GNaD13 Dec 1, 2023
92649be
update nits
GNaD13 Dec 1, 2023
97bf329
Merge branch 'main' into gnad/add-allowed-packet
GNaD13 Dec 3, 2023
8f21fc9
Merge branch 'main' into gnad/add-allowed-packet
GNaD13 Dec 5, 2023
9a90455
key
GNaD13 Dec 5, 2023
7f28ad5
remove keys because the order is indeterminate
GNaD13 Dec 5, 2023
65cc3ad
iterator allowedPacketDataList
GNaD13 Dec 5, 2023
c42ba43
nit
GNaD13 Dec 6, 2023
075b914
move consume gas out of if statement
GNaD13 Dec 6, 2023
54ce770
Merge branch 'main' into gnad/add-allowed-packet
GNaD13 Dec 6, 2023
062f9ee
add keys to error
GNaD13 Dec 6, 2023
3bcbdbe
Merge branch 'main' into gnad/add-allowed-packet
GNaD13 Dec 6, 2023
576a8e6
Merge branch 'main' into gnad/add-allowed-packet
Dec 7, 2023
a36979e
Update 08-authorizations.md
Dec 7, 2023
08b49a6
Update 08-authorizations.md
Dec 7, 2023
a04a68c
Update transfer_authorization_test.go
Dec 7, 2023
15c674b
Update transfer_authorization.go
Dec 7, 2023
154a3e0
Update transfer_authorization_test.go
Dec 7, 2023
6cb2914
Move const outside of function
DimitrisJim Dec 7, 2023
1a1c9e3
Update modules/apps/transfer/types/transfer_authorization.go
DimitrisJim Dec 7, 2023
a39cb4d
Merge branch 'main' into gnad/add-allowed-packet
DimitrisJim Dec 7, 2023
cb6b948
Merge branch 'main' into gnad/add-allowed-packet
Dec 7, 2023
6f3e206
update
GNaD13 Dec 8, 2023
05c03c3
merge
GNaD13 Dec 8, 2023
4d5cd53
proto
GNaD13 Dec 8, 2023
6ef45f9
AllowedPacketData
GNaD13 Dec 8, 2023
4bc9d00
validate memo
GNaD13 Dec 8, 2023
6adf3b8
Merge branch 'main' into gnad/add-allowed-packet
mgl2150 Dec 8, 2023
481489e
nit
GNaD13 Dec 8, 2023
a74aaa2
add allowed packet data to allocation
GNaD13 Dec 11, 2023
7d00786
Update modules/apps/transfer/types/transfer_authorization.go
GNaD13 Dec 12, 2023
3352e26
Merge branch 'main' into gnad/add-allowed-packet
lichdu29 Dec 12, 2023
1a0bb62
Merge branch 'main' into gnad/add-allowed-packet
Dec 13, 2023
5673453
change error string
Dec 13, 2023
b003428
Apply suggestions from code review
Dec 13, 2023
9d9d5a3
Merge branch 'main' into gnad/add-allowed-packet
Dec 13, 2023
66bb1e7
Merge branch 'main' into gnad/add-allowed-packet
Dec 13, 2023
aa3b5a1
Merge branch 'main' into gnad/add-allowed-packet
hoangdv2429 Dec 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/docs/02-apps/01-transfer/08-authorizations.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@ 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.

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:

Expand All @@ -48,6 +51,9 @@ 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
AllowedPacketData []string
}

```
113 changes: 86 additions & 27 deletions modules/apps/transfer/types/authz.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions modules/apps/transfer/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 packet data keys in authz transfer messages
AllowAllPacketDataKeys = "*"

KeyTotalEscrowPrefix = "totalEscrowForDenom"

ParamsKey = "params"
Expand Down
61 changes: 57 additions & 4 deletions modules/apps/transfer/types/transfer_authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package types

import (
"context"
"encoding/json"
"math/big"
"strings"

"github.com/cosmos/gogoproto/proto"

Expand Down Expand Up @@ -50,6 +52,11 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer")
}

err := validateMemo(sdk.UnwrapSDKContext(ctx), msgTransfer.Memo, allocation.AllowedPacketData)
if err != nil {
return authz.AcceptResponse{}, err
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
}

// 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
Expand All @@ -70,10 +77,11 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a
}}, nil
}
a.Allocations[index] = Allocation{
SourcePort: allocation.SourcePort,
SourceChannel: allocation.SourceChannel,
SpendLimit: limitLeft,
AllowList: allocation.AllowList,
SourcePort: allocation.SourcePort,
SourceChannel: allocation.SourceChannel,
SpendLimit: limitLeft,
AllowList: allocation.AllowList,
AllowedPacketData: allocation.AllowedPacketData,
}

return authz.AcceptResponse{Accept: true, Delete: false, Updated: &TransferAuthorization{
Expand Down Expand Up @@ -145,6 +153,51 @@ 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, allowedPacketDataList []string) error {
// if the allow list is empty, then the memo must be an empty string
if len(allowedPacketDataList) == 0 {
if len(strings.TrimSpace(memo)) != 0 {
Comment on lines +159 to +160
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these two conditions be included in a logical AND?

The current code means you cannot include any memo unless there is something in allowPacketDataList. What if I want to add a random string memo "damian's packet" - it would fail.

Copy link
Contributor Author

@GNaD13 GNaD13 Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, according to the design if allowedPacketDataList is empty, we cannot include any memo. We will allow any memo if the allowedPacketDataList is *

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so authz users must set the allowedPacketDataList to use any kind of memo at all. That's good I guess.. definitely explicit 😅

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 packet data keys
if len(allowedPacketDataList) == 1 && allowedPacketDataList[0] == AllowAllPacketDataKeys {
return nil
}

jsonObject := make(map[string]interface{})
err := json.Unmarshal([]byte(memo), &jsonObject)
if err != nil {
return err
}

if len(jsonObject) > len(allowedPacketDataList) {
return errorsmod.Wrapf(ErrInvalidAuthorization, "packet contains more packet data keys than packet allow list has")
}
Comment on lines +179 to +181
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue I see of doing this is that we don't give any information to the user of what keys are the ones that are not allowed? cc @DimitrisJim

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this was also removed in line 195 to probably drop usage of exp. I think we can just create a little function (follow up) that creates a diff of the keys in the two key sets for the dictionaries and just re-use in both places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're fine if I drop this code as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the printing of the keys was dropped!

but no, drop it away 😄 we can just add it afterwards so as to not hold up this issue even longer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the printing of the keys was dropped!

Ay, true! I can put that back and drop this for now then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will tackle this in a follow-up because the keys were shown before using the experimental feature, so I will try getting the keys in a different way.


gasCostPerIteration := ctx.KVGasConfig().IterNextCostFlat

for _, key := range allowedPacketDataList {
ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")

_, ok := jsonObject[key]
if ok {
delete(jsonObject, key)
}
}

if len(jsonObject) != 0 {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
return errorsmod.Wrapf(ErrInvalidAuthorization, "packet data not allowed")
}

return nil
}

// 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
Expand Down
68 changes: 68 additions & 0 deletions modules/apps/transfer/types/transfer_authorization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ 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"}}}}`

func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
var (
msgTransfer types.MsgTransfer
Expand Down Expand Up @@ -101,6 +103,72 @@ 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 = testMemo
},
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{"wasm", "forward"}
transferAuthz.Allocations[0].AllowedPacketData = allowedList
msgTransfer.Memo = testMemo
},
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 = testMemo
},
func(res authz.AcceptResponse, err error) {
suite.Require().Error(err)
},
},
{
"memo not allowed",
func() {
allowedList := []string{"forward"}
transferAuthz.Allocations[0].AllowedPacketData = allowedList
msgTransfer.Memo = testMemo
},
func(res authz.AcceptResponse, err error) {
suite.Require().Error(err)
},
},
{
"test multiple coins does not overspend",
func() {
Expand Down
3 changes: 3 additions & 0 deletions proto/ibc/applications/transfer/v1/authz.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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 packet data keys, an empty list prohibits all packet data keys;
// a list only with "*" permits any packet data key
repeated string allowed_packet_data = 5;
}

// TransferAuthorization allows the grantee to spend up to spend_limit coins from
Expand Down
Loading