Skip to content

Commit

Permalink
fix(feegrant): infinite feegrant bug (cosmos#16097)
Browse files Browse the repository at this point in the history
  • Loading branch information
@assafmo @JeancarloBarrios
assafmo authored and JeancarloBarrios committed Sep 28, 2024
1 parent 7174f6e commit e844399
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 155 deletions.
14 changes: 9 additions & 5 deletions x/feegrant/filtered_fee.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,16 @@ func (a *AllowedMsgAllowance) Accept(ctx context.Context, fee sdk.Coins, msgs []
}

remove, err := allowance.Accept(ctx, fee, msgs)
if err == nil && !remove {
if err = a.SetAllowance(allowance); err != nil {
return false, err
}
if err != nil {
return false, err
}
return remove, err

a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message))
if err != nil {
return false, err
}

return remove, nil
}

func (a *AllowedMsgAllowance) allowedMsgsToMap(ctx context.Context) (map[string]struct{}, error) {
Expand Down
180 changes: 30 additions & 150 deletions x/feegrant/filtered_fee_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,167 +1,69 @@
package feegrant_test

import (
"context"
"testing"
"time"

bank "github.com/cosmos/cosmos-sdk/x/bank/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"

appmodulev2 "cosmossdk.io/core/appmodule/v2"
corecontext "cosmossdk.io/core/context"
"cosmossdk.io/core/header"
storetypes "cosmossdk.io/store/types"
banktypes "cosmossdk.io/x/bank/types"
"cosmossdk.io/x/feegrant"
"cosmossdk.io/x/feegrant/module"

addresscodec "github.com/cosmos/cosmos-sdk/codec/address"
codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil"
"github.com/cosmos/cosmos-sdk/testutil"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/simapp"
sdk "github.com/cosmos/cosmos-sdk/types"
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
"github.com/cosmos/cosmos-sdk/x/feegrant"
)

func TestFilteredFeeValidAllow(t *testing.T) {
key := storetypes.NewKVStoreKey(feegrant.StoreKey)
testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test"))
encCfg := moduletestutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}, module.AppModule{})

ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()})
app := simapp.Setup(false)

eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 10))
atom := sdk.NewCoins(sdk.NewInt64Coin("atom", 555))
smallAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 43))
smallAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 488))
bigAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 1000))
leftAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 512))
now := ctx.HeaderInfo().Time
oneHour := now.Add(1 * time.Hour)

ac := addresscodec.NewBech32Codec("cosmos")
basicAllowance, _ := codectypes.NewAnyWithValue(&feegrant.BasicAllowance{
SpendLimit: bigAtom,
})

// msg we will call in the all cases
call := banktypes.MsgSend{}
cases := map[string]struct {
allowance *feegrant.BasicAllowance
msgs []string
allowance *feegrant.AllowedMsgAllowance
// all other checks are ignored if valid=false
fee sdk.Coins
blockTime time.Time
valid bool
accept bool
remove bool
remains sdk.Coins
}{
"msg contained": {
allowance: &feegrant.BasicAllowance{},
msgs: []string{sdk.MsgTypeURL(&call)},
accept: true,
},
"msg not contained": {
allowance: &feegrant.BasicAllowance{},
msgs: []string{"/cosmos.gov.v1.MsgVote"},
accept: false,
},
"small fee without expire": {
allowance: &feegrant.BasicAllowance{
SpendLimit: atom,
"internal fee is updated": {
allowance: &feegrant.AllowedMsgAllowance{
Allowance: basicAllowance,
AllowedMessages: []string{"/cosmos.bank.v1beta1.MsgSend"},
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: smallAtom,
accept: true,
remove: false,
remains: leftAtom,
},
"all fee without expire": {
allowance: &feegrant.BasicAllowance{
SpendLimit: smallAtom,
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: smallAtom,
accept: true,
remove: true,
},
"wrong fee": {
allowance: &feegrant.BasicAllowance{
SpendLimit: smallAtom,
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: eth,
accept: false,
},
"non-expired": {
allowance: &feegrant.BasicAllowance{
SpendLimit: atom,
Expiration: &oneHour,
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: smallAtom,
blockTime: now,
accept: true,
remove: false,
remains: leftAtom,
},
"expired": {
allowance: &feegrant.BasicAllowance{
SpendLimit: atom,
Expiration: &now,
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: smallAtom,
blockTime: oneHour,
accept: false,
remove: true,
},
"fee more than allowed": {
allowance: &feegrant.BasicAllowance{
SpendLimit: atom,
Expiration: &oneHour,
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: bigAtom,
blockTime: now,
accept: false,
},
"with out spend limit": {
allowance: &feegrant.BasicAllowance{
Expiration: &oneHour,
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: bigAtom,
blockTime: now,
accept: true,
},
"expired no spend limit": {
allowance: &feegrant.BasicAllowance{
Expiration: &now,
},
msgs: []string{sdk.MsgTypeURL(&call)},
fee: bigAtom,
blockTime: oneHour,
accept: false,
},
}

for name, tc := range cases {
for name, stc := range cases {
tc := stc // to make scopelint happy
t.Run(name, func(t *testing.T) {
err := tc.allowance.ValidateBasic()
require.NoError(t, err)

ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: tc.blockTime})

// create grant
granter, grantee := sdk.AccAddress("granter"), sdk.AccAddress("grantee")
allowance, err := feegrant.NewAllowedMsgAllowance(tc.allowance, tc.msgs)
require.NoError(t, err)
granterStr, err := ac.BytesToString(granter)
require.NoError(t, err)
granteeStr, err := ac.BytesToString(grantee)
require.NoError(t, err)
ctx := app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockTime(tc.blockTime)

// now try to deduct
removed, err := allowance.Accept(context.WithValue(ctx, corecontext.EnvironmentContextKey, appmodulev2.Environment{
HeaderService: mockHeaderService{},
GasService: mockGasService{},
}), tc.fee, []sdk.Msg{&call})
removed, err := tc.allowance.Accept(ctx, tc.fee, []sdk.Msg{
&bank.MsgSend{
FromAddress: "gm",
ToAddress: "gn",
Amount: tc.fee,
},
})
if !tc.accept {
require.Error(t, err)
return
Expand All @@ -170,32 +72,10 @@ func TestFilteredFeeValidAllow(t *testing.T) {

require.Equal(t, tc.remove, removed)
if !removed {
// mimic save & load process (#10564)
// the cached allowance was correct even before the fix,
// however, the saved value was not.
// so we need this to catch the bug.

// create a new updated grant
newGrant, err := feegrant.NewGrant(
granterStr,
granteeStr,
allowance)
require.NoError(t, err)

// save the grant
bz, err := encCfg.Codec.Marshal(&newGrant)
require.NoError(t, err)

// load the grant
var loadedGrant feegrant.Grant
err = encCfg.Codec.Unmarshal(bz, &loadedGrant)
require.NoError(t, err)
var basicAllowanceLeft feegrant.BasicAllowance
app.AppCodec().Unmarshal(tc.allowance.Allowance.Value, &basicAllowanceLeft)

newAllowance, err := loadedGrant.GetGrant()
require.NoError(t, err)
feeAllowance, err := newAllowance.(*feegrant.AllowedMsgAllowance).GetAllowance()
require.NoError(t, err)
assert.Equal(t, tc.remains, feeAllowance.(*feegrant.BasicAllowance).SpendLimit)
assert.Equal(t, tc.remains, basicAllowanceLeft.SpendLimit)
}
})
}
Expand Down

0 comments on commit e844399

Please sign in to comment.