Skip to content

Commit

Permalink
imp: represent unlimited approvals with MaxUint256 value (#3454)
Browse files Browse the repository at this point in the history
* imp: represent unlimited approvals with a nil value

* CHANGELOG

* Update CHANGELOG.md

* fix: updated unlimited spending limit to be represented with the MaxInt64

* Update modules/apps/transfer/types/transfer_authorization.go

Co-authored-by: Federico Kunze Küllmer <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Federico Kunze Küllmer <[email protected]>

* fix: lint

* Update modules/apps/transfer/types/transfer_authorization.go

* fix: update failing test and add test case suggested in review

* fix: moved isAllowedAddress check before coin loop

* fix: use maxUint256 case so it aligns with what's being passed from the EVM extension

* refactor transfer authz to remove coins iteration in favour of explicit amount checking

* make format

* Update modules/apps/transfer/types/transfer_authorization.go

Co-authored-by: Damian Nolan <[email protected]>

* fix: add the Authorization to Updated.

* moving allowlist check to before spend limit logic

* Apply suggestions from code review

* fix: add comment to spend limit check

* review feedback

* Update modules/apps/transfer/types/transfer_authorization.go

Co-authored-by: Damian Nolan <[email protected]>

* Update docs/apps/transfer/authorizations.md

* fix: changed to new sentinel value name

---------

Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
Co-authored-by: Damian Nolan <[email protected]>
  • Loading branch information
4 people authored May 15, 2023
1 parent 3882830 commit 7e6eb4c
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 10 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Improvements

* (tests) [\#3138](https://github.com/cosmos/ibc-go/pull/3138) Support benchmarks and fuzz tests through `testing.TB`.
* (apps/transfer) [\#3454](https://github.com/cosmos/ibc-go/pull/3454) Support transfer authorization unlimited spending when the max `uint256` value is provided as limit.

### Features

Expand Down
4 changes: 2 additions & 2 deletions docs/apps/transfer/authorizations.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# `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.
`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.

Expand All @@ -12,7 +12,7 @@ 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 spend. The `SpendLimit` is updated as the tokens are spent. This `SpendLimit` may also be updated to increase or decrease the limit as the granter wishes.
- a `SpendLimit` that specifies the maximum amount of tokens the grantee can transfer. The `SpendLimit` is updated as the tokens are transfered, 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`.

Expand Down
2 changes: 1 addition & 1 deletion e2e/tests/core/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ func (s *ClientTestSuite) extractChainPrivateKeys(ctx context.Context, chain *co
}

// createMaliciousTMHeader creates a header with the provided trusted height with an invalid app hash.
func createMaliciousTMHeader(chainID string, blockHeight int64, trustedHeight clienttypes.Height, timestamp time.Time, tmValSet, tmTrustedVals *tmtypes.ValidatorSet, signers []tmtypes.PrivValidator, oldHeader testsuite.Header ) (*ibctm.Header, error) {
func createMaliciousTMHeader(chainID string, blockHeight int64, trustedHeight clienttypes.Height, timestamp time.Time, tmValSet, tmTrustedVals *tmtypes.ValidatorSet, signers []tmtypes.PrivValidator, oldHeader testsuite.Header) (*ibctm.Header, error) {
tmHeader := tmtypes.Header{
Version: tmprotoversion.Consensus{Block: tmversion.BlockProtocol, App: 2},
ChainID: chainID,
Expand Down
1 change: 0 additions & 1 deletion e2e/testsuite/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,6 @@ func (s *E2ETestSuite) QueryTotalEscrowForDenom(ctx context.Context, chain ibc.C
res, err := queryClient.TotalEscrowForDenom(ctx, &transfertypes.QueryTotalEscrowForDenomRequest{
Denom: denom,
})

if err != nil {
return sdk.Coin{}, err
}
Expand Down
30 changes: 25 additions & 5 deletions modules/apps/transfer/types/transfer_authorization.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
package types

import (
"math/big"

errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/authz"

ibcerrors "github.com/cosmos/ibc-go/v7/internal/errors"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
)

var _ authz.Authorization = (*TransferAuthorization)(nil)

// maxUint256 is the maximum value for a 256 bit unsigned integer.
var maxUint256 = new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 256), big.NewInt(1))

// NewTransferAuthorization creates a new TransferAuthorization object.
func NewTransferAuthorization(allocations ...Allocation) *TransferAuthorization {
return &TransferAuthorization{
Expand All @@ -36,15 +41,20 @@ func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.Accep
continue
}

if !isAllowedAddress(ctx, msgTransfer.Receiver, allocation.AllowList) {
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer")
}

// 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: &a}, nil
}

limitLeft, isNegative := allocation.SpendLimit.SafeSub(msgTransfer.Token)
if isNegative {
return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrInsufficientFunds, "requested amount is more than spend limit")
}

if !isAllowedAddress(ctx, msgTransfer.Receiver, allocation.AllowList) {
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed address for transfer")
}

if limitLeft.IsZero() {
a.Allocations = append(a.Allocations[:index], a.Allocations[index+1:]...)
if len(a.Allocations) == 0 {
Expand All @@ -65,6 +75,7 @@ func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.Accep
Allocations: a.Allocations,
}}, nil
}

return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrNotFound, "requested port and channel allocation does not exist")
}

Expand Down Expand Up @@ -128,3 +139,12 @@ func isAllowedAddress(ctx sdk.Context, receiver string, allowedAddrs []string) b
}
return false
}

