Skip to content

Commit

Permalink
fix: prevent signing from wrong key in multisig (#1319)
Browse files Browse the repository at this point in the history
* Add multisig check

* Update CHANGELOG

* Update CHANGELOG.md

(cherry picked from commit c051dcc)

# Conflicts:
#	CHANGELOG.md
  • Loading branch information
ulbqb authored and mergify[bot] committed Mar 28, 2024
1 parent 971875b commit 8255dd1
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 8 deletions.
22 changes: 22 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,33 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Improvements

### Bug Fixes
<<<<<<< HEAD
* (x/auth) [#1281](https://github.com/Finschia/finschia-sdk/pull/1281) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. (backport #1274)
* (x/foundation) [\#1283](https://github.com/Finschia/finschia-sdk/pull/1283) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic (backport #1277)
* (x/collection) [\#1282](https://github.com/Finschia/finschia-sdk/pull/1282) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis (backport #1276)
* (x/collection) [\#1290](https://github.com/Finschia/finschia-sdk/pull/1290) export x/collection params into genesis (backport #1268)
* (x/foundation) [\#1295](https://github.com/Finschia/finschia-sdk/pull/1295) add missing error handling for migration
=======
* chore(deps) [\#1141](https://github.com/Finschia/finschia-sdk/pull/1141) Bump github.com/cosmos/ledger-cosmos-go from 0.12.2 to 0.13.2 to fix ledger signing issue
* (x/auth, x/slashing) [\#1179](https://github.com/Finschia/finschia-sdk/pull/1179) modify missing changes of converting to tendermint
* (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
* (x/collection) [\#1268](https://github.com/Finschia/finschia-sdk/pull/1268) export x/collection params into genesis
* (x/collection) [\#1294](https://github.com/Finschia/finschia-sdk/pull/1294) reject NFT coins on FT APIs
* (sec) [\#1302](https://github.com/Finschia/finschia-sdk/pull/1302) remove map iteration non-determinism with keys + sorting
* (client) [\#1303](https://github.com/Finschia/finschia-sdk/pull/1303) fix possible overflow in BuildUnsignedTx
* (types) [\#1299](https://github.com/Finschia/finschia-sdk/pull/1299) add missing nil checks
* (x/staking) [\#1301](https://github.com/Finschia/finschia-sdk/pull/1301) Use bytes instead of string comparison in delete validator queue (backport cosmos/cosmos-sdk#12303)
* (x/gov) [\#1304](https://github.com/Finschia/finschia-sdk/pull/1304) fetch a failed proposal tally from proposal.FinalTallyResult in the gprc query
* (x/staking) [\#1306](https://github.com/Finschia/finschia-sdk/pull/1306) add validation for potential slashing evasion during re-delegation
* (client/keys) [#1312](https://github.com/Finschia/finschia-sdk/pull/1312) ignore error when key not found in `keys delete`
* (store) [\#1310](https://github.com/Finschia/finschia-sdk/pull/1310) fix app-hash mismatch if upgrade migration commit is interrupted(backport cosmos/cosmos-sdk#13530)
* (types) [\#1313](https://github.com/Finschia/finschia-sdk/pull/1313) fix correctly coalesce coins even with repeated denominations(backport cosmos/cosmos-sdk#13265)
* (x/crypto) [\#1316](https://github.com/Finschia/finschia-sdk/pull/1316) error if incorrect ledger public key (backport cosmos/cosmos-sdk#14460, cosmos/cosmos-sdk#19691)
* (x/auth) [#1319](https://github.com/Finschia/finschia-sdk/pull/1319) prevent signing from wrong key in multisig
>>>>>>> c051dcc91 (fix: prevent signing from wrong key in multisig (#1319))
### Removed

Expand Down
10 changes: 10 additions & 0 deletions x/auth/client/cli/tx_multisign.go
Original file line number Diff line number Diff line change
Expand Up @@ -413,3 +413,13 @@ func getMultisigInfo(clientCtx client.Context, name string) (keyring.Info, error

return multisigInfo, nil
}

func getMultisigRecord(clientCtx client.Context, name string) (keyring.Info, error) {
kb := clientCtx.Keyring
multisigRecord, err := kb.Key(name)
if err != nil {
return nil, errors.Wrap(err, "error getting keybase multisig account")
}

Check warning on line 422 in x/auth/client/cli/tx_multisign.go

View check run for this annotation

Codecov / codecov/patch

x/auth/client/cli/tx_multisign.go#L417-L422

Added lines #L417 - L422 were not covered by tests

return multisigRecord, nil

Check warning on line 424 in x/auth/client/cli/tx_multisign.go

View check run for this annotation

Codecov / codecov/patch

x/auth/client/cli/tx_multisign.go#L424

Added line #L424 was not covered by tests
}
31 changes: 25 additions & 6 deletions x/auth/client/cli/tx_sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"github.com/Finschia/finschia-sdk/client"
"github.com/Finschia/finschia-sdk/client/flags"
"github.com/Finschia/finschia-sdk/client/tx"
sdk "github.com/Finschia/finschia-sdk/types"
kmultisig "github.com/Finschia/finschia-sdk/crypto/keys/multisig"
authclient "github.com/Finschia/finschia-sdk/x/auth/client"
)

Expand Down Expand Up @@ -258,14 +258,33 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {

overwrite, _ := f.GetBool(flagOverwrite)
if multisig != "" {
multisigAddr, err := sdk.AccAddressFromBech32(multisig)
// Bech32 decode error, maybe it's a name, we try to fetch from keyring
multisigAddr, multisigName, _, err := client.GetFromFields(txFactory.Keybase(), multisig, clientCtx.GenerateOnly)

Check warning on line 262 in x/auth/client/cli/tx_sign.go

View check run for this annotation

Codecov / codecov/patch

x/auth/client/cli/tx_sign.go#L261-L262

Added lines #L261 - L262 were not covered by tests
if err != nil {
// Bech32 decode error, maybe it's a name, we try to fetch from keyring
multisigAddr, _, _, err = client.GetFromFields(txFactory.Keybase(), multisig, clientCtx.GenerateOnly)
if err != nil {
return fmt.Errorf("error getting account from keybase: %w", err)
return fmt.Errorf("error getting account from keybase: %w", err)
}
multisigkey, err := getMultisigRecord(clientCtx, multisigName)
if err != nil {
return err
}
multisigPubKey := multisigkey.GetPubKey()
multisigLegacyPub := multisigPubKey.(*kmultisig.LegacyAminoPubKey)

fromRecord, err := clientCtx.Keyring.Key(fromName)
if err != nil {
return fmt.Errorf("error getting account from keybase: %w", err)
}
fromPubKey := fromRecord.GetPubKey()

var found bool
for _, pubkey := range multisigLegacyPub.GetPubKeys() {
if pubkey.Equals(fromPubKey) {
found = true

Check warning on line 282 in x/auth/client/cli/tx_sign.go

View check run for this annotation

Codecov / codecov/patch

x/auth/client/cli/tx_sign.go#L264-L282

Added lines #L264 - L282 were not covered by tests
}
}
if !found {
return fmt.Errorf("signing key is not a part of multisig key")
}

Check warning on line 287 in x/auth/client/cli/tx_sign.go

View check run for this annotation

Codecov / codecov/patch

x/auth/client/cli/tx_sign.go#L285-L287

Added lines #L285 - L287 were not covered by tests
err = authclient.SignTxWithSignerAddress(
txF, clientCtx, multisigAddr, fromName, txBuilder, clientCtx.Offline, overwrite)
if err != nil {
Expand Down
18 changes: 16 additions & 2 deletions x/auth/client/testutil/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ func (s *IntegrationTestSuite) SetupSuite() {
account2, _, err := kb.NewMnemonic("newAccount2", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
s.Require().NoError(err)

// Create a dummy account for testing purpose
_, _, err = kb.NewMnemonic("dummyAccount", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
s.Require().NoError(err)

multi := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{account1.GetPubKey(), account2.GetPubKey()})
_, err = kb.SaveMultisig("multi", multi)
s.Require().NoError(err)
Expand Down Expand Up @@ -790,6 +794,10 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() {
multisigInfo, err := val1.ClientCtx.Keyring.Key("multi")
s.Require().NoError(err)

// Generate dummy account which is not a part of multisig.
dummyAcc, err := val1.ClientCtx.Keyring.Key("dummyAccount")
s.Require().NoError(err)

resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, multisigInfo.GetAddress())
s.Require().NoError(err)

Expand Down Expand Up @@ -842,12 +850,18 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() {

sign1File := testutil.WriteToNewTempFile(s.T(), account1Signature.String())

// Sign with account1
// Sign with account2
account2Signature, err := TxSignExec(val1.ClientCtx, account2.GetAddress(), multiGeneratedTxFile.Name(), "--multisig", multisigInfo.GetAddress().String())
s.Require().NoError(err)

sign2File := testutil.WriteToNewTempFile(s.T(), account2Signature.String())

// Sign with dummy account
dummyAddr := dummyAcc.GetAddress()
_, err = TxSignExec(val1.ClientCtx, dummyAddr, multiGeneratedTxFile.Name(), "--multisig", multisigInfo.GetAddress().String())
s.Require().Error(err)
s.Require().Contains(err.Error(), "signing key is not a part of multisig key")

multiSigWith2Signatures, err := TxMultiSignExec(val1.ClientCtx, multisigInfo.GetName(), multiGeneratedTxFile.Name(), sign1File.Name(), sign2File.Name())
s.Require().NoError(err)

Expand Down Expand Up @@ -902,7 +916,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")
}

func (s *IntegrationTestSuite) TestCLIMultisign() {
Expand Down

0 comments on commit 8255dd1

Please sign in to comment.