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 24 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
crodriguezvega Dec 7, 2023
a36979e
Update 08-authorizations.md
crodriguezvega Dec 7, 2023
08b49a6
Update 08-authorizations.md
crodriguezvega Dec 7, 2023
a04a68c
Update transfer_authorization_test.go
crodriguezvega Dec 7, 2023
15c674b
Update transfer_authorization.go
crodriguezvega Dec 7, 2023
154a3e0
Update transfer_authorization_test.go
crodriguezvega 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
crodriguezvega 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
crodriguezvega Dec 13, 2023
5673453
change error string
crodriguezvega Dec 13, 2023
b003428
Apply suggestions from code review
crodriguezvega Dec 13, 2023
9d9d5a3
Merge branch 'main' into gnad/add-allowed-packet
crodriguezvega Dec 13, 2023
66bb1e7
Merge branch 'main' into gnad/add-allowed-packet
crodriguezvega 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 `AllowPacketData` 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 `AllowPacketData`

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.
// an empty list prohibits all packet data keys; a list only with "*" permits any packet data key
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
AllowPacketData []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
55 changes: 55 additions & 0 deletions modules/apps/transfer/types/transfer_authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@ package types

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

"github.com/cosmos/gogoproto/proto"
"golang.org/x/exp/maps"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we happy using an experimental feature? @damiannolan @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'd have a preference not to use unstable features in general but considering this wouldn't affect our API in any way if its API changed (I'm reminded of this sdk issue cosmos/cosmos-sdk#18415) we should be gtg.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I would avoid experimental features, especially since this is being used for an error message format


errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"
Expand Down Expand Up @@ -50,6 +54,15 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer")
}

isAllowedPacket, err := isAllowedPacketDataKeys(sdk.UnwrapSDKContext(ctx), msgTransfer.Memo, allocation.AllowPacketData)
if err != nil {
return authz.AcceptResponse{}, errorsmod.Wrapf(ErrInvalidMemo, "error: %v", err)
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
}

if !isAllowedPacket {
return authz.AcceptResponse{}, ErrInvalidMemo
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should add additional information here, "invalid memo" on its own wouldn't help me out too much 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should just return an error rather than true/false and rename to something like validateMemo

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, I do think this makes most sense. false leads to an err, it has no meaningful recovery option so should just omit it altogether.

}

// 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 Down Expand Up @@ -145,6 +158,48 @@ func isAllowedAddress(ctx sdk.Context, receiver string, allowedAddrs []string) b
return false
}

// areAllowedPacketDataKeys returns a boolean indicating if the memo is valid for transfer. If it is not valid and non-nil error is also returned.
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
func isAllowedPacketDataKeys(ctx sdk.Context, memo string, allowedPacketDataList []string) (bool, error) {
// if the allow list is empty, then the memo must be an empty string
if len(allowedPacketDataList) == 0 {
return len(strings.TrimSpace(memo)) == 0, nil
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

// 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 true, nil
}

// unmarshal memo
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
jsonObject := make(map[string]interface{})
err := json.Unmarshal([]byte(memo), &jsonObject)
if err != nil {
return false, err
}

if len(jsonObject) > len(allowedPacketDataList) {
return false, fmt.Errorf("packet type greater than packet allow list")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

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 false, fmt.Errorf("not allowed packet data keys: %v", maps.Keys(jsonObject))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

return true, 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 @@ -17,6 +17,8 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
transferAuthz types.TransferAuthorization
)

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"}}}}`

testCases := []struct {
name string
malleate func()
Expand Down Expand Up @@ -101,6 +103,72 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
suite.Require().Nil(res.Updated)
},
},
{
"success: empty allowPacketDataList and empty memo",
func() {
allowedList := []string{}
transferAuthz.Allocations[0].AllowPacketData = 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: allowPacketData allows any packet",
func() {
allowedList := []string{"*"}
transferAuthz.Allocations[0].AllowPacketData = 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].AllowPacketData = 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 allowPacketData but not empty memo",
func() {
allowedList := []string{}
transferAuthz.Allocations[0].AllowPacketData = allowedList
msgTransfer.Memo = testMemo
},
func(res authz.AcceptResponse, err error) {
suite.Require().Error(err)
},
},
{
"memo not allowed",
func() {
allowedList := []string{"forward"}
transferAuthz.Allocations[0].AllowPacketData = 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.
// an empty list prohibits all packet data keys; a list only with "*" permits any packet data key
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
repeated string allow_packet_data = 5;
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

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