// 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
// will be granted the privilege to do ICS20 token transfers for the total amount
// of the denomination available at the granter's account.
func UnboundedSpendLimit() sdkmath.Int {
return sdk.NewIntFromBigInt(maxUint256)
}
50 changes: 49 additions & 1 deletion modules/apps/transfer/types/transfer_authorization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package types_test
import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/authz"

"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
ibctesting "github.com/cosmos/ibc-go/v7/testing"
"github.com/cosmos/ibc-go/v7/testing/mock"
Expand Down Expand Up @@ -86,6 +85,48 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
suite.Require().Len(updatedAuthz.Allocations, 1)
},
},
{
"success: with unlimited spend limit of max uint256",
func() {
transferAuthz.Allocations[0].SpendLimit = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, types.UnboundedSpendLimit()))
},
func(res authz.AcceptResponse, err error) {
suite.Require().NoError(err)

updatedTransferAuthz, ok := res.Updated.(*types.TransferAuthorization)
suite.Require().True(ok)

remainder := updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf(sdk.DefaultBondDenom)
suite.Require().True(types.UnboundedSpendLimit().Equal(remainder))
},
},
{
"test multiple coins does not overspend",
func() {
transferAuthz.Allocations[0].SpendLimit = transferAuthz.Allocations[0].SpendLimit.Add(
sdk.NewCoins(
sdk.NewCoin("test-denom", sdk.NewInt(100)),
sdk.NewCoin("test-denom2", sdk.NewInt(100)),
)...,
)
msgTransfer.Token = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(50))
},
func(res authz.AcceptResponse, err error) {
suite.Require().NoError(err)

updatedTransferAuthz, ok := res.Updated.(*types.TransferAuthorization)
suite.Require().True(ok)

remainder := updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf(sdk.DefaultBondDenom)
suite.Require().True(sdk.NewInt(50).Equal(remainder))

remainder = updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf("test-denom")
suite.Require().True(sdk.NewInt(100).Equal(remainder))

remainder = updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf("test-denom2")
suite.Require().True(sdk.NewInt(100).Equal(remainder))
},
},
{
"no spend limit set for MsgTransfer port/channel",
func() {
Expand Down Expand Up @@ -190,6 +231,13 @@ func (suite *TypesTestSuite) TestTransferAuthorizationValidateBasic() {
},
true,
},
{
"success: with unlimited spend limit of max uint256",
func() {
transferAuthz.Allocations[0].SpendLimit = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, types.UnboundedSpendLimit()))
},
true,
},
{
"empty allocations",
func() {
Expand Down

0 comments on commit 7e6eb4c

Please sign in to comment.