imp: represent unlimited approvals with MaxUint256 value (#3454)

* 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 <31522760+fedekunze@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> * 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 <damiannolan@gmail.com> * 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 <damiannolan@gmail.com> * Update docs/apps/transfer/authorizations.md * fix: changed to new sentinel value name --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
cosmos · May 15, 2023 · 7e6eb4c · 7e6eb4c
1 parent 3882830
commit 7e6eb4c
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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{
@@ -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
@@ -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() {