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

Fix multisignature #2595

Closed
Arjentix opened this issue Aug 8, 2022 · 6 comments
Closed

Fix multisignature #2595

Arjentix opened this issue Aug 8, 2022 · 6 comments
Assignees
Labels
Epic iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security

Comments

@Arjentix
Copy link
Contributor

Arjentix commented Aug 8, 2022

After the merge of #2582 we will broke multisignature mechanism. It was working because of signature checking mistake (see #2580).

@Arjentix Arjentix added iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security Yellow Epic labels Aug 8, 2022
@Arjentix
Copy link
Contributor Author

Arjentix commented Aug 8, 2022

The perfect fix we are thinking of is to allow users to use WASM smartcontract validators to check transactions.
That requires things such as:

  • WASM validators
  • Second queue for multisig transactions

@Arjentix
Copy link
Contributor Author

@SamHSmith

@Erigara
Copy link
Contributor

Erigara commented Dec 22, 2022

Second queue for multisig transactions

But how we will decide which transaction is multisig?

@Erigara
Copy link
Contributor

Erigara commented Dec 22, 2022

Also it's worth noticing that our Queue is broken in terms of multisigs.

Problem is the following:
We have method check_tx which treats false result of signature check as error and does not allow to add such signatures into the queue.
Mutisig transactions that does not received enough signatures would return false (because such transaction is not complete).
Simplest solution for such problem would be to allow putting transaction into the queue without signature_check (or at ) and pop from the queue transactions that pass `signature_check.
Problem with such approach is that malicious actor can flood queue with invalid transactions.

@Erigara
Copy link
Contributor

Erigara commented Dec 23, 2022

After meeting with @Arjentix we have two possible solutions for the problem:

  1. Revert to initial approach where we allow to put transaction into the queue even if it not yet pass signature check condition.
    Some pros and cons of the first approach:
    +) Simple and straightforward to implement;
    +) Using isi for signature check is more transparent then wasm;
    -) Such approach could suffer from flooding of transactions that don't pass signature check (they will stay in the queue untill expired).

  2. Introduce 3 variant (Allow, Deny, Pending) logic for signature check where we would reject to add transaction into the queue if it is signed incorrectly and allow to add transaction that still waits for more signatures.
    Some pros and cons of the second approach:
    +) More protected against flooding, because we can cut off some of invalid transactions, but malicious actor still can flood queue with transactions that are waiting for more transactions;
    +) Same advantage about transparency;
    -) Much harder to write signature check condition for 3 variant logic;
    -) Increasing size of Value even further;

  3. For switching to WASM validators we are still considering all pros and cons.

As addition for method 1 and 2 second queue could be introduced, which will partially solve flooding problem, because transactions that are waiting for more signatures could be stored here. And transactions that pass signature check would be processed without any problem. However second queue still can reach it's full capacity.

@mversic
Copy link
Contributor

mversic commented Jan 27, 2023

I will close this issue in favor of #3088

@mversic mversic closed this as completed Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic iroha2-dev The re-implementation of a BFT hyperledger in RUST Security This issue asks for improved security
Projects
None yet
Development

No branches or pull requests

4 participants