From 49cf5d2a8fe9e565f09108a233943afe325ba608 Mon Sep 17 00:00:00 2001 From: 170210 Date: Tue, 19 Mar 2024 17:05:20 +0900 Subject: [PATCH 1/5] fix: use accAddress to compare in validatebasic function in collection & token modules Signed-off-by: 170210 --- x/collection/msgs.go | 9 ++++++--- x/token/msgs.go | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/x/collection/msgs.go b/x/collection/msgs.go index 4c416335f0..7c103fc6c7 100644 --- a/x/collection/msgs.go +++ b/x/collection/msgs.go @@ -428,14 +428,17 @@ func (m MsgAuthorizeOperator) ValidateBasic() error { return err } - if _, err := sdk.AccAddressFromBech32(m.Holder); err != nil { + holderAcc, err := sdk.AccAddressFromBech32(m.Holder) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid holder address: %s", m.Holder) } - if _, err := sdk.AccAddressFromBech32(m.Operator); err != nil { + + operatorAcc, err := sdk.AccAddressFromBech32(m.Operator) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid operator address: %s", m.Operator) } - if m.Operator == m.Holder { + if holderAcc.Equals(operatorAcc) { return ErrApproverProxySame } diff --git a/x/token/msgs.go b/x/token/msgs.go index d3d315aa0a..5029a1be64 100644 --- a/x/token/msgs.go +++ b/x/token/msgs.go @@ -146,14 +146,17 @@ func (m MsgAuthorizeOperator) ValidateBasic() error { return err } - if _, err := sdk.AccAddressFromBech32(m.Holder); err != nil { + holderAcc, err := sdk.AccAddressFromBech32(m.Holder) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid holder address: %s", m.Holder) } - if _, err := sdk.AccAddressFromBech32(m.Operator); err != nil { + + operatorAcc, err := sdk.AccAddressFromBech32(m.Operator) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid operator address: %s", m.Operator) } - if m.Operator == m.Holder { + if holderAcc.Equals(operatorAcc) { return ErrApproverProxySame } From 628861db8be316d67145d4aa17a9f8e4a9e240fb Mon Sep 17 00:00:00 2001 From: 170210 Date: Tue, 19 Mar 2024 17:05:32 +0900 Subject: [PATCH 2/5] chore: add test cases Signed-off-by: 170210 --- x/collection/msgs_test.go | 26 +++++++++++++++++++------- x/token/msgs_test.go | 20 +++++++++++++------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/x/collection/msgs_test.go b/x/collection/msgs_test.go index 236629d1ab..bc6e690763 100644 --- a/x/collection/msgs_test.go +++ b/x/collection/msgs_test.go @@ -376,15 +376,15 @@ func TestMsgOperatorSendNFT(t *testing.T) { } func TestMsgAuthorizeOperator(t *testing.T) { - addrs := make([]sdk.AccAddress, 2) + addrs := make([]string, 2) for i := range addrs { - addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() } testCases := map[string]struct { contractID string - holder sdk.AccAddress - operator sdk.AccAddress + holder string + operator string err error }{ "valid msg": { @@ -407,14 +407,26 @@ func TestMsgAuthorizeOperator(t *testing.T) { holder: addrs[0], err: sdkerrors.ErrInvalidAddress, }, + "proxy and approver should be different": { + contractID: "deadbeef", + holder: addrs[0], + operator: addrs[0], + err: collection.ErrApproverProxySame, + }, + "proxy and approver should be different (uppercase)": { + contractID: "deadbeef", + holder: addrs[0], + operator: strings.ToUpper(addrs[0]), + err: collection.ErrApproverProxySame, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { msg := collection.MsgAuthorizeOperator{ ContractId: tc.contractID, - Holder: tc.holder.String(), - Operator: tc.operator.String(), + Holder: tc.holder, + Operator: tc.operator, } require.ErrorIs(t, msg.ValidateBasic(), tc.err) @@ -422,7 +434,7 @@ func TestMsgAuthorizeOperator(t *testing.T) { return } - require.Equal(t, []sdk.AccAddress{tc.holder}, msg.GetSigners()) + require.Equal(t, []sdk.AccAddress{sdk.MustAccAddressFromBech32(tc.holder)}, msg.GetSigners()) }) } } diff --git a/x/token/msgs_test.go b/x/token/msgs_test.go index 298dfb6c4b..b41b2f403d 100644 --- a/x/token/msgs_test.go +++ b/x/token/msgs_test.go @@ -221,15 +221,15 @@ func TestMsgRevokeOperator(t *testing.T) { } func TestMsgAuthorizeOperator(t *testing.T) { - addrs := make([]sdk.AccAddress, 2) + addrs := make([]string, 2) for i := range addrs { - addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() } testCases := map[string]struct { contractID string - holder sdk.AccAddress - operator sdk.AccAddress + holder string + operator string err error }{ "valid msg": { @@ -258,14 +258,20 @@ func TestMsgAuthorizeOperator(t *testing.T) { operator: addrs[0], err: token.ErrApproverProxySame, }, + "proxy and approver should be different (uppercase)": { + contractID: "deadbeef", + holder: addrs[0], + operator: strings.ToUpper(addrs[0]), + err: token.ErrApproverProxySame, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { msg := token.MsgAuthorizeOperator{ ContractId: tc.contractID, - Holder: tc.holder.String(), - Operator: tc.operator.String(), + Holder: tc.holder, + Operator: tc.operator, } err := msg.ValidateBasic() @@ -274,7 +280,7 @@ func TestMsgAuthorizeOperator(t *testing.T) { return } - require.Equal(t, []sdk.AccAddress{tc.holder}, msg.GetSigners()) + require.Equal(t, []sdk.AccAddress{sdk.MustAccAddressFromBech32(tc.holder)}, msg.GetSigners()) }) } } From f4ec72371185b89cfaec10d220db1dc6c65cf0b6 Mon Sep 17 00:00:00 2001 From: 170210 Date: Tue, 19 Mar 2024 17:16:18 +0900 Subject: [PATCH 3/5] chore: update CHANGELOG.md Signed-off-by: 170210 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d14bde6448..38defd73bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/auth) [#1274](https://github.com/Finschia/finschia-sdk/pull/1274) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. * (x/collection) [\#1276](https://github.com/Finschia/finschia-sdk/pull/1276) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis * (x/foundation) [\#1277](https://github.com/Finschia/finschia-sdk/pull/1277) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic +* (x/collection, x/token) [\#1288](https://github.com/Finschia/finschia-sdk/pull/1288) use accAddress to compare in validatebasic function in collection & token modules ### Removed From 2c64d225ee81fb51c3eb34939f73e134fea7f9ec Mon Sep 17 00:00:00 2001 From: 170210 Date: Tue, 19 Mar 2024 18:33:05 +0900 Subject: [PATCH 4/5] fixup: fix for comment Signed-off-by: 170210 --- x/collection/msgs.go | 9 ++++++--- x/token/msgs.go | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/x/collection/msgs.go b/x/collection/msgs.go index 7c103fc6c7..3da74a2c0e 100644 --- a/x/collection/msgs.go +++ b/x/collection/msgs.go @@ -474,14 +474,17 @@ func (m MsgRevokeOperator) ValidateBasic() error { return err } - if _, err := sdk.AccAddressFromBech32(m.Holder); err != nil { + holderAcc, err := sdk.AccAddressFromBech32(m.Holder) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid holder address: %s", m.Holder) } - if _, err := sdk.AccAddressFromBech32(m.Operator); err != nil { + + operatorAcc, err := sdk.AccAddressFromBech32(m.Operator) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid operator address: %s", m.Operator) } - if m.Operator == m.Holder { + if holderAcc.Equals(operatorAcc) { return ErrApproverProxySame } diff --git a/x/token/msgs.go b/x/token/msgs.go index 5029a1be64..7ba1b886e0 100644 --- a/x/token/msgs.go +++ b/x/token/msgs.go @@ -103,14 +103,17 @@ func (m MsgRevokeOperator) ValidateBasic() error { return err } - if _, err := sdk.AccAddressFromBech32(m.Holder); err != nil { + holderAcc, err := sdk.AccAddressFromBech32(m.Holder) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid holder address: %s", m.Holder) } - if _, err := sdk.AccAddressFromBech32(m.Operator); err != nil { + + operatorAcc, err := sdk.AccAddressFromBech32(m.Operator) + if err != nil { return sdkerrors.ErrInvalidAddress.Wrapf("invalid operator address: %s", m.Operator) } - if m.Operator == m.Holder { + if holderAcc.Equals(operatorAcc) { return ErrApproverProxySame } From bda77056211535ccaadff5989e03fe94f62a15d7 Mon Sep 17 00:00:00 2001 From: 170210 Date: Tue, 19 Mar 2024 18:38:04 +0900 Subject: [PATCH 5/5] fixup: add test cases Signed-off-by: 170210 --- x/collection/msgs_test.go | 26 +++++++++++++++++++------- x/token/msgs_test.go | 20 +++++++++++++------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/x/collection/msgs_test.go b/x/collection/msgs_test.go index bc6e690763..aff826fbf4 100644 --- a/x/collection/msgs_test.go +++ b/x/collection/msgs_test.go @@ -440,15 +440,15 @@ func TestMsgAuthorizeOperator(t *testing.T) { } func TestMsgRevokeOperator(t *testing.T) { - addrs := make([]sdk.AccAddress, 2) + addrs := make([]string, 2) for i := range addrs { - addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() } testCases := map[string]struct { contractID string - holder sdk.AccAddress - operator sdk.AccAddress + holder string + operator string err error }{ "valid msg": { @@ -471,14 +471,26 @@ func TestMsgRevokeOperator(t *testing.T) { holder: addrs[0], err: sdkerrors.ErrInvalidAddress, }, + "proxy and approver should be different": { + contractID: "deadbeef", + holder: addrs[0], + operator: addrs[0], + err: collection.ErrApproverProxySame, + }, + "proxy and approver should be different (uppercase)": { + contractID: "deadbeef", + holder: addrs[0], + operator: strings.ToUpper(addrs[0]), + err: collection.ErrApproverProxySame, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { msg := collection.MsgRevokeOperator{ ContractId: tc.contractID, - Holder: tc.holder.String(), - Operator: tc.operator.String(), + Holder: tc.holder, + Operator: tc.operator, } require.ErrorIs(t, msg.ValidateBasic(), tc.err) @@ -486,7 +498,7 @@ func TestMsgRevokeOperator(t *testing.T) { return } - require.Equal(t, []sdk.AccAddress{tc.holder}, msg.GetSigners()) + require.Equal(t, []sdk.AccAddress{sdk.MustAccAddressFromBech32(tc.holder)}, msg.GetSigners()) }) } } diff --git a/x/token/msgs_test.go b/x/token/msgs_test.go index b41b2f403d..9fe3dd4549 100644 --- a/x/token/msgs_test.go +++ b/x/token/msgs_test.go @@ -162,15 +162,15 @@ func TestMsgOperatorSend(t *testing.T) { } func TestMsgRevokeOperator(t *testing.T) { - addrs := make([]sdk.AccAddress, 2) + addrs := make([]string, 2) for i := range addrs { - addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + addrs[i] = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() } testCases := map[string]struct { contractID string - holder sdk.AccAddress - operator sdk.AccAddress + holder string + operator string err error }{ "valid msg": { @@ -199,14 +199,20 @@ func TestMsgRevokeOperator(t *testing.T) { operator: addrs[0], err: token.ErrApproverProxySame, }, + "operator and holder should be different (uppercase)": { + contractID: "deadbeef", + holder: addrs[0], + operator: strings.ToUpper(addrs[0]), + err: token.ErrApproverProxySame, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { msg := token.MsgRevokeOperator{ ContractId: tc.contractID, - Holder: tc.holder.String(), - Operator: tc.operator.String(), + Holder: tc.holder, + Operator: tc.operator, } err := msg.ValidateBasic() @@ -215,7 +221,7 @@ func TestMsgRevokeOperator(t *testing.T) { return } - require.Equal(t, []sdk.AccAddress{tc.holder}, msg.GetSigners()) + require.Equal(t, []sdk.AccAddress{sdk.MustAccAddressFromBech32(tc.holder)}, msg.GetSigners()) }) } }