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

) (#3580) * 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 <[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]> (cherry picked from commit 7e6eb4c) # Conflicts: # CHANGELOG.md # e2e/tests/core/client_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/transfer_authorization.go # modules/apps/transfer/types/transfer_authorization_test.go * resolving conflicts --------- Co-authored-by: Vladislav Varadinov <[email protected]> Co-authored-by: Damian Nolan <[email protected]>
cosmos · May 15, 2023 · 94221f7 · 94221f7
1 parent 55caad4
commit 94221f7
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 27 deletions.
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.
 
@@ -13,7 +13,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/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go
@@ -1,6 +1,11 @@
 package types
 
 import (
+	"math/big"
+
+	errorsmod "cosmossdk.io/errors"
+	sdkmath "cosmossdk.io/math"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/x/authz"
@@ -9,10 +14,11 @@ import (
 	host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
 )
 
-const gasCostPerIteration = uint64(10)
-
 var _ authz.Authorization = &TransferAuthorization{}
 
+// 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{
@@ -33,37 +39,45 @@ func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.Accep
 	}
 
 	for index, allocation := range a.Allocations {
-		if allocation.SourceChannel == msgTransfer.SourceChannel && allocation.SourcePort == msgTransfer.SourcePort {
-			limitLeft, isNegative := allocation.SpendLimit.SafeSub(msgTransfer.Token)
-			if isNegative {
-				return authz.AcceptResponse{}, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "requested amount is more than spend limit")
-			}
+		if !(allocation.SourceChannel == msgTransfer.SourceChannel && allocation.SourcePort == msgTransfer.SourcePort) {
+			continue
+		}
 
-			if !isAllowedAddress(ctx, msgTransfer.Receiver, allocation.AllowList) {
-				return authz.AcceptResponse{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "not allowed address for transfer")
-			}
+		if !isAllowedAddress(ctx, msgTransfer.Receiver, allocation.AllowList) {
+			return authz.AcceptResponse{}, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "not allowed receiver address for transfer")
+		}
 
-			if limitLeft.IsZero() {
-				a.Allocations = append(a.Allocations[:index], a.Allocations[index+1:]...)
-				if len(a.Allocations) == 0 {
-					return authz.AcceptResponse{Accept: true, Delete: true}, nil
-				}
-				return authz.AcceptResponse{Accept: true, Delete: false, Updated: &TransferAuthorization{
-					Allocations: a.Allocations,
-				}}, nil
-			}
-			a.Allocations[index] = Allocation{
-				SourcePort:    allocation.SourcePort,
-				SourceChannel: allocation.SourceChannel,
-				SpendLimit:    limitLeft,
-				AllowList:     allocation.AllowList,
-			}
+		// 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(sdkerrors.ErrInsufficientFunds, "requested amount is more than spend limit")
+		}
 
+		if limitLeft.IsZero() {
+			a.Allocations = append(a.Allocations[:index], a.Allocations[index+1:]...)
+			if len(a.Allocations) == 0 {
+				return authz.AcceptResponse{Accept: true, Delete: true}, nil
+			}
 			return authz.AcceptResponse{Accept: true, Delete: false, Updated: &TransferAuthorization{
 				Allocations: a.Allocations,
 			}}, nil
 		}
+		a.Allocations[index] = Allocation{
+			SourcePort:    allocation.SourcePort,
+			SourceChannel: allocation.SourceChannel,
+			SpendLimit:    limitLeft,
+			AllowList:     allocation.AllowList,
+		}
+
+		return authz.AcceptResponse{Accept: true, Delete: false, Updated: &TransferAuthorization{
+			Allocations: a.Allocations,
+		}}, nil
 	}
+
 	return authz.AcceptResponse{}, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "requested port and channel allocation does not exist")
 }
 
@@ -117,6 +131,8 @@ func isAllowedAddress(ctx sdk.Context, receiver string, allowedAddrs []string) b
 		return true
 	}
 
+	gasCostPerIteration := ctx.KVGasConfig().IterNextCostFlat
+
 	for _, addr := range allowedAddrs {
 		ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")
 		if addr == receiver {
@@ -125,3 +141,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
@@ -86,6 +86,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 +232,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() {