From ddf029450377ee986c9a62d348530286a6d4a96c Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 14 May 2024 16:12:34 +0200 Subject: [PATCH 1/5] fix(x/accounts): check for overflows in multisig weights and votes --- x/accounts/defaults/multisig/account.go | 37 +++++++++++++++++--- x/accounts/defaults/multisig/account_test.go | 32 +++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/x/accounts/defaults/multisig/account.go b/x/accounts/defaults/multisig/account.go index 6ab25a8440d9..7ff0704404d9 100644 --- a/x/accounts/defaults/multisig/account.go +++ b/x/accounts/defaults/multisig/account.go @@ -91,7 +91,10 @@ func (a *Account) Init(ctx context.Context, msg *v1.MsgInit) (*v1.MsgInitRespons return nil, err } - totalWeight += msg.Members[i].Weight + totalWeight, err = safeAdd(totalWeight, msg.Members[i].Weight) + if err != nil { + return nil, err + } } if err := validateConfig(*msg.Config, totalWeight); err != nil { @@ -266,11 +269,20 @@ func (a Account) ExecuteProposal(ctx context.Context, msg *v1.MsgExecuteProposal switch v1.VoteOption(vote) { case v1.VoteOption_VOTE_OPTION_YES: - yesVotes += weight + yesVotes, err = safeAdd(yesVotes, weight) + if err != nil { + return true, err + } case v1.VoteOption_VOTE_OPTION_NO: - noVotes += weight + noVotes, err = safeAdd(noVotes, weight) + if err != nil { + return true, err + } case v1.VoteOption_VOTE_OPTION_ABSTAIN: - abstainVotes += weight + abstainVotes, err = safeAdd(abstainVotes, weight) + if err != nil { + return true, err + } } return false, nil }) @@ -278,7 +290,11 @@ func (a Account) ExecuteProposal(ctx context.Context, msg *v1.MsgExecuteProposal return nil, err } - totalWeight := yesVotes + noVotes + abstainVotes + totalWeight, err := safeAdd(yesVotes, noVotes, abstainVotes) + if err != nil { + return nil, err + } + var ( rejectErr error execErr error @@ -387,3 +403,14 @@ func (a *Account) RegisterQueryHandlers(builder *accountstd.QueryBuilder) { accountstd.RegisterQueryHandler(builder, a.QueryProposal) accountstd.RegisterQueryHandler(builder, a.QueryConfig) } + +func safeAdd(nums ...uint64) (uint64, error) { + var sum uint64 + for _, num := range nums { + if sum+num < sum { + return 0, errors.New("overflow") + } + sum += num + } + return sum, nil +} diff --git a/x/accounts/defaults/multisig/account_test.go b/x/accounts/defaults/multisig/account_test.go index ab95b5be4803..53ad9f6f45d6 100644 --- a/x/accounts/defaults/multisig/account_test.go +++ b/x/accounts/defaults/multisig/account_test.go @@ -2,6 +2,7 @@ package multisig import ( "context" + "math" "testing" "time" @@ -658,3 +659,34 @@ func TestProposalPassing(t *testing.T) { require.Equal(t, expectedMembers, cfg.Members) } + +func TestWeightOverflow(t *testing.T) { + ctx, ss := newMockContext(t) + acc := setup(t, ctx, ss, nil) + + startAcc := &v1.MsgInit{ + Config: &v1.Config{ + Threshold: 2640, + Quorum: 2000, + VotingPeriod: 60, + }, + Members: []*v1.Member{ + { + Address: "addr1", + Weight: math.MaxUint64, + }, + }, + } + + _, err := acc.Init(ctx, startAcc) + require.NoError(t, err) + + // add another member with weight 1 to trigger an overflow + startAcc.Members = append(startAcc.Members, &v1.Member{ + Address: "addr2", + Weight: 1, + }) + _, err = acc.Init(ctx, startAcc) + require.ErrorContains(t, err, "overflow") + +} From 96289d8aafa51af66b090fe774a10a0cf212fd5d Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 14 May 2024 17:12:40 +0200 Subject: [PATCH 2/5] lint fix --- x/accounts/defaults/multisig/account_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/accounts/defaults/multisig/account_test.go b/x/accounts/defaults/multisig/account_test.go index 53ad9f6f45d6..efc80c43cac9 100644 --- a/x/accounts/defaults/multisig/account_test.go +++ b/x/accounts/defaults/multisig/account_test.go @@ -688,5 +688,4 @@ func TestWeightOverflow(t *testing.T) { }) _, err = acc.Init(ctx, startAcc) require.ErrorContains(t, err, "overflow") - } From 00f8d18f6fe5ee7f975d8d492ca42755e585b4cf Mon Sep 17 00:00:00 2001 From: Facundo Date: Wed, 15 May 2024 18:30:44 +0200 Subject: [PATCH 3/5] check overflow on update config --- x/accounts/defaults/multisig/update_config.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x/accounts/defaults/multisig/update_config.go b/x/accounts/defaults/multisig/update_config.go index a1966c787b0b..150a80c57975 100644 --- a/x/accounts/defaults/multisig/update_config.go +++ b/x/accounts/defaults/multisig/update_config.go @@ -44,7 +44,11 @@ func (a Account) UpdateConfig(ctx context.Context, msg *v1.MsgUpdateConfig) (*v1 // get the weight from the stored members totalWeight := uint64(0) err := a.Members.Walk(ctx, nil, func(_ []byte, value uint64) (stop bool, err error) { - totalWeight += value + var adderr error + totalWeight, adderr = safeAdd(totalWeight, value) + if adderr != nil { + return true, err + } return false, nil }) if err != nil { From f800a9200681e9fe72047834d371b832382964dc Mon Sep 17 00:00:00 2001 From: Facundo Date: Wed, 15 May 2024 21:35:35 +0200 Subject: [PATCH 4/5] fix --- x/accounts/defaults/multisig/update_config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/accounts/defaults/multisig/update_config.go b/x/accounts/defaults/multisig/update_config.go index 150a80c57975..d352d49926fa 100644 --- a/x/accounts/defaults/multisig/update_config.go +++ b/x/accounts/defaults/multisig/update_config.go @@ -47,7 +47,7 @@ func (a Account) UpdateConfig(ctx context.Context, msg *v1.MsgUpdateConfig) (*v1 var adderr error totalWeight, adderr = safeAdd(totalWeight, value) if adderr != nil { - return true, err + return true, adderr } return false, nil }) From 3350270fe3409c0c4966cded76aae71dbfd157b6 Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 21 May 2024 11:18:26 +0200 Subject: [PATCH 5/5] remove check from tally and add test for update --- x/accounts/defaults/multisig/account.go | 20 ++++------------- x/accounts/defaults/multisig/account_test.go | 23 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/x/accounts/defaults/multisig/account.go b/x/accounts/defaults/multisig/account.go index 7ff0704404d9..d6fadbfabff9 100644 --- a/x/accounts/defaults/multisig/account.go +++ b/x/accounts/defaults/multisig/account.go @@ -269,20 +269,11 @@ func (a Account) ExecuteProposal(ctx context.Context, msg *v1.MsgExecuteProposal switch v1.VoteOption(vote) { case v1.VoteOption_VOTE_OPTION_YES: - yesVotes, err = safeAdd(yesVotes, weight) - if err != nil { - return true, err - } + yesVotes += weight case v1.VoteOption_VOTE_OPTION_NO: - noVotes, err = safeAdd(noVotes, weight) - if err != nil { - return true, err - } + noVotes += weight case v1.VoteOption_VOTE_OPTION_ABSTAIN: - abstainVotes, err = safeAdd(abstainVotes, weight) - if err != nil { - return true, err - } + abstainVotes += weight } return false, nil }) @@ -290,10 +281,7 @@ func (a Account) ExecuteProposal(ctx context.Context, msg *v1.MsgExecuteProposal return nil, err } - totalWeight, err := safeAdd(yesVotes, noVotes, abstainVotes) - if err != nil { - return nil, err - } + totalWeight := yesVotes + noVotes + abstainVotes var ( rejectErr error diff --git a/x/accounts/defaults/multisig/account_test.go b/x/accounts/defaults/multisig/account_test.go index efc80c43cac9..58d3e11a4457 100644 --- a/x/accounts/defaults/multisig/account_test.go +++ b/x/accounts/defaults/multisig/account_test.go @@ -313,6 +313,29 @@ func TestUpdateConfig(t *testing.T) { }, }, }, + { + "change members, invalid weights", + &v1.MsgUpdateConfig{ + UpdateMembers: []*v1.Member{ + { + Address: "addr1", + Weight: math.MaxUint64, + }, + { + Address: "addr2", + Weight: 1, + }, + }, + Config: &v1.Config{ + Threshold: 666, + Quorum: 400, + VotingPeriod: 60, + }, + }, + "overflow", + nil, + nil, + }, } for _, tc := range testcases {