From a2553e0f3f46770d5786de89da43da591193ebf5 Mon Sep 17 00:00:00 2001 From: Alexander Peters Date: Thu, 20 Feb 2025 17:14:23 +0100 Subject: [PATCH] Merge commit from fork * Prevent empty groups (cherry picked from commit 95090a0ec1f193104ba7d6d033a490d1515e54d8) * Handle inflight proposals (cherry picked from commit 855983471882068894ed7952b592ab3e61b464b9) * Update changelog * Imports only * Set release date * updates * Update RELEASE_NOTES.md * Update RELEASE_NOTES.md * Update CHANGELOG.md --------- Co-authored-by: Julien Robert Co-authored-by: Alex | Interchain Labs --- CHANGELOG.md | 6 + RELEASE_NOTES.md | 18 ++- x/group/keeper/keeper_test.go | 222 ++++++++++++++----------------- x/group/keeper/msg_server.go | 4 + x/group/simulation/operations.go | 2 +- x/group/types.go | 4 +- x/group/types_test.go | 22 +++ 7 files changed, 146 insertions(+), 132 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ce0894e5122..5fc12aa63060 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,12 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +## [v0.47.16](/~https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.16) - 2025-02-20 + +### Bug Fixes + +* [GHSA-x5vx-95h7-rv4p](/~https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-x5vx-95h7-rv4p) Fix Group module can halt chain when handling a malicious proposal + ## [v0.47.15](/~https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.15) - 2024-12-16 ### Bug Fixes diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 836816e2d78e..9b24ff58d3ba 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,18 +1,16 @@ -# Cosmos SDK v0.47.15 Release Notes +# Cosmos SDK v0.47.16 Release Notes 💬 [**Release Discussion**](/~https://github.com/orgs/cosmos/discussions/6) ## 🚀 Highlights -This release fixes [ABS-0043/ABS-0044](/~https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm). Check the advisory for more information. +This patch release fixes [GHSA-x5vx-95h7-rv4p](/~https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-x5vx-95h7-rv4p). +It resolves a `x/group` module issue that can halt chain when handling a malicious proposal. +Only users of the `x/group` module are affected by this issue. -Additionally, this release is mainly here to disclose the incoming end-of-life of the `v0.47.x` line. +We recommended to upgrade to this patch release as soon as possible. +When upgrading from <= v0.47.15, please use a chain upgrade to ensure that 2/3 of the validator power upgrade to v0.47.16. -Check out the [changelog](/~https://github.com/cosmos/cosmos-sdk/blob/v0.47.15/CHANGELOG.md) for an exhaustive list of changes or [compare changes](/~https://github.com/cosmos/cosmos-sdk/compare/v0.47.14...v0.47.15) from last release. +## 📝 Changelog -## End-of-Life Notice - -`v0.47.15` is the last release of the `v0.47.x` line. Per this version, the v0.47.x line reached its end-of-life. -The SDK team maintains the [latest two major versions of the SDK](/~https://github.com/cosmos/cosmos-sdk/blob/main/RELEASE_PROCESS.md#major-release-maintenance). This means no features, improvements or bug fixes will be backported to the `v0.47.x` line. Per our policy, the `v0.47.x` line will receive security patches only. - -We encourage all chains to upgrade to Cosmos SDK Olympus (`rc`) (`v0.52.0`), or the `v0.50.x` line. +Check out the [changelog](/~https://github.com/cosmos/cosmos-sdk/blob/v0.47.16/CHANGELOG.md) for an exhaustive list of changes or [compare changes](/~https://github.com/cosmos/cosmos-sdk/compare/v0.47.15...v0.47.16) from last release. diff --git a/x/group/keeper/keeper_test.go b/x/group/keeper/keeper_test.go index 1e3b09e0c144..9d1dc1ab13db 100644 --- a/x/group/keeper/keeper_test.go +++ b/x/group/keeper/keeper_test.go @@ -506,20 +506,13 @@ func (s *TestSuite) TestUpdateGroupMetadata() { } func (s *TestSuite) TestUpdateGroupMembers() { - addrs := s.addrs - addr3 := addrs[2] - addr4 := addrs[3] - addr5 := addrs[4] - addr6 := addrs[5] - - member1 := addr5.String() - member2 := addr6.String() - members := []group.MemberRequest{{ - Address: member1, - Weight: "1", - }} + unknownAddr, myAdmin := s.addrs[0].String(), s.addrs[1].String() + member1, member2, member3 := s.addrs[2].String(), s.addrs[3].String(), s.addrs[4].String() + members := []group.MemberRequest{ + {Address: member1, Weight: "1"}, + {Address: member2, Weight: "2"}, + } - myAdmin := addr4.String() groupRes, err := s.groupKeeper.CreateGroup(s.ctx, &group.MsgCreateGroup{ Admin: myAdmin, Members: members, @@ -530,95 +523,83 @@ func (s *TestSuite) TestUpdateGroupMembers() { specs := map[string]struct { req *group.MsgUpdateGroupMembers expErr bool + expErrMsg string expGroup *group.GroupInfo expMembers []*group.GroupMember }{ "add new member": { req: &group.MsgUpdateGroupMembers{ - GroupId: groupID, - Admin: myAdmin, - MemberUpdates: []group.MemberRequest{{ - Address: member2, - Weight: "2", - }}, + GroupId: groupID, + Admin: myAdmin, + MemberUpdates: []group.MemberRequest{{Address: member3, Weight: "3"}}, }, expGroup: &group.GroupInfo{ Id: groupID, Admin: myAdmin, - TotalWeight: "3", + TotalWeight: "6", Version: 2, CreatedAt: s.blockTime, }, expMembers: []*group.GroupMember{ { - Member: &group.Member{ - Address: member2, - Weight: "2", - AddedAt: s.sdkCtx.BlockTime(), - }, + Member: &group.Member{Address: member3, Weight: "3", AddedAt: s.blockTime}, + GroupId: groupID, + }, + { + Member: &group.Member{Address: member1, Weight: "1", AddedAt: s.blockTime}, GroupId: groupID, }, { - Member: &group.Member{ - Address: member1, - Weight: "1", - AddedAt: s.blockTime, - }, + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, GroupId: groupID, }, }, }, "update member": { req: &group.MsgUpdateGroupMembers{ - GroupId: groupID, - Admin: myAdmin, - MemberUpdates: []group.MemberRequest{{ - Address: member1, - Weight: "2", - }}, + GroupId: groupID, + Admin: myAdmin, + MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "2"}}, }, expGroup: &group.GroupInfo{ Id: groupID, Admin: myAdmin, - TotalWeight: "2", + TotalWeight: "4", Version: 2, CreatedAt: s.blockTime, }, expMembers: []*group.GroupMember{ { + Member: &group.Member{Address: member1, Weight: "2", AddedAt: s.blockTime}, + GroupId: groupID, + }, + { + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, GroupId: groupID, - Member: &group.Member{ - Address: member1, - Weight: "2", - AddedAt: s.blockTime, - }, }, }, }, "update member with same data": { req: &group.MsgUpdateGroupMembers{ - GroupId: groupID, - Admin: myAdmin, - MemberUpdates: []group.MemberRequest{{ - Address: member1, - Weight: "1", - }}, + GroupId: groupID, + Admin: myAdmin, + MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "1"}}, }, expGroup: &group.GroupInfo{ Id: groupID, Admin: myAdmin, - TotalWeight: "1", + TotalWeight: "3", Version: 2, CreatedAt: s.blockTime, }, expMembers: []*group.GroupMember{ { GroupId: groupID, - Member: &group.Member{ - Address: member1, - Weight: "1", - AddedAt: s.blockTime, - }, + Member: &group.Member{Address: member1, Weight: "1", AddedAt: s.blockTime}, + }, + { + GroupId: groupID, + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, }, }, }, @@ -627,58 +608,51 @@ func (s *TestSuite) TestUpdateGroupMembers() { GroupId: groupID, Admin: myAdmin, MemberUpdates: []group.MemberRequest{ - { - Address: member1, - Weight: "0", - }, - { - Address: member2, - Weight: "1", - }, + {Address: member1, Weight: "0"}, + {Address: member3, Weight: "1"}, }, }, expGroup: &group.GroupInfo{ Id: groupID, Admin: myAdmin, - TotalWeight: "1", + TotalWeight: "3", Version: 2, CreatedAt: s.blockTime, }, - expMembers: []*group.GroupMember{{ - GroupId: groupID, - Member: &group.Member{ - Address: member2, - Weight: "1", - AddedAt: s.sdkCtx.BlockTime(), + expMembers: []*group.GroupMember{ + { + Member: &group.Member{Address: member3, Weight: "1", AddedAt: s.blockTime}, + GroupId: groupID, }, - }}, + { + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, + GroupId: groupID, + }}, }, "remove existing member": { req: &group.MsgUpdateGroupMembers{ - GroupId: groupID, - Admin: myAdmin, - MemberUpdates: []group.MemberRequest{{ - Address: member1, - Weight: "0", - }}, + GroupId: groupID, + Admin: myAdmin, + MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "0"}}, }, expGroup: &group.GroupInfo{ Id: groupID, Admin: myAdmin, - TotalWeight: "0", + TotalWeight: "2", Version: 2, CreatedAt: s.blockTime, }, - expMembers: []*group.GroupMember{}, + expMembers: []*group.GroupMember{ + { + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, + GroupId: groupID, + }}, }, "remove unknown member": { req: &group.MsgUpdateGroupMembers{ - GroupId: groupID, - Admin: myAdmin, - MemberUpdates: []group.MemberRequest{{ - Address: addr4.String(), - Weight: "0", - }}, + GroupId: groupID, + Admin: myAdmin, + MemberUpdates: []group.MemberRequest{{Address: unknownAddr, Weight: "0"}}, }, expErr: true, expGroup: &group.GroupInfo{ @@ -688,84 +662,92 @@ func (s *TestSuite) TestUpdateGroupMembers() { Version: 1, CreatedAt: s.blockTime, }, - expMembers: []*group.GroupMember{{ - GroupId: groupID, - Member: &group.Member{ - Address: member1, - Weight: "1", - }, - }}, + expMembers: []*group.GroupMember{ + { + GroupId: groupID, + Member: &group.Member{Address: member1, Weight: "1"}, + }, { + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, + GroupId: groupID, + }}, }, "with wrong admin": { req: &group.MsgUpdateGroupMembers{ - GroupId: groupID, - Admin: addr3.String(), - MemberUpdates: []group.MemberRequest{{ - Address: member1, - Weight: "2", - }}, + GroupId: groupID, + Admin: unknownAddr, + MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "2"}}, }, - expErr: true, + expErr: true, + expErrMsg: "not group admin", expGroup: &group.GroupInfo{ Id: groupID, Admin: myAdmin, - TotalWeight: "1", + TotalWeight: "3", Version: 1, CreatedAt: s.blockTime, }, expMembers: []*group.GroupMember{{ GroupId: groupID, - Member: &group.Member{ - Address: member1, - Weight: "1", - }, + Member: &group.Member{Address: member1, Weight: "1"}, + }, { + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, + GroupId: groupID, }}, }, "with unknown groupID": { req: &group.MsgUpdateGroupMembers{ - GroupId: 999, - Admin: myAdmin, - MemberUpdates: []group.MemberRequest{{ - Address: member1, - Weight: "2", - }}, + GroupId: 999, + Admin: myAdmin, + MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "2"}}, }, - expErr: true, + expErr: true, + expErrMsg: "not found", expGroup: &group.GroupInfo{ Id: groupID, Admin: myAdmin, - TotalWeight: "1", + TotalWeight: "3", Version: 1, CreatedAt: s.blockTime, }, expMembers: []*group.GroupMember{{ GroupId: groupID, - Member: &group.Member{ - Address: member1, - Weight: "1", - }, + Member: &group.Member{Address: member1, Weight: "1"}, + }, { + Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime}, + GroupId: groupID, }}, }, + "remove all members": { + req: &group.MsgUpdateGroupMembers{ + GroupId: groupID, + Admin: myAdmin, + MemberUpdates: []group.MemberRequest{ + {Address: member1, Weight: "0"}, + {Address: member2, Weight: "0"}, + }, + }, + expErr: true, + expErrMsg: "group must not be empty", + }, } for msg, spec := range specs { - spec := spec s.Run(msg, func() { sdkCtx, _ := s.sdkCtx.CacheContext() - ctx := sdk.WrapSDKContext(sdkCtx) - _, err := s.groupKeeper.UpdateGroupMembers(ctx, spec.req) + _, err := s.groupKeeper.UpdateGroupMembers(sdkCtx, spec.req) if spec.expErr { s.Require().Error(err) + s.Require().Contains(err.Error(), spec.expErrMsg) return } s.Require().NoError(err) // then - res, err := s.groupKeeper.GroupInfo(ctx, &group.QueryGroupInfoRequest{GroupId: groupID}) + res, err := s.groupKeeper.GroupInfo(sdkCtx, &group.QueryGroupInfoRequest{GroupId: groupID}) s.Require().NoError(err) s.Assert().Equal(spec.expGroup, res.Info) // and members persisted - membersRes, err := s.groupKeeper.GroupMembers(ctx, &group.QueryGroupMembersRequest{GroupId: groupID}) + membersRes, err := s.groupKeeper.GroupMembers(sdkCtx, &group.QueryGroupMembersRequest{GroupId: groupID}) s.Require().NoError(err) loadedMembers := membersRes.Members s.Require().Equal(len(spec.expMembers), len(loadedMembers)) diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go index 348c27c89285..64382a775ed2 100644 --- a/x/group/keeper/msg_server.go +++ b/x/group/keeper/msg_server.go @@ -182,6 +182,10 @@ func (k Keeper) UpdateGroupMembers(goCtx context.Context, req *group.MsgUpdateGr return err } } + // ensure that group has one or more members + if totalWeight.IsZero() { + return errors.ErrInvalid.Wrap("group must not be empty") + } // Update group in the groupTable. g.TotalWeight = totalWeight.String() g.Version++ diff --git a/x/group/simulation/operations.go b/x/group/simulation/operations.go index fe6b53eda0fc..c932e58ee504 100644 --- a/x/group/simulation/operations.go +++ b/x/group/simulation/operations.go @@ -627,7 +627,7 @@ func SimulateMsgUpdateGroupMembers(cdc *codec.ProtoCodec, ak group.AccountKeeper // set existing random group member weight to zero to remove from the group existigMembers := res.Members - if len(existigMembers) > 0 { + if len(existigMembers) > 1 { memberToRemove := existigMembers[r.Intn(len(existigMembers))] var isDuplicateMember bool for idx, m := range members { diff --git a/x/group/types.go b/x/group/types.go index 77df3ced4025..b8803c2511f4 100644 --- a/x/group/types.go +++ b/x/group/types.go @@ -206,7 +206,9 @@ func (p PercentageDecisionPolicy) Allow(tally TallyResult, totalPower string) (D if err != nil { return DecisionPolicyResult{}, sdkerrors.Wrap(err, "total power") } - + if totalPowerDec.IsZero() { + return DecisionPolicyResult{Allow: false, Final: true}, nil + } yesPercentage, err := yesCount.Quo(totalPowerDec) if err != nil { return DecisionPolicyResult{}, err diff --git a/x/group/types_test.go b/x/group/types_test.go index ff117940dc32..d3be27dd769c 100644 --- a/x/group/types_test.go +++ b/x/group/types_test.go @@ -239,6 +239,28 @@ func TestPercentageDecisionPolicyAllow(t *testing.T) { }, false, }, + { + "empty total power", + &group.PercentageDecisionPolicy{ + Percentage: "0.5", + Windows: &group.DecisionPolicyWindows{ + VotingPeriod: time.Second * 100, + }, + }, + &group.TallyResult{ + YesCount: "1", + NoCount: "0", + AbstainCount: "0", + NoWithVetoCount: "0", + }, + "0", + time.Second * 50, + group.DecisionPolicyResult{ + Allow: false, + Final: true, + }, + false, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) {