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 6 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 `AllowPacketDataList` list that specifies the list of memo that are allowed to send the packet. If this list is empty, then only allow empty memo packet (any other type of `memo` will be denied). If this list is `"*"`, then allow any type of `memo`
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved

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 packet `memo` not allowed by `AllowPacketDataList`
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved

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
AllowPacketDataList []string
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
}

```
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
46 changes: 46 additions & 0 deletions modules/apps/transfer/types/transfer_authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

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

"github.com/cosmos/gogoproto/proto"

Expand Down Expand Up @@ -50,6 +54,15 @@
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer")
}

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

if !isAllowedPacket {
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidRequest, "not allowed packet data type for transfer")
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 Down Expand Up @@ -145,6 +158,39 @@
return false
}

// areAllowedPacketDataKeys returns a boolean indicating if the memo is valid for transfer.
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
func areAllowedPacketDataKeys(ctx sdk.Context, memo string, allowedPacketDataList []string) (bool, error) {
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
// 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 allowedPacketData have only 1 elements and it equal AllowAllPacketDataKeys
// then accept all the packet
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
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
}

gasCostPerIteration := ctx.KVGasConfig().IterNextCostFlat

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

if !slices.Contains(allowedPacketDataList, key) {
return false, fmt.Errorf("not allowed packet data key: %s", key)
}
}
github-advanced-security[bot] marked this conversation as resolved.
Fixed
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
66 changes: 66 additions & 0 deletions modules/apps/transfer/types/transfer_authorization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,72 @@
suite.Require().Nil(res.Updated)
},
},
{
"success: empty allowedPacketDataList and empty memo",
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
func() {
allowedList := []string{}
transferAuthz.Allocations[0].AllowPacketDataList = 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: allowedPacketDataList allow any packet",
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
func() {
allowedList := []string{"*"}
transferAuthz.Allocations[0].AllowPacketDataList = allowedList
msgTransfer.Memo = `{"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"}}}}`

Check failure on line 123 in modules/apps/transfer/types/transfer_authorization_test.go

View workflow job for this annotation

GitHub Actions / lint

string `{"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"}}}}` has 4 occurrences, make it a constant (goconst)
},
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].AllowPacketDataList = allowedList
msgTransfer.Memo = `{"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(res authz.AcceptResponse, err error) {
suite.Require().NoError(err)

suite.Require().True(res.Accept)
suite.Require().True(res.Delete)
suite.Require().Nil(res.Updated)
},
},
{
"error: empty allowedPacketDataList but not empty memo",
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
func() {
allowedList := []string{}
transferAuthz.Allocations[0].AllowPacketDataList = allowedList
msgTransfer.Memo = `{"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(res authz.AcceptResponse, err error) {
suite.Require().Error(err)
},
},
{
"error: memo not allowed",
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
func() {
allowedList := []string{"forward"}
transferAuthz.Allocations[0].AllowPacketDataList = allowedList
msgTransfer.Memo = `{"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(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_list = 5;
GNaD13 marked this conversation as resolved.
Show resolved Hide resolved
}

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