diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e2752c63c2..0de649b444a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/docs/apps/transfer/authorizations.md b/docs/apps/transfer/authorizations.md index 8835e3d807c..ed6979a6f1d 100644 --- a/docs/apps/transfer/authorizations.md +++ b/docs/apps/transfer/authorizations.md @@ -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. @@ -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`. diff --git a/e2e/tests/core/client_test.go b/e2e/tests/core/client_test.go index 55d0211c9de..bc1f34a1552 100644 --- a/e2e/tests/core/client_test.go +++ b/e2e/tests/core/client_test.go @@ -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, diff --git a/e2e/testsuite/grpc_query.go b/e2e/testsuite/grpc_query.go index d0d263c1ee3..a3f8db2a18d 100644 --- a/e2e/testsuite/grpc_query.go +++ b/e2e/testsuite/grpc_query.go @@ -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 } diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go index 108e210f3fb..7a152b23dc6 100644 --- a/modules/apps/transfer/types/transfer_authorization.go +++ b/modules/apps/transfer/types/transfer_authorization.go @@ -1,10 +1,12 @@ 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" @@ -12,6 +14,9 @@ import ( 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{ @@ -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 { @@ -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") } @@ -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) +} diff --git a/modules/apps/transfer/types/transfer_authorization_test.go b/modules/apps/transfer/types/transfer_authorization_test.go index f0f7f3ab0b9..39d7a827c38 100644 --- a/modules/apps/transfer/types/transfer_authorization_test.go +++ b/modules/apps/transfer/types/transfer_authorization_test.go @@ -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" @@ -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() { @@ -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() {