From 8a376a41c9d4fbd1b1311e8e4a0eb9fc3e05a559 Mon Sep 17 00:00:00 2001 From: MD Aleem <72057206+aleem1314@users.noreply.github.com> Date: Fri, 9 Apr 2021 18:03:27 +0530 Subject: [PATCH] x/authz charge gas for expensive authorizations (#8995) * WIP * Add consume gas * update authz.go * add build flag * Update x/staking/types/authz.go * Update x/staking/types/authz.go * add tests docs * review changes * fix operations * Update x/authz/types/msgs.go Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> * review changes * update gas cost * review changes Co-authored-by: SaReN Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> --- types/msgservice/msg_service.go | 7 +++ types/tx/types.go | 12 +--- x/authz/client/cli/tx_test.go | 4 +- x/authz/exported/authorizations.go | 8 ++- x/authz/keeper/keeper.go | 2 +- x/authz/simulation/operations.go | 8 ++- x/authz/types/generic_authorization.go | 20 +++++-- x/authz/types/generic_authorization_test.go | 20 +++++++ x/authz/types/msgs.go | 6 +- x/bank/types/send_authorization.go | 15 ++++- x/bank/types/send_authorization_test.go | 64 +++++++++++++++++++++ x/staking/types/authz.go | 31 +++++++--- x/staking/types/authz_test.go | 15 ++++- 13 files changed, 175 insertions(+), 37 deletions(-) create mode 100644 x/authz/types/generic_authorization_test.go create mode 100644 x/bank/types/send_authorization_test.go diff --git a/types/msgservice/msg_service.go b/types/msgservice/msg_service.go index 277ee3d157..ddffed943e 100644 --- a/types/msgservice/msg_service.go +++ b/types/msgservice/msg_service.go @@ -3,6 +3,7 @@ package msgservice import ( "context" "fmt" + "strings" "github.com/gogo/protobuf/proto" "google.golang.org/grpc" @@ -42,3 +43,9 @@ func RegisterMsgServiceDesc(registry codectypes.InterfaceRegistry, sd *grpc.Serv func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) { return nil, nil } + +// IsServiceMsg checks if a type URL corresponds to a service method name, +// i.e. /cosmos.bank.Msg/Send vs /cosmos.bank.MsgSend +func IsServiceMsg(typeURL string) bool { + return strings.Count(typeURL, "/") >= 2 +} diff --git a/types/tx/types.go b/types/tx/types.go index a0b05b5dce..66fbb193ad 100644 --- a/types/tx/types.go +++ b/types/tx/types.go @@ -2,12 +2,12 @@ package tx import ( "fmt" - "strings" codectypes "github.com/cosmos/cosmos-sdk/codec/types" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/msgservice" ) // MaxGasWanted defines the max gas allowed. @@ -27,7 +27,7 @@ func (t *Tx) GetMsgs() []sdk.Msg { res := make([]sdk.Msg, len(anys)) for i, any := range anys { var msg sdk.Msg - if isServiceMsg(any.TypeUrl) { + if msgservice.IsServiceMsg(any.TypeUrl) { req := any.GetCachedValue() if req == nil { panic("Any cached value is nil. Transaction messages must be correctly packed Any values.") @@ -183,7 +183,7 @@ func (m *TxBody) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error { for _, any := range m.Messages { // If the any's typeUrl contains 2 slashes, then we unpack the any into // a ServiceMsg struct as per ADR-031. - if isServiceMsg(any.TypeUrl) { + if msgservice.IsServiceMsg(any.TypeUrl) { var req sdk.MsgRequest err := unpacker.UnpackAny(any, &req) if err != nil { @@ -222,9 +222,3 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) { registry.RegisterInterface("cosmos.tx.v1beta1.Tx", (*sdk.Tx)(nil)) registry.RegisterImplementations((*sdk.Tx)(nil), &Tx{}) } - -// isServiceMsg checks if a type URL corresponds to a service method name, -// i.e. /cosmos.bank.Msg/Send vs /cosmos.bank.MsgSend -func isServiceMsg(typeURL string) bool { - return strings.Count(typeURL, "/") >= 2 -} diff --git a/x/authz/client/cli/tx_test.go b/x/authz/client/cli/tx_test.go index 179feca333..b7dad15e65 100644 --- a/x/authz/client/cli/tx_test.go +++ b/x/authz/client/cli/tx_test.go @@ -156,8 +156,8 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() { fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), }, - &sdk.TxResponse{}, 29, - false, + nil, 0, + true, }, { "failed with error both validators not allowed", diff --git a/x/authz/exported/authorizations.go b/x/authz/exported/authorizations.go index d7cca5ae79..32fea0b0b3 100644 --- a/x/authz/exported/authorizations.go +++ b/x/authz/exported/authorizations.go @@ -3,8 +3,6 @@ package exported import ( "github.com/gogo/protobuf/proto" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -17,5 +15,9 @@ type Authorization interface { // Accept determines whether this grant permits the provided sdk.ServiceMsg to be performed, and if // so provides an upgraded authorization instance. - Accept(msg sdk.ServiceMsg, block tmproto.Header) (updated Authorization, delete bool, err error) + Accept(ctx sdk.Context, msg sdk.ServiceMsg) (updated Authorization, delete bool, err error) + + // ValidateBasic does a simple validation check that + // doesn't require access to any other information. + ValidateBasic() error } diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index 33d6248f71..dcbee170cb 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -87,7 +87,7 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, service if authorization == nil { return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "authorization not found") } - updated, del, err := authorization.Accept(serviceMsg, ctx.BlockHeader()) + updated, del, err := authorization.Accept(ctx, serviceMsg) if err != nil { return nil, err } diff --git a/x/authz/simulation/operations.go b/x/authz/simulation/operations.go index 959b0c06fe..8fef468b06 100644 --- a/x/authz/simulation/operations.go +++ b/x/authz/simulation/operations.go @@ -96,8 +96,12 @@ func SimulateMsgGrantAuthorization(ak types.AccountKeeper, bk types.BankKeeper, } blockTime := ctx.BlockTime() + spendLimit := spendableCoins.Sub(fees) + if spendLimit == nil { + return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantAuthorization, "spend limit is nil"), nil, nil + } msg, err := types.NewMsgGrantAuthorization(granter.Address, grantee.Address, - banktype.NewSendAuthorization(spendableCoins.Sub(fees)), blockTime.AddDate(1, 0, 0)) + banktype.NewSendAuthorization(spendLimit), blockTime.AddDate(1, 0, 0)) if err != nil { return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantAuthorization, err.Error()), nil, err @@ -249,7 +253,7 @@ func SimulateMsgExecuteAuthorized(ak types.AccountKeeper, bk types.BankKeeper, k msg := types.NewMsgExecAuthorized(grantee.Address, []sdk.ServiceMsg{execMsg}) sendGrant := targetGrant.Authorization.GetCachedValue().(*banktype.SendAuthorization) - _, _, err = sendGrant.Accept(execMsg, ctx.BlockHeader()) + _, _, err = sendGrant.Accept(ctx, execMsg) if err != nil { return simtypes.NoOpMsg(types.ModuleName, TypeMsgExecDelegated, err.Error()), nil, nil } diff --git a/x/authz/types/generic_authorization.go b/x/authz/types/generic_authorization.go index 672260c67f..101f3acf16 100644 --- a/x/authz/types/generic_authorization.go +++ b/x/authz/types/generic_authorization.go @@ -1,9 +1,9 @@ package types import ( - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/msgservice" "github.com/cosmos/cosmos-sdk/x/authz/exported" ) @@ -19,11 +19,19 @@ func NewGenericAuthorization(methodName string) *GenericAuthorization { } // MethodName implements Authorization.MethodName. -func (cap GenericAuthorization) MethodName() string { - return cap.MessageName +func (authorization GenericAuthorization) MethodName() string { + return authorization.MessageName } // Accept implements Authorization.Accept. -func (cap GenericAuthorization) Accept(msg sdk.ServiceMsg, block tmproto.Header) (updated exported.Authorization, delete bool, err error) { - return &cap, false, nil +func (authorization GenericAuthorization) Accept(ctx sdk.Context, msg sdk.ServiceMsg) (updated exported.Authorization, delete bool, err error) { + return &authorization, false, nil +} + +// ValidateBasic implements Authorization.ValidateBasic. +func (authorization GenericAuthorization) ValidateBasic() error { + if !msgservice.IsServiceMsg(authorization.MessageName) { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, " %s is not a valid service msg", authorization.MessageName) + } + return nil } diff --git a/x/authz/types/generic_authorization_test.go b/x/authz/types/generic_authorization_test.go new file mode 100644 index 0000000000..e3330d1c65 --- /dev/null +++ b/x/authz/types/generic_authorization_test.go @@ -0,0 +1,20 @@ +package types_test + +import ( + "testing" + + "github.com/cosmos/cosmos-sdk/x/authz/types" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + "github.com/stretchr/testify/require" +) + +func TestGenericAuthorization(t *testing.T) { + t.Log("verify ValidateBasic returns error for non-service msg") + authorization := types.NewGenericAuthorization(banktypes.TypeMsgSend) + require.Error(t, authorization.ValidateBasic()) + + t.Log("verify ValidateBasic returns nil for service msg") + authorization = types.NewGenericAuthorization(banktypes.SendAuthorization{}.MethodName()) + require.NoError(t, authorization.ValidateBasic()) + require.Equal(t, banktypes.SendAuthorization{}.MethodName(), authorization.MessageName) +} diff --git a/x/authz/types/msgs.go b/x/authz/types/msgs.go index 8fa3bdd1f0..0b72d1a7e8 100644 --- a/x/authz/types/msgs.go +++ b/x/authz/types/msgs.go @@ -63,7 +63,11 @@ func (msg MsgGrantAuthorizationRequest) ValidateBasic() error { return sdkerrors.Wrap(ErrInvalidExpirationTime, "Time can't be in the past") } - return nil + authorization, ok := msg.Authorization.GetCachedValue().(exported.Authorization) + if !ok { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (exported.Authorization)(nil), msg.Authorization.GetCachedValue()) + } + return authorization.ValidateBasic() } // GetGrantAuthorization returns the cache value from the MsgGrantAuthorization.Authorization if present. diff --git a/x/bank/types/send_authorization.go b/x/bank/types/send_authorization.go index ccd755c1fc..82d1bf1e63 100644 --- a/x/bank/types/send_authorization.go +++ b/x/bank/types/send_authorization.go @@ -3,8 +3,6 @@ package types import ( "reflect" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" authz "github.com/cosmos/cosmos-sdk/x/authz/exported" @@ -27,7 +25,7 @@ func (authorization SendAuthorization) MethodName() string { } // Accept implements Authorization.Accept. -func (authorization SendAuthorization) Accept(msg sdk.ServiceMsg, block tmproto.Header) (updated authz.Authorization, delete bool, err error) { +func (authorization SendAuthorization) Accept(ctx sdk.Context, msg sdk.ServiceMsg) (updated authz.Authorization, delete bool, err error) { if reflect.TypeOf(msg.Request) == reflect.TypeOf(&MsgSend{}) { msg, ok := msg.Request.(*MsgSend) if ok { @@ -44,3 +42,14 @@ func (authorization SendAuthorization) Accept(msg sdk.ServiceMsg, block tmproto. } return nil, false, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "type mismatch") } + +// ValidateBasic implements Authorization.ValidateBasic. +func (authorization SendAuthorization) ValidateBasic() error { + if authorization.SpendLimit == nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "spend limit cannot be nil") + } + if !authorization.SpendLimit.IsAllPositive() { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "spend limit cannot be negitive") + } + return nil +} diff --git a/x/bank/types/send_authorization_test.go b/x/bank/types/send_authorization_test.go new file mode 100644 index 0000000000..055a0b1fb0 --- /dev/null +++ b/x/bank/types/send_authorization_test.go @@ -0,0 +1,64 @@ +package types_test + +import ( + "testing" + + "github.com/cosmos/cosmos-sdk/simapp" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/bank/types" + "github.com/stretchr/testify/require" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" +) + +var ( + coins1000 = sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(1000))) + coins500 = sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(500))) + fromAddr = sdk.AccAddress("_____from _____") + toAddr = sdk.AccAddress("_______to________") +) + +func TestSendAuthorization(t *testing.T) { + app := simapp.Setup(false) + ctx := app.BaseApp.NewContext(false, tmproto.Header{}) + authorization := types.NewSendAuthorization(coins1000) + + t.Log("verify authorization returns valid method name") + require.Equal(t, authorization.MethodName(), "/cosmos.bank.v1beta1.Msg/Send") + require.NoError(t, authorization.ValidateBasic()) + send := types.NewMsgSend(fromAddr, toAddr, coins1000) + srvMsg := sdk.ServiceMsg{ + MethodName: "/cosmos.bank.v1beta1.Msg/Send", + Request: send, + } + require.NoError(t, authorization.ValidateBasic()) + + t.Log("verify updated authorization returns nil") + updated, del, err := authorization.Accept(ctx, srvMsg) + require.NoError(t, err) + require.True(t, del) + require.Nil(t, updated) + + authorization = types.NewSendAuthorization(coins1000) + require.Equal(t, authorization.MethodName(), "/cosmos.bank.v1beta1.Msg/Send") + require.NoError(t, authorization.ValidateBasic()) + send = types.NewMsgSend(fromAddr, toAddr, coins500) + srvMsg = sdk.ServiceMsg{ + MethodName: "/cosmos.bank.v1beta1.Msg/Send", + Request: send, + } + require.NoError(t, authorization.ValidateBasic()) + updated, del, err = authorization.Accept(ctx, srvMsg) + + t.Log("verify updated authorization returns remaining spent limit") + require.NoError(t, err) + require.False(t, del) + require.NotNil(t, updated) + sendAuth := types.NewSendAuthorization(coins500) + require.Equal(t, sendAuth.String(), updated.String()) + + t.Log("expect updated authorization nil after spending remaining amount") + updated, del, err = updated.Accept(ctx, srvMsg) + require.NoError(t, err) + require.True(t, del) + require.Nil(t, updated) +} diff --git a/x/staking/types/authz.go b/x/staking/types/authz.go index e88288da4e..32e8b719d1 100644 --- a/x/staking/types/authz.go +++ b/x/staking/types/authz.go @@ -1,18 +1,21 @@ package types import ( - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" authz "github.com/cosmos/cosmos-sdk/x/authz/exported" ) +// TODO: Revisit this once we have propoer gas fee framework. +// Tracking issues https://github.com/cosmos/cosmos-sdk/issues/9054, https://github.com/cosmos/cosmos-sdk/discussions/9072 +const gasCostPerIteration = uint64(10) + var ( - _ authz.Authorization = &StakeAuthorization{} - TypeDelegate = "/cosmos.staking.v1beta1.Msg/Delegate" - TypeUndelegate = "/cosmos.staking.v1beta1.Msg/Undelegate" - TypeBeginRedelegate = "/cosmos.staking.v1beta1.Msg/BeginRedelegate" + _ authz.Authorization = &StakeAuthorization{} + + TypeDelegate = "/cosmos.staking.v1beta1.Msg/Delegate" + TypeUndelegate = "/cosmos.staking.v1beta1.Msg/Undelegate" + TypeBeginRedelegate = "/cosmos.staking.v1beta1.Msg/BeginRedelegate" ) // NewStakeAuthorization creates a new StakeAuthorization object. @@ -46,8 +49,19 @@ func (authorization StakeAuthorization) MethodName() string { return authzType } +func (authorization StakeAuthorization) ValidateBasic() error { + if authorization.MaxTokens != nil && authorization.MaxTokens.IsNegative() { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "negative coin amount: %v", authorization.MaxTokens) + } + if authorization.AuthorizationType == AuthorizationType_AUTHORIZATION_TYPE_UNSPECIFIED { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "unknown authorization type") + } + + return nil +} + // Accept implements Authorization.Accept. -func (authorization StakeAuthorization) Accept(msg sdk.ServiceMsg, block tmproto.Header) (updated authz.Authorization, delete bool, err error) { +func (authorization StakeAuthorization) Accept(ctx sdk.Context, msg sdk.ServiceMsg) (updated authz.Authorization, delete bool, err error) { var validatorAddress string var amount sdk.Coin @@ -68,13 +82,16 @@ func (authorization StakeAuthorization) Accept(msg sdk.ServiceMsg, block tmproto isValidatorExists := false allowedList := authorization.GetAllowList().GetAddress() for _, validator := range allowedList { + ctx.GasMeter().ConsumeGas(gasCostPerIteration, "stake authorization") if validator == validatorAddress { isValidatorExists = true break } } + denyList := authorization.GetDenyList().GetAddress() for _, validator := range denyList { + ctx.GasMeter().ConsumeGas(gasCostPerIteration, "stake authorization") if validator == validatorAddress { return nil, false, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, " cannot delegate/undelegate to %s validator", validator) } diff --git a/x/staking/types/authz_test.go b/x/staking/types/authz_test.go index 18709d9ecd..c43ef9e1fb 100644 --- a/x/staking/types/authz_test.go +++ b/x/staking/types/authz_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ) @@ -21,13 +22,21 @@ var ( ) func TestAuthzAuthorizations(t *testing.T) { + app := simapp.Setup(false) + ctx := app.BaseApp.NewContext(false, tmproto.Header{}) + + // verify ValidateBasic returns error for the AUTHORIZATION_TYPE_UNSPECIFIED authorization type + delAuth, err := stakingtypes.NewStakeAuthorization([]sdk.ValAddress{val1, val2}, []sdk.ValAddress{}, stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_UNSPECIFIED, &coin100) + require.NoError(t, err) + require.Error(t, delAuth.ValidateBasic()) // verify MethodName - delAuth, _ := stakingtypes.NewStakeAuthorization([]sdk.ValAddress{val1, val2}, []sdk.ValAddress{}, stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, &coin100) + delAuth, err = stakingtypes.NewStakeAuthorization([]sdk.ValAddress{val1, val2}, []sdk.ValAddress{}, stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, &coin100) + require.NoError(t, err) require.Equal(t, delAuth.MethodName(), stakingtypes.TypeDelegate) // error both allow & deny list - _, err := stakingtypes.NewStakeAuthorization([]sdk.ValAddress{val1, val2}, []sdk.ValAddress{val1}, stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, &coin100) + _, err = stakingtypes.NewStakeAuthorization([]sdk.ValAddress{val1, val2}, []sdk.ValAddress{val1}, stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, &coin100) require.Error(t, err) // verify MethodName @@ -243,7 +252,7 @@ func TestAuthzAuthorizations(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { delAuth, err := stakingtypes.NewStakeAuthorization(tc.allowed, tc.denied, tc.msgType, tc.limit) require.NoError(t, err) - updated, del, err := delAuth.Accept(tc.srvMsg, tmproto.Header{}) + updated, del, err := delAuth.Accept(ctx, tc.srvMsg) if tc.expectErr { require.Error(t, err) require.Equal(t, tc.isDelete, del)