V12 SDK Fixes#549
Closed
illuzen wants to merge 36 commits into
Closed
Conversation
SECURITY: Fixes critical vulnerability where multisig address prediction could accept duplicate or oversized signer lists, leading to address collision attacks. Changes: - Rust: Add MIN_SIGNERS=2, MAX_SIGNERS=100 constants - Rust: Canonicalize signers (sort+dedup) before address derivation - Rust: Validate uniqueness, threshold bounds, and signer count limits - Dart: Update _validateSignersAndThreshold to check duplicates and maxSigners - Dart: Change predictMultisigAddress to require minSigners=2 - Tests: Add comprehensive validation tests for both Rust and Dart The Rust layer now rejects: - Fewer than 2 unique signers - More than 100 signers - Duplicate signers in input - Threshold of 0 or exceeding signer count
…s (#93096) SECURITY: Fixes high severity vulnerability where reversible transfer delay units (blocks vs milliseconds) were erased, causing users to potentially misunderstand the actual reversal window. Changes: - Add DelayUnit enum (blocks, milliseconds) to distinguish unit types - Add ReversibleDelay class to preserve both value and unit - Update TransactionInfo to use reversibleDelay instead of raw int - Keep reversibleTimeframe as deprecated getter for backwards compatibility - Update toString() to format delays with correct units: - Blocks: '100 blocks' - Milliseconds: '5 minutes', '1 hour', '45 seconds', or '500 ms' - Update cold-wallet-app sign_transaction_screen to use new API - Add comprehensive tests for delay unit preservation and formatting Users can now correctly see whether a reversible transfer uses a block-based or time-based delay, preventing potential confusion about reversal windows.
…#93184) SECURITY: Fixes high severity vulnerability where a compromised or malicious indexer could return spoofed multisig records with fraudulent signer lists. Changes: - discoverForUser now verifies each discovered multisig's address matches the deterministically predicted address from signers/threshold/nonce - Records with mismatched addresses are silently skipped - Records with invalid signer data are also skipped This prevents attackers from tricking users into signing transactions for fake multisigs where the actual on-chain multisig has different signers than displayed in the UI.
SECURITY: Fixes high severity vulnerability where clearAll() would silently delete mnemonics from secure storage, potentially causing irreversible loss of funds. Changes: - clearAll() now only clears SharedPreferences, preserving mnemonics - Add deleteAllMnemonics() for explicit mnemonic deletion - Add clearAllIncludingMnemonics() for intentional full reset - Update SubstrateService.logout() to use clearAllIncludingMnemonics() - Add documentation warnings about destructive operations - Add test verifying clearAll preserves secure storage The separation ensures: 1. Accidental clearAll() calls don't destroy wallet recovery phrases 2. Developers must explicitly choose to delete mnemonics 3. Users' funds are protected even if app state is cleared
SECURITY: Fixes high severity vulnerability where addresses from any SS58-compatible network were accepted, potentially allowing funds to be sent to wrong-network addresses. Changes: - ss58_to_account_id now validates the address prefix against the expected network prefix set via set_default_ss58_prefix - Returns error instead of panicking for invalid addresses - Returns descriptive error for prefix mismatch including both prefixes - Add tests for prefix validation (accept matching, reject different) - Regenerate Flutter Rust Bridge bindings The function now throws exceptions for: - Invalid SS58 addresses (malformed) - Addresses with wrong network prefix (e.g., Polkadot address when expecting Quantus network) This prevents users from accidentally sending funds to addresses on other networks by ensuring address validation at the SDK layer.
SECURITY: Fixes high severity vulnerability where RPC failover could fail to properly recover from endpoint failures, potentially leaving users unable to transact. Changes: - Add consecutiveFailures counter for exponential backoff on failures - Add cooldown mechanism (5min * failures, max 50min) before retrying - Add effectiveLatency getter that considers failure state - Cap penalty latency at 24h to prevent integer overflow (was 365 days) - Add server error detection (5xx) to trigger failover on HTTP errors - Sort endpoints before each request attempt - Fix bestEndpointUrl to sort before returning - Add recordSuccess/recordFailure methods for cleaner state management The improved failover now: 1. Properly penalizes failing endpoints with exponential backoff 2. Allows failed endpoints to recover after cooldown period 3. Fails over on HTTP 5xx errors, not just exceptions 4. Always returns the truly best endpoint based on current health
SECURITY: Fixes high severity vulnerability where development mnemonics with publicly known private keys could be used in production, leading to potential theft of funds. Changes: - Add DevMnemonicNotAllowedException for clearer error handling - Add allowDevMnemonics static flag (defaults to globalDebug = false) - Add validateMnemonic() for explicit mnemonic safety checks - keyPairAtIndex() now throws exception for dev mnemonics in release - deriveWormhole() also validates dev mnemonic usage - Add warning log when dev mnemonics are used in debug builds - Block wormhole derivation for dev mnemonics entirely The development mnemonics (//Crystal Alice, etc.) produce well-known keypairs that anyone can access. These are now blocked by default and only allowed when allowDevMnemonics is explicitly set to true (e.g., in test environments).
…3274, #93275) Fixes incorrect pallet indices in the documentation comment: - Balances pallet: was 3, corrected to 2 - ReversibleTransfers pallet: was 12, corrected to 13 The actual code was already using the correct indices. This only fixes the documentation to match reality.
…3272) SECURITY: Fixes critical bug where guardian pullAllFunds used the wrong pallet (pallet_recovery) instead of reversibleTransfers.recoverFunds. The previous implementation: - Used pallet_recovery calls (initiateRecovery, vouchRecovery, etc.) - Required independent recovery configuration that was never set up - Used non-atomic utility.batch which could partially fail - Would fail at runtime because high-security setup only configures the reversibleTransfers pallet, not pallet_recovery The fix: - Uses reversibleTransfers.recoverFunds as intended by the pallet - This is an atomic operation that either succeeds or fails completely - Cancels all pending transfers (with volume fees) and transfers remaining balance to the guardian - Removes incorrect recovery pallet fee from setup fee calculation This makes the guardian emergency rescue flow consistent with the high-security setup and allows guardians to actually recover funds from compromised accounts as documented.
The previous length-based paste detection (length delta > 1) failed when users performed select-all replacement pastes with similar-length content. For example, selecting '1000' and pasting '1,000' (delta=1) was misclassified as typing, causing the grouping separator to be converted to a decimal separator, resulting in '1.000' instead of '1000'. Replace length-delta heuristic with precise single-character insertion detection that verifies: - Text is exactly one character longer - Old selection was collapsed (no text selected) - Cursor moved exactly one position forward - Text before and after insertion point is unchanged This ensures the typing-mode logic (which accepts either . or , as decimal separator) only applies to true single-character typing. All other edits (paste, select-all replacement) now correctly strip grouping separators.
Add input validation to ExchangeRatesResult.fromJson to prevent cache poisoning attacks from malicious or compromised exchange rate responses: 1. Currency code validation: Only accept ISO 4217 format (3 uppercase ASCII letters). Rejects malformed keys that could cause issues downstream. 2. Rate value validation: Rates must be finite positive numbers within reasonable bounds (1e-6 to 1e7). Rejects negative, zero, infinite, NaN, and absurdly large/small values that could cause incorrect fiat conversions. 3. Expiry cap: Server-supplied expiry is capped at 7 days maximum, regardless of what the server returns. Prevents an attacker from setting a far-future expiry that would keep poisoned rates in cache indefinitely. Invalid entries are silently filtered rather than causing parse failures, ensuring partial data from a partially-compromised response is still usable while malicious entries are discarded. Add comprehensive test coverage for all validation scenarios.
Add validation to reject oversized signer arrays at the SDK boundary, preventing DoS attacks from malicious indexer responses. Changes: - Add boundedStringListFromJson() helper that enforces max list length - MultisigCreatedEvent.fromMultisigGraphql: validate signers against palletConstants.maxSigners (100) before parsing - MultisigCreationDraftFields.fromDraft: validate draft.signers length The pallet declares maxSigners=100, so larger arrays cannot represent valid on-chain state. Rejecting them early prevents: - Memory exhaustion from processing impossibly large signer arrays - Cache pollution with invalid multisig metadata - Downstream parsing of data that could never exist on-chain Add comprehensive test coverage for boundary conditions.
Network fees are unsigned balance values on-chain, but the SDK modeled them as signed BigInts without validation. A malicious indexer or RPC response could supply negative fees, causing: - Understated creation costs in preflight checks - Inflated available balance via negative pending outgoing - Incorrect totalCost calculations affecting balance providers Changes: - Add nonNegativeBigIntFromJson() helper for parsing unsigned values - MultisigCreatedEvent._networkFeeFromGraphql: validate fee >= 0 - MultisigCreationDraftFields.fromDraft: validate networkFee >= 0 - MultisigCreationEvent constructor: add assertions for both fees The chain enforces unsigned balances, so the SDK now matches this constraint at the parsing boundary rather than trusting external data. Add test coverage for negative fee rejection in all parsing paths.
Account operations (add, update, remove, removeWallet) perform read-modify-write on a single SharedPreferences JSON blob across await boundaries. Without synchronization, concurrent async flows could interleave and silently lose account data: 1. Flow A reads [X] 2. Flow B reads [X] 3. Flow A writes [X, A] 4. Flow B writes [X, B] ← A is lost Changes: - Add _AsyncMutex class to serialize async critical sections - Wrap all account persistence methods with _accountsMutex - Wrap all multisig persistence methods with _multisigMutex - Add createAndAddAccount() for atomic index allocation + persistence - Add comprehensive concurrency tests The mutex ensures operations execute in FIFO order, preventing data loss when createNewAccount+addAccount, ensureEncryptedAccountsForSoftwareWallets, discovery persistence, or MigrationService run concurrently. If two callers race with createNewAccount before addAccount, the second addAccount now fails with 'Account already exists' rather than silently overwriting. Use createAndAddAccount() for guaranteed atomic creation.
The discoverAccounts function had no validation on gapLimit and no maximum scan horizon, allowing: 1. Excessive CPU/memory from large gapLimit values 2. Infinite scanning from malicious indexer responses that mark all queried IDs as existing (keeping consecutiveMissing at 0) 3. Response manipulation by returning IDs that weren't queried Changes: - Add gapLimit validation: must be between 1 and 100 (default 20) - Add maxScanIndex (10,000) to guarantee termination regardless of indexer responses - Filter existingIds to only include actually-queried IDs, preventing response injection attacks - Cap batch size to not exceed maxScanIndex - Expose constants publicly so callers can validate input The max scan index of 10,000 accounts is far beyond realistic HD wallet usage while still providing a hard upper bound on resource consumption. Add test coverage for validation constants.
HTTP requests through RedundantEndpointService (GraphQL, RPC) had no timeout, allowing stalled connections to hang futures indefinitely. This blocked account discovery, wallet import, and other critical flows. Changes: - Add defaultTimeout (30s) and maxTimeout (5 minutes) constants - Wrap all HTTP tasks with Future.timeout() in _executeTask - Add EndpointTimeoutException for clear error identification - Treat TimeoutException and EndpointTimeoutException as reachability errors so they trigger endpoint failover - Add optional timeout parameter to get(), post(), and rpcTask() - Clamp caller-supplied timeouts to maxTimeout to prevent abuse The retry wrapper now advances to the next endpoint after a timeout, enabling recovery when a backend stalls. The 30-second default is long enough for legitimate slow responses while preventing indefinite hangs from malicious or broken backends. Add test coverage for timeout configuration and exception handling.
The pagination in ChainHistoryService._pageFromEvents computed hasMore from the raw row count but returned only successfully parsed items. fetchAllTransactionTypes then advanced nextOtherOffset by the parsed item count (transfers.length) instead of the raw rows consumed. This caused cursor drift when rows were skipped (null-parsed): - Re-fetching already-shown transfers - Infinite loop when all rows in a window are skipped (offset stays fixed while hasMore is true, causing repeated identical requests) The bug was exploitable: an attacker generating many ae-ms-/ae-multisig- account_event rows referencing a victim account could trigger sustained network and battery drain in the history feature. Changes: - Add rawRowsConsumed field to _Page and OtherTransfersResult - Track raw rows consumed in _pageFromEvents (including skipped rows) - Advance nextOtherOffset and nextScheduledOffset by rawRowsConsumed - Add tests verifying rawRowsConsumed can differ from items.length The offset cursor now correctly advances through the underlying data regardless of how many rows parse to null.
HighSecurityService.getHighSecurityConfig threw ArgumentError when encountering a BlockNumber delay variant, but the runtime's BlockNumberOrTimestamp type legitimately includes both variants. The SDK's own ReversibleTransfersService.delayFromBlocks() helper and setHighSecurity() both accept block-number delays, and other clients or direct runtime calls can create them. This created a crash vector: an attacker could configure a valid high-security account with a block-number delay and set the victim as guardian. When the victim's guardian-management UI queried getHighSecurityConfig for that account, it would throw, breaking the entire high-security status view. Changes: - Add SafeguardDelay sealed class with TimestampDelay and BlockNumberDelay subtypes to model both runtime variants - Update HighSecurityData to use SafeguardDelay instead of Duration - Add safeguardWindow getter (returns Duration? - null for blocks) - Add safeguardBlocks getter (returns int? - null for timestamps) - Preserve backward compatibility via copyWith() and withDuration() - Update getHighSecurityConfig to map both delay variants correctly - Add comprehensive tests for the new sealed class hierarchy UI code can now use pattern matching on data.delay to handle both types appropriately, or use the convenience getters for simple cases.
HumanReadableChecksumService.getHumanReadableName accepted arbitrary strings without validation, creating two memory exhaustion vectors: 1. Oversized addresses: No length limit meant huge strings were copied into the isolate work queue and potentially cached 2. Unbounded cache: _checkPhraseCache grew without limit, allowing an attacker to exhaust memory by querying many unique addresses Changes: - Add maxAddressLength (64) constant - SS58 addresses are 47-50 chars - Reject empty or oversized addresses immediately (before isolate work) - Replace unbounded Map with _LruCache class (capacity: maxCacheSize=1000) - LRU cache evicts least-recently-used entries when full - Only cache successful (non-empty) results to avoid poisoning cache - Add defensive check for empty words in uppercase transformation The early rejection of invalid addresses prevents both isolate queue flooding and cache pollution from attacker-supplied data. The LRU cache ensures bounded memory even under sustained unique-address load.
…ccounts MigrationService had three critical flaws: 1. Only fetched mnemonic for baseWalletIndex=0, ignoring accounts from other wallets entirely 2. Hardcoded walletIndex=0 in rebuilt accounts, losing the original wallet association 3. Used keyPairAtIndex for all accounts, including encrypted/wormhole accounts that require a different derivation path This caused accounts not in wallet 0 to be derived from the wrong seed, producing addresses that don't correspond to their on-chain funds. After clearOldAccounts(), the original data was permanently deleted, and two accounts sharing an index across different wallets would collapse to identical addresses. Changes: - Add MigrationResult sealed class with MigrationSuccess/MigrationFailure - getMigrationData now fetches mnemonic per account's walletIndex - Cache mnemonics to avoid repeated secure storage reads - Detect encrypted accounts (by type or encryptedAccountIndex) and use deriveWormholeKeyPair instead of keyPairAtIndex - Preserve walletIndex and accountType in migrated accounts - Return failures list from performMigration for UI feedback - Only clear old accounts if ALL migrations succeeded (prevent data loss) - Update mobile app to use new API and display failure counts - Add backward-compatible MigrationAccountData.fromSuccess factory - Add comprehensive tests for wallet index preservation The migration now correctly handles multi-wallet setups and encrypted accounts, with graceful degradation when mnemonics are missing.
…tors (#93265) Amount parsing accepted malformed or cross-locale numeric strings because grouping separators were stripped before their placement was validated, and wire amount parsing guessed decimal vs grouping from ambiguous input. Problems: 1. Paste flow: '1,5' in US locale was collapsed to '15' instead of rejected 2. LocaleNumberConfig.normalize(): '9,9,9' became '999' (invalid grouping) 3. parseWireAmount(): '1,000' was guessed as 1.0 instead of being ambiguous These could cause 10x-1000x scale errors in transfers from pasted amounts, payment URLs, or POS QR codes. Changes to LocaleNumberConfig: - Add _validateGrouping() to enforce proper thousands grouping structure - First group: 1-3 digits, subsequent groups: exactly 3 digits - Reject grouping separators in fractional part - normalize() now throws InvalidNumberInputException for invalid grouping Changes to DecimalInputFilter: - Stop stripping grouping separators before validation in paste mode - Let normalize() validate grouping structure and reject malformed input - Catch InvalidNumberInputException and return oldValue (reject paste) - Strip grouping separators AFTER validation for display Changes to NumberFormattingService: - Add _singleSeparatorWireLocale() to detect ambiguous inputs - Return null for single-separator amounts with exactly 3 digits after (e.g., '1,000' or '1.000' - could be 1 or 1000) - Multiple occurrences of same separator = definitely grouping - Both separators present = unambiguous (last one is decimal) Tests verify all PoC cases now properly reject or return null.
…ir (#93266) Account.getKeypair() now: - Rejects keystone/external accounts (no local keys) - Rejects encrypted accounts (use ZK proofs, not signatures) - Validates derived address matches stored accountId SubstrateService.getExtrinsicPayload() now uses account.getKeypair() instead of directly calling keyPairAtIndex, ensuring all validation is applied before signing. This prevents: - Silent mismatch between displayed address and signing key - Using wrong derivation path for encrypted accounts - Signing with locally-derived key for hardware wallet accounts - Accepting tampered Account records that could redirect transactions
…unt (#93270) MultisigProposal now verifies that call_raw bytes match the indexer- provided recipient and amount to prevent spoofed indexer data from misleading signers about what action they're approving. Added: - CallVerificationStatus enum with verified/noCallRaw/decodeError/ notATransfer/recipientMismatch/amountMismatch states - callRaw, verificationStatus, verificationError fields - _verifyCallRaw() that decodes call_raw and compares to indexer data - isVerified getter (true when verified or notATransfer) - hasVerificationMismatch getter (true for recipient/amount mismatches) UI should check isVerified before allowing approval/execution, and block when hasVerificationMismatch is true (potential indexer spoofing).
…ed state (#93282) WormholeAddressManager: - Added generation counter incremented on clear() - initialize() checks generation after await to detect stale calls - Prevents secret material from being restored after clear() ConnectivityService: - Added _disposed flag checked after await and in callbacks - Prevents emitting to closed StreamController after dispose()
…172) RedundantEndpointService.get() and post() now use _buildSafeUri() which: - Rejects paths containing @ (userinfo syntax) - Rejects protocol-relative paths (//) - Rejects absolute URLs (://) - Verifies resolved host matches original base URL - Uses Uri.resolve() for safe path resolution This prevents attackers from redirecting requests to arbitrary hosts by supplying malicious paths like '@attacker.example/path'.
RecentAddressesService now enforces maxAddressLength=256: - addAddress() returns false for oversized inputs - getAddresses() filters out any previously stored oversized entries This prevents storage abuse from attacker-controlled address strings passed via QR codes, deep links, or other untrusted sources.
…ble-hashing (#93189) getProxyRecoveredAccount() was incorrectly using crypto.toAccountId() to encode the AccountId32 returned by recovery.proxy storage. Since toAccountId() Poseidon-hashes its input, this double-hashed the already-derived account ID, producing a wrong address. Changed to use AddressExtension.ss58AddressFromBytes() which directly encodes the AccountId32 bytes to SS58 without additional hashing. This matches the pattern used by other services (reversible_transfers, high_security) and the documented convention in AddressExtension.
…unts (#93193) The test code that injected a hard-coded address into the guardian's intercepted accounts list was left in production code, allowing: - Fake accounts to appear as entrusted by a guardian - Guardians to be tricked into signing recovery transactions for accounts they don't actually protect Removed the test code that appended 'qzkaf6wMj...' to non-empty results.
…199) Encrypted (wormhole) accounts are designed to be unlinkable to the user's identity for privacy. Registering them with the notification service would allow Senoti operators or anyone with backend access to deanonymize wormhole activity by correlating it with the user's transparent wallet addresses. Changed registerDevice() to filter out AccountType.encrypted accounts before sending the address list to Senoti.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a1a5b0b. Configure here.
When every account is a MigrationFailure (e.g., missing mnemonic), successCount is zero so the Migrate button is disabled. Previously the sheet was non-dismissible with no escape route since Try later only appeared after an upload error. Now shows a Skip button when successCount == 0, allowing users with old accounts but no usable wallet data to dismiss the modal.
Previously, _performMigration and _tryLater set _needsMigration to false even when performMigration returned failures. This hid that migration was incomplete until the next cold start. Changes: - Add _hasMigrationFailures state to track partial failures - Show snackbar notification when some accounts fail to migrate - Properly capture failures from performMigration in _tryLater Old accounts are intentionally preserved when failures occur (the MigrationService already handles this), and migration will retry on next app launch.
call_raw verification previously returned notATransfer for non-Balances calls (e.g., utility::batch, System::remark), and isVerified treated that as safe. However, if the indexer also claimed a recipient/amount, this allowed spoofed display data to appear verified. Changes: - Add CallVerificationStatus.notATransferButIndexerClaimsTransfer - Return this status when call_raw doesn't decode to a transfer BUT the indexer provides recipient/amount (suspicious mismatch) - Include this status in hasVerificationMismatch check - notATransfer now only means 'verified' when indexer also has no recipient/amount to display This prevents wrapped or non-balance calls from showing as verified when the displayed recipient/amount don't match the actual call.
The dev mnemonic guard was using print() which runs in release builds, unlike the project's logging policy. Changed to debugPrint() which is stripped from release builds.
Resolve conflict in settings_service.dart by keeping both: - Mutex wrapper for race condition fix - Wallet origin and recovery phrase viewed key cleanup
n13
requested changes
Jul 2, 2026
n13
left a comment
n13
left a comment
Collaborator
There was a problem hiding this comment.
#93270 - Multisig approvals now verify call_raw matches indexer-provided recipient/amount before signing
#93266 - Account signing validates account type and derived address matches accountId
#93267 - Dev mnemonics blocked in release builds
None of these are real bugs
I don't think it makes sense to do this
The entire app and all flows have to be re-tested after this. Why were 4,000 lines of code added? What do they do?
n13
requested changes
Jul 2, 2026
n13
left a comment
n13
left a comment
Collaborator
There was a problem hiding this comment.
Review
Verdict: Request changes.
This PR bundles a handful of genuinely valuable bug fixes with a large amount of defensive hardening whose security impact is overstated, ~600 lines of generated-binding churn, and a red CI. The real fixes are worth landing, but the PR needs trimming and the blocking issues below fixed first.
The security framing is overstated
#93270 (call_raw verification) — the headline critical — does not hold up:
call_raw,transfer_to, andtransfer_amountall come from the same indexer record. A compromised indexer serves a self-consistent lie (fakecall_rawmatching the fake display fields) and verification passes. This is a self-consistency check within a single untrusted source, not a trust boundary. It only catches an attacker who tampers the display columns but forgetscall_raw.- Nothing consumes it.
isVerified/hasVerificationMismatchare referenced nowhere outside the model and its tests — no approve/execute UI checks them. As merged, this changes no behavior at all. - The approve extrinsic commits only to
proposalId(the PR's own test demonstrates this), so the meaningful fix is to verify the proposal against on-chain storage via RPC before signing, and have the approval UI block on that. Either do that, or don't call this a critical fix.
By contrast, the discoverForUser check (skip records whose address ≠ locally computed hash(signers, threshold, nonce)) is sound — the address cryptographically commits to the signer set. Good fix, but see blocking issue 6.
Similar pattern elsewhere: the SSRF fix guards paths that no call site passes untrusted data into (all callers use post(body:) with fixed/empty paths); the exchange-rate bounds still accept wrong-but-plausible rates from a compromised rate API; the DoS bounds (address length, LRU cache, gap limit) are local-resource hygiene. Fine to have, but low severity.
The fixes actually worth landing are mostly correctness, not security — and they're good:
- Migration fix (wallet-index/encrypted derivation): the old code derived accounts from the wrong seed and then cleared the old data — permanent fund loss. Most impactful fix in the PR.
- #93272 guardian recovery used a pallet that was never configured — the rescue path could not work.
- #93189 recovery proxy double-hashing (wrong address returned).
- #93193 leftover hard-coded test address.
clearAll()no longer silently deleting mnemonics; pagination cursor drift; #93199 Senoti wormhole privacy; #93263 SS58 prefix validation (panic→error is also a good change).
Blocking issues
- CI Analyze is red (
melos exec flutter analyze, warnings are fatal). mobile-app: 3×invalid_return_type_for_catch_error, 1× unused_hasMigrationFailures. quantus_sdk: unusedwormholeKeyPairlocal, unused imports/locals in new tests. RecentAddressesService.addAddressreturn type change (Future<void>→Future<bool>) breaks the three.catchError((e) => debugPrint(...))call sites — the handler now must returnFutureOr<bool>; if the future ever fails, the handler itself throws a runtimeTypeErrorinsideunawaited(...). Fix the call sites or keep the void return.Account.getKeypair()encrypted branch is dead code: it derives the wormhole keypair, then unconditionally throws. The exception message also claims encrypted accounts can derive keypairs, contradicting the behavior. Delete the derivation and fix the message/doc comment._hasMigrationFailuresis written but never read — either surface it in UI or drop it.- Wire-amount ambiguity check is over-broad:
parseWireAmount('1234.567')returns null, but a grouping reading is impossible there (first group would be 4 digits). Only treat single-separator + 3 trailing digits as ambiguous when the integer part is 1–3 digits. As written, this will reject legitimate payment-URL/QR amounts. discoverForUsersilentlycontinues on mismatch/parse failure with no logging — a legit multisig disappearing from a user's list with zero diagnostics will be unsupportable. Also, when the indexer omitsnonce, the code assumesdefaultMultisigNonce; a legit multisig created with a different nonce would be silently hidden. Log every skip with the reason._buildSafeUrichanges URL semantics: empty path now produces a trailing slash (…/v1/graphql/— I verified the production Hasura tolerates this, 200), and a leading-/path now replaces the base path instead of appending (old:$url$path). Latent breakage for any future endpoint with a path prefix; the double-negative validation logic should also be simplified — it's hard to convince yourself it's airtight.TransactionInfo's deprecatedreversibleTimeframeconstructor parameter is accepted and silently dropped. Remove it or make it populatereversibleDelay.- Diff noise:
frb_generated.*reformatting (~600 lines) andpubspec.lockdowngrades (meta 1.18.0→1.17.0, test_api 0.7.11→0.7.10) suggest the bindings were regenerated with a different Flutter/FRB toolchain than the repo's. Regenerate with the pinned toolchain so the diff shows only the real change (thess58_to_account_iderror codec). - Minor: PR description says dev mnemonics are "blocked in release builds", but
allowDevMnemonicsdefaults toAppConstants.globalDebug, which isconst false— they're blocked everywhere unless code is edited. Fine as behavior, but say what it does.
Verification performed
flutter testin quantus_sdk: 220 passed; 3 test files (account_extension_test,generate_keys_test,recovery_proxy_encoding_test) require the native Rust dylib and can't run from a clean checkout —cargo buildfails to resolve the yankedcore2 ^0.3(via KeystoneHQlibflate) becauserust/Cargo.lockisn't committed. Worth fixing separately; note CI isn't running these tests either.flutter analyze: cold-wallet-app clean; mobile-app and quantus_sdk warnings as listed above (matches the failing CI job).- Rust changes reviewed: multisig canonicalization/threshold validation and SS58 prefix validation look correct.
Recommendation
Split out and land the genuine bug fixes (migration, guardian recovery pallet, proxy double-hash, test-address removal, clearAll, pagination) in a slim PR. For #93270, either make verification meaningful — check against on-chain proposal state and wire isVerified/hasVerificationMismatch into the approve/execute UI — or drop the 200 lines of unused machinery. Revert the generated-file and lockfile churn.
Collaborator
|
Closing in favor of #552, which lands only the correctness fixes from this PR (migration derivation, guardian recovery, recovery proxy encoding, test address removal, clearAll mnemonic handling, pagination cursor, Senoti privacy, SS58 validation). See the review above for details on the remaining findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security review remediation for quantus_sdk addressing 20 findings from the V12 security audit.
Changes
Critical/High Severity
call_rawmatches indexer-provided recipient/amount before signingMedium Severity
RedundantEndpointServicegetInterceptedAccountsOther Fixes
RedundantEndpointServiceTesting
All fixes include unit tests verifying the security boundary. Run with: