fix: replace Math.random() with crypto.getRandomValues() (WAPI-1127)#73
Open
chakra-guy wants to merge 5 commits intocyfrin/wapi-1118from
Open
fix: replace Math.random() with crypto.getRandomValues() (WAPI-1127)#73chakra-guy wants to merge 5 commits intocyfrin/wapi-1118from
chakra-guy wants to merge 5 commits intocyfrin/wapi-1118from
Conversation
Math.random() is not cryptographically secure and should never be used for security-sensitive values. This replaces all usages with crypto.getRandomValues() which is available cross-platform via globalThis.crypto (Node 19+, all modern browsers, React Native). The critical fix is in OTP generation for the untrusted connection flow. Demo app log entry IDs are also updated for consistency.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Use rejection sampling in _uniformRandom() to produce perfectly uniform random values, eliminating the modulo bias flagged in PR review. Fix CI: add globalThis.crypto polyfill for Node 18 test environment, and reorder changelog sections to match Keep a Changelog spec (Changed before Fixed).
Revert Math.random() replacements in demo app log entry IDs - these are not security-sensitive (just React keys) and don't need CSPRNG. Simplify OTP generation back to direct modulo. The modulo bias is ~0.004% which is negligible for a time-limited 6-digit OTP. Keep the changelog section ordering fix and Node 18 test polyfill.
globalThis.crypto is not available by default in Node.js <19.
Fall back to require("node:crypto").randomFillSync() when the
Web Crypto API is not present.
adonesky1
reviewed
Feb 25, 2026
Comment on lines
+9
to
+18
| // Node 18 doesn't expose globalThis.crypto; polyfill it for tests. | ||
| if (!globalThis.crypto) { | ||
| globalThis.crypto = { | ||
| getRandomValues<T extends ArrayBufferView | null>(array: T): T { | ||
| if (array) randomFillSync(array as any); | ||
| return array; | ||
| }, | ||
| } as Crypto; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is reminding me we should create a ticket to set minimum node version to 20.x and update CI runners accordingly
There was a problem hiding this comment.
globalThis.crypto was added in Node 19. Every production runtime for this library already has it — browsers, React Native (Hermes/JSC), and Node 20+ LTS. Node 18 hit EOL April 2025, and eciesjs 0.4.16 (which this project depends on) already dropped Node 18 support. The fallback branch will never execute in production — it only exists to satisfy CI running on an outdated Node version.
There was a problem hiding this comment.
adonesky1
approved these changes
Feb 25, 2026
4 tasks
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
Math.random()calls withcrypto.getRandomValues()for cryptographically secure randomnessMath.randomfrom the codebase)Background
The Cyfrin audit flagged
Math.random()usage for OTP generation.Math.random()uses a non-cryptographic PRNG and its output is predictable, making OTP values potentially guessable.crypto.getRandomValues()is available cross-platform viaglobalThis.crypto(Node 19+, all modern browsers, React Native).Changes
packages/wallet-client/src/handlers/untrusted-connection-handler.ts: OTP generation usesUint32Array+crypto.getRandomValues()apps/web-demo/src/components/UntrustedDemo.tsx: Log entry IDsapps/web-demo/src/components/TrustedDemo.tsx: Log entry IDsapps/web-demo/src/components/MetaMaskMobileDemo.tsx: Log entry IDsapps/rn-demo/context/WalletContext.tsx: Log entry IDsTest plan
yarn buildpassesyarn test:unitpasses (68/68 tests)yarn lintpasses (no new warnings)Math.random()calls remain in source filesNote
Medium Risk
Touches security-sensitive handshake OTP generation; while the change is small, any regression could break untrusted pairing or alter OTP distribution, especially across Node/runtime environments.
Overview
Untrusted connection OTP generation now uses a cryptographically secure RNG via
globalThis.crypto.getRandomValues()(with a Node <19node:crypto.randomFillSyncfallback) instead ofMath.random().Unit tests are updated to run on Node 18 by polyfilling
globalThis.crypto.getRandomValues, and the wallet-client changelog records the fix under Unreleased.Written by Cursor Bugbot for commit 89b9097. This will update automatically on new commits. Configure here.