fix: Prevent signing from wrong key in multisig #12446
Conversation
…ita/fix-multisig-key
Codecov Report
@@ Coverage Diff @@
## main #12446 +/- ##
==========================================
- Coverage 65.32% 65.28% -0.04%
==========================================
Files 693 693
Lines 71823 71869 +46
==========================================
+ Hits 46919 46921 +2
- Misses 22265 22308 +43
- Partials 2639 2640 +1
|
…ita/fix-multisig-key
likhita-809
requested review from
amaury1093,
alexanderbez,
anilcse,
atheeshp,
JeancarloBarrios,
julienrbrt and
tac0turtle
|
Crap, I forgot to mention a changelog is needed prior to merging this. @likhita-809 can you please create a PR with a chagnelog entry for this please? Also, should we backport this to v0.46? |
There was a problem hiding this comment.
I don't think this should have been merged, I have some doubts that this PR introduces regressions, would like to clear that up first.
@alexanderbez for non-trivial PRs like this one, how about let's keep 2 approvals before merge?
| @@ -59,6 +60,16 @@ func NewMultiRecord(name string, pk cryptotypes.PubKey) (*Record, error) { | |||
| return newRecord(name, pk, recordMultiItem) | |||
| } | |||
|
|
|||
| // GetMultisigPubKey fetches a public key of the multi type record | |||
| func (k *Record) GetMultisigPubKey() (*multisig.LegacyAminoPubKey, error) { | |||
There was a problem hiding this comment.
I don't think this is a good API on Record: there should only be a method to get the PubKey.
Then, in tx_sign.go, we can cast .GetPubKey().(*multisig.LegacyAminoPubKey)
| @@ -243,15 +242,44 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error { | |||
| } | |||
|
|
|||
| overwrite, _ := f.GetBool(flagOverwrite) | |||
| if multisig != "" { | |||
| multisigAddr, err := sdk.AccAddressFromBech32(multisig) | |||
There was a problem hiding this comment.
It seems to me that this line got deleted. @likhita-809 is this intended?
There was a problem hiding this comment.
yes, because at the line L251, we are getting both name and address of the multisig irrespective of multisig flag value(be it name or address).
| @@ -1057,7 +1072,7 @@ func (s *IntegrationTestSuite) TestSignWithMultisig() { | |||
| // as the main point of this test is to test the `--multisig` flag with an address | |||
| // that is not in the keyring. | |||
| _, err = TxSignExec(val1.ClientCtx, addr1, multiGeneratedTx2File.Name(), "--multisig", multisigAddr.String()) | |||
| s.Require().Contains(err.Error(), "tx intended signer does not match the given signer") | |||
| s.Require().Contains(err.Error(), "error getting account from keybase") | |||
There was a problem hiding this comment.
The fact that we changed this test seems a bit suspicious to me.
There was a problem hiding this comment.
In this test, we are passing multisig flag value as its address.
As you can see here https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/client/cli/tx_sign.go#L247, since it is an address the error check will be skipped. But if multisig flag is a name, then we'll encounter error getting account from keybase error.
This reverts commit 28f4fb9.
Sure. |
Description
Closes: #12328
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change