Skip to content

wallet: prevent multisig accounts from being added as multisig signers (fixes #708) - #721

Closed
mikky1sCp wants to merge 1 commit into
Quantus-Network:mainfrom
mikky1sCp:fix/multisig-signer-validation
Closed

mikky1sCp wants to merge 1 commit into
Quantus-Network:mainfrom
mikky1sCp:fix/multisig-signer-validation

Conversation

@mikky1sCp

@mikky1sCp mikky1sCp commented Oct 27, 2026 •

Copy link
Copy Markdown

What and why

Fixes a bug where a user could add another multisig account as a signer when creating a new multisig. Because a multisig has no private key, it can never sign a transaction. Such a multisig becomes permanently unexecutable — the funds get stuck with no way to move them out through the wallet.

This was reported on-chain; the user had to recover funds manually through the CLI.

Changes

  • add_multisig_screen.dart:
    • _addSigner is now Future<void> and queries the chain via MultisigService.isMultisigIndexed(address) after passing SS58 and duplicate checks.
    • If the address is an indexed multisig, the field shows multisigCreateCannotUseMultisig and the signer is not added.
    • Added an _isAddingSigner flag so the "Add Signer" button is disabled and shows a spinner while the check is in flight, preventing double-taps.
    • Network errors fall back to multisigCreateInvalidSigner (fail closed — never silently add an unchecked signer).
  • app_localizations.dart + _en + _id:
    • Added multisigCreateCannotUseMultisig ("Multisig accounts cannot be used as signers").

Validation

  • Not tested locally against Flutter — I don't have the Flutter SDK installed on this machine. Relying on CI to run flutter analyze and the existing test suite. The change is small, purely additive, and follows the existing patterns in the file (same shape as the duplicate-signer check).
  • Manual review of the flow:
    • Valid regular address → passes SS58 → passes duplicate check → isMultisigIndexed returns false → added (unchanged behaviour).
    • Another multisig address → isMultisigIndexed returns true → field shows error, signer not added (new behaviour).
    • Network error during check → field shows invalidSigner, signer not added (fail closed).
    • Double-tap "Add Signer" while check is running → blocked by _isAddingSigner.

Risks and mitigations

  • Extra network call on add: one GraphQL request per added signer. Adds a few hundred ms of latency to a user-initiated action, which is acceptable.
  • Fail-closed on network error: offline users cannot add a signer until they are back online. This is the safe choice — silently accepting an unchecked address is what caused the original bug.
  • Localisation: added to en and id only, matching the existing locale set.

Follow-ups (out of scope)

  • Discovery of already-broken multisigs: this PR prevents new ones from being created but does not surface existing broken multisigs to the user. A "recovery" flow (e.g. showing a banner on multisig details if any signer is a multisig) would need a separate issue.
  • Shared validation: the SS58 + duplicate + multisig checks could live in MultisigService rather than the screen, so the same rules apply if signer entry is added elsewhere.

fixes #708

@mikky1sCp mikky1sCp changed the title wallet: prevent multisig accounts from being added as multisig signers (#708) wallet: prevent multisig accounts from being added as multisig signers (fixes #708) Oct 27, 2026
@n13 n13 added the bot-review-fix Fixes for the automated review are pushed; review again label Nov 7, 2026
@n13

n13 commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

Fixer loop does not support PRs from forks. Stopping. Use the bot-review label for a one-shot review.

@n13 n13 added bot-review Request automated review from review-bot and removed bot-review-fix Fixes for the automated review are pushed; review again labels Nov 7, 2026
@n13

n13 commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

Closing - already done in #715

@n13 n13 closed this Nov 7, 2026

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: Request changes
Reviewer: GPT-6.1 Sol

Reviewed head f82f3d5990332908ae40f72e22670886d8f88e0b against base 29277c8ed5bb62b4853c800e082ae168fa8e7dd6.

  1. [P1] Add the localization key to the ARB sources before regenerating. mobile-app/lib/l10n/app_localizations.dart:681-685: multisigCreateCannotUseMultisig was added only to the three generated Dart files. Neither app_en.arb nor app_id.arb contains it. The app enables flutter: generate: true, so localization generation removes this getter while add_multisig_screen.dart:119 still references it, breaking compilation. Reproduced by running flutter gen-l10n on copies of this head's localization inputs, then compiling a Flutter test that accesses the new getter: The getter 'multisigCreateCannotUseMultisig' isn't defined. Add both ARB messages and the English metadata, then regenerate the Dart files.

  2. [P2] Prevent creation while an added signer is still being checked. mobile-app/lib/v2/screens/multisig/add_multisig_screen.dart:112-115: _isAddingSigner disables only Add Signer; the Create button's _isDisabled condition ignores it. With an existing ready two-signer configuration, start adding a third signer and tap Create before the lookup completes. _createMultisig captures the old signer list and threshold in its preflight; when the lookup completes, the screen adds the third signer and updates the displayed threshold while submission still uses the earlier draft. For the default thresholds, the screen can show 2-of-3 while creation submits 1-of-2. Include the pending signer check in the creation guard and serialize form changes with submission. Add a regression using a delayed signer lookup and delayed creation preflight.

Validation: inspected the full diff, both signer-entry callbacks, indexer lookup, and creation/submission paths. The isolated localization regeneration/compilation probe reproduced finding 1. Formatting checks fail on add_multisig_screen.dart locally and in GitHub CI; git diff --check also reports trailing whitespace at line 129. Local locked bootstrap is blocked by installed Flutter 3.44.4 versus the repository's pinned 3.47.7; the existing focused SDK test could not execute with dependencies unavailable, and workspace analysis was stopped at the 10-second limit. Finding 2 is based on tracing the asynchronous state and preflight arguments, not an executed widget regression.

The PR was open when review began and was closed before submission; this review covers the recorded head above.

@n13 n13 removed the bot-review Request automated review from review-bot label Nov 7, 2026
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.

Multisig creation allows a multisig account as a signer, creating an unexecutable multisig

2 participants