Skip to content

[Multisig] Sanity check new threshold/key pair during key updates#5

Open
sras wants to merge 1 commit into
murbard:masterfrom
serokell:sras/sanity-check-key-update
Open

[Multisig] Sanity check new threshold/key pair during key updates#5
sras wants to merge 1 commit into
murbard:masterfrom
serokell:sras/sanity-check-key-update

Conversation

@sras

@sras sras commented Mar 18, 2020

Copy link
Copy Markdown

Sanity check new threshold/key pair during key updates

The current key update function allows following bad values for new threshold/key pair.

  1. Threshold value is Zero, which renders multisig contract without any
    additional security.

  2. Threshold value is larger than the number of keys, which locks
    out the multisig contract completely and renders it useless. This could even lock the target contract where the multisig contract acts as an administrator/owner.

This change adds sanity checks that rejects bad threshold/key pairs as
listed above.

The current key update function allows following bad values.

1. Zero threshold value, when renders multisig contract without any
additional security.

2. Threshold value that is larger than the number of keys, which locks
out the multisig contract completely and renders it useless.

This change adds sanity checks that rejects bad threshold/key pairs as
listed above.
@sras sras force-pushed the sras/sanity-check-key-update branch from 8402d1a to 44f900c Compare April 13, 2020 17:26
@rafoo

rafoo commented Apr 19, 2020

Copy link
Copy Markdown
Contributor

IMO these checks should be done off-chain. The Tezos client already check this: https://gitlab.com/tezos/tezos/-/blob/master/src/proto_006_PsCARTHA/lib_client/client_proto_multisig.ml#L691.

@sras

sras commented Jun 29, 2020

Copy link
Copy Markdown
Author

IMO these checks should be done off-chain

@rafoo Why though? Couldn't one can interact with a smart contract via tezos-clients raw contract-call commands (or even via an RPC calls) which, AFAIU does not make any such checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants