Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multisignature specs #680

Merged
merged 5 commits into from Dec 5, 2022
Merged

Multisignature specs #680

merged 5 commits into from Dec 5, 2022

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Oct 25, 2022

Specs addressing #81

@grarco grarco requested a review from Fraccaman October 25, 2022 12:49
@grarco grarco marked this pull request as ready for review November 23, 2022 14:42
@grarco grarco requested a review from cwgoes November 23, 2022 14:53
tzemanovic
tzemanovic previously approved these changes Nov 24, 2022
The [replay protection](./replay-protection.md) mechanism of Namada will prevent third-party malicious users from replaying a transaction having a multisig account as the source. This mechanism, though, is not enough to completely protect multisigned transactions: in this case, in fact, the threat could come from the members of the account themselves.

With the current hash-based replay protection mechanism, once the transaction has been executed, its hash gets stored to prevent a replay. The issue, with a multisig transaction, is that the same transaction with a different set or amount of signatures would have a different hash, meaning that it can be replayed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it might be worth adding a counter to multisig for this reason

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we hash the inner-inner transaction (what the multisig signers sign over) and replay-protect with that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a better idea! We can use the hash of an unsigned tx, which we can reconstruct from a signed tx (we already do this for sig verification)

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! see comments

The k-of-n multisignature validity predicate authorizes transactions on the basis of k out of n parties approving them. This document targets the encrypted wasm transactions: at the moment there's no support for multisignature on wrapper or protocol transactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need multisignatures for wrapper or protocol transactions, as far as I am aware

## Protocol

Namada transactions get signed before being delivered to the network. This signature is then checked by the VPs to determine the validity of the transaction. To support multisignature we need to extend the current `SignedTxData` struct to the following:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either:

  • there should be one VP for all accounts, which is a multisignature (and regular accounts are just 1-of-1), or
  • we should use a different struct for the two different VPs

It looks like you're proposing the latter, right? wdyt about the former? it does reduce the number of VPs by one...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of a single VP should work and we could use vp_user also for multisig accounts

Steps 1 and 2 allow us to short-circuit the validation process and avoid unnecessary processing and storage access. Each signature will be validated **only** against the public key found in the list at the specified index. Step 3 will halt as soon as it retrieves enough valid signatures to match the threshold, meaning that the remaining signatures will not be verified.

In the transaction initiating the established address, the submitter will be able to provide a custom VP if the provided one doesn't impose the desired constraints.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I think we won't support non-whitelisted VPs in Namada initially though, omit this for now?

- An initial transaction to be used as an initializer for the relevant data

## Multisig account init validation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to skip all this by having a fixed code for a k-of-n multisig VP and just different data

Copy link
Contributor Author

@grarco grarco Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah in the end we won't apply any kind of validation (actually not even in case the VP was a custom one)

The [replay protection](./replay-protection.md) mechanism of Namada will prevent third-party malicious users from replaying a transaction having a multisig account as the source. This mechanism, though, is not enough to completely protect multisigned transactions: in this case, in fact, the threat could come from the members of the account themselves.

With the current hash-based replay protection mechanism, once the transaction has been executed, its hash gets stored to prevent a replay. The issue, with a multisig transaction, is that the same transaction with a different set or amount of signatures would have a different hash, meaning that it can be replayed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we hash the inner-inner transaction (what the multisig signers sign over) and replay-protect with that?

cwgoes
cwgoes previously approved these changes Nov 30, 2022
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - thanks!

@tzemanovic tzemanovic merged commit 3357ac9 into main Dec 5, 2022
@tzemanovic tzemanovic deleted the fraccaman+grarco/multisig-specs branch December 5, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants