From 0b8193916ac6fc549c317d6e928fbe6782387ac2 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 16 Nov 2022 10:11:42 +0100 Subject: [PATCH] fix(group): add group members weight checks (backport #13869) (#13880) * fix(group): add group members weight checks (#13869) (cherry picked from commit 3423442ab198434adc29862414dc49990155083f) # Conflicts: # CHANGELOG.md # api/cosmos/tx/v1beta1/service.pulsar.go # types/tx/service.pb.go # x/group/keeper/keeper.go * fix conflicts * updates * updates Co-authored-by: Julien Robert --- CHANGELOG.md | 1 + x/group/internal/math/dec.go | 13 ++++++++++++- x/group/internal/math/dec_test.go | 8 ++++++++ x/group/keeper/keeper.go | 5 +++-- x/group/keeper/keeper_test.go | 20 ++++++++++++++++++++ x/group/keeper/msg_server.go | 12 +++++------- x/group/module/abci.go | 6 +----- x/group/msgs.go | 5 +---- 8 files changed, 51 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14027dea492c..81c204f0b437 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (x/group) [#13869](https://github.com/cosmos/cosmos-sdk/pull/13869) Group members weight must be positive and a finite number. * (x/bank) [#13821](https://github.com/cosmos/cosmos-sdk/pull/13821) Fix bank store migration of coin metadata. * (x/group) [#13808](https://github.com/cosmos/cosmos-sdk/pull/13808) Fix propagation of message events to the current context in `EndBlocker`. * (x/gov) [#13728](https://github.com/cosmos/cosmos-sdk/pull/13728) Fix propagation of message events to the current context in `EndBlocker`. diff --git a/x/group/internal/math/dec.go b/x/group/internal/math/dec.go index 07d789e4cb46..3755304ccacd 100644 --- a/x/group/internal/math/dec.go +++ b/x/group/internal/math/dec.go @@ -1,4 +1,4 @@ -// Package math provides helper functions for doing mathematical calculations and parsing for the ecocredit module. +// Package math provides helper functions for doing mathematical calculations and parsing for the group module. package math import ( @@ -46,11 +46,22 @@ func (x Dec) IsPositive() bool { return !x.dec.Negative && !x.dec.IsZero() } +func (x Dec) IsFinite() bool { + return x.dec.Form != apd.Finite +} + +// NewDecFromString returns a new Dec from a string +// It only support finite numbers, not NaN, +Inf, -Inf func NewDecFromString(s string) (Dec, error) { d, _, err := apd.NewFromString(s) if err != nil { return Dec{}, errors.ErrInvalidDecString.Wrap(err.Error()) } + + if d.Form != apd.Finite { + return Dec{}, errors.ErrInvalidDecString.Wrapf("expected a finite decimal, got %s", s) + } + return Dec{*d}, nil } diff --git a/x/group/internal/math/dec_test.go b/x/group/internal/math/dec_test.go index 6bf0edb5afe0..22bca12d1669 100644 --- a/x/group/internal/math/dec_test.go +++ b/x/group/internal/math/dec_test.go @@ -54,6 +54,14 @@ func TestDec(t *testing.T) { require.NoError(t, err) minusFivePointZero, err := NewDecFromString("-5.0") require.NoError(t, err) + _, err = NewDecFromString("inf") + require.Error(t, err) + _, err = NewDecFromString("Infinite") + require.Error(t, err) + _, err = NewDecFromString("foo") + require.Error(t, err) + _, err = NewDecFromString("NaN") + require.Error(t, err) res, err := two.Add(zero) require.NoError(t, err) diff --git a/x/group/keeper/keeper.go b/x/group/keeper/keeper.go index fdbc5b923e98..8dcdcc306ea6 100644 --- a/x/group/keeper/keeper.go +++ b/x/group/keeper/keeper.go @@ -380,6 +380,8 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error { if err != nil { return nil } + + //nolint:gosec // implicit memory aliasing in for loop for _, proposal := range proposals { policyInfo, err := k.getGroupPolicyInfo(ctx, proposal.GroupPolicyAddress) if err != nil { @@ -400,8 +402,7 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error { return err } } else { - err = k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo) //nolint:gosec // implicit memory aliasing in for loop - if err != nil { + if err := k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo); err != nil { return sdkerrors.Wrap(err, "doTallyAndUpdate") } diff --git a/x/group/keeper/keeper_test.go b/x/group/keeper/keeper_test.go index a485a38b17b8..0db2166b6861 100644 --- a/x/group/keeper/keeper_test.go +++ b/x/group/keeper/keeper_test.go @@ -192,6 +192,26 @@ func (s *TestSuite) TestCreateGroup() { }, expErr: true, }, + "invalid member weight - Inf": { + req: &group.MsgCreateGroup{ + Admin: addr1.String(), + Members: []group.MemberRequest{{ + Address: addr3.String(), + Weight: "inf", + }}, + }, + expErr: true, + }, + "invalid member weight - NaN": { + req: &group.MsgCreateGroup{ + Admin: addr1.String(), + Members: []group.MemberRequest{{ + Address: addr3.String(), + Weight: "NaN", + }}, + }, + expErr: true, + }, } var seq uint32 = 1 diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go index fd144525cc34..2320b417ca4c 100644 --- a/x/group/keeper/msg_server.go +++ b/x/group/keeper/msg_server.go @@ -687,15 +687,13 @@ func (k Keeper) doTallyAndUpdate(ctx sdk.Context, p *group.Proposal, electorate sinceSubmission := ctx.BlockTime().Sub(p.SubmitTime) // duration passed since proposal submission. result, err := policy.Allow(tallyResult, electorate.TotalWeight, sinceSubmission) - // If the result was final (i.e. enough votes to pass) or if the voting - // period ended, then we consider the proposal as final. - isFinal := result.Final || ctx.BlockTime().After(p.VotingPeriodEnd) - - switch { - case err != nil: + if err != nil { return sdkerrors.Wrap(err, "policy allow") + } - case isFinal: + // If the result was final (i.e. enough votes to pass) or if the voting + // period ended, then we consider the proposal as final. + if isFinal := result.Final || ctx.BlockTime().After(p.VotingPeriodEnd); isFinal { if err := k.pruneVotes(ctx, p.Id); err != nil { return err } diff --git a/x/group/module/abci.go b/x/group/module/abci.go index 4e3817f804b1..22809b61d60e 100644 --- a/x/group/module/abci.go +++ b/x/group/module/abci.go @@ -9,12 +9,8 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) { if err := k.TallyProposalsAtVPEnd(ctx); err != nil { panic(err) } - pruneProposals(ctx, k) -} -func pruneProposals(ctx sdk.Context, k keeper.Keeper) { - err := k.PruneProposals(ctx) - if err != nil { + if err := k.PruneProposals(ctx); err != nil { panic(err) } } diff --git a/x/group/msgs.go b/x/group/msgs.go index 639bcda1de37..6cab5e3dfbfe 100644 --- a/x/group/msgs.go +++ b/x/group/msgs.go @@ -293,10 +293,7 @@ func (m MsgCreateGroupPolicy) GetSignBytes() []byte { // GetSigners returns the expected signers for a MsgCreateGroupPolicy. func (m MsgCreateGroupPolicy) GetSigners() []sdk.AccAddress { - admin, err := sdk.AccAddressFromBech32(m.Admin) - if err != nil { - panic(err) - } + admin := sdk.MustAccAddressFromBech32(m.Admin) return []sdk.AccAddress{admin} }