| … | … | @@ -0,0 +1,343 @@ |
| 1 | # Code Review Findings: hyper-wallet-desktop |
| 2 | |
| 3 | **Date:** 2026-01-29 |
| 4 | **Reviewer:** Claude Sonnet 4.5 |
| 5 | **Focus Areas:** QR code generation for USDT, address selection/display |
| 6 | |
| 7 | --- |
| 8 | |
| 9 | ## Critical Bugs Found |
| 10 | |
| 11 | ### Bug #1: QR Code Shows Wrong Address for Token Chains (USDT) |
| 12 | |
| 13 | **Location:** `wallet-server.cjs:666-689` (GET /api/qrcode/) |
| 14 | |
| 15 | **Severity:** HIGH - Users may receive funds to wrong address |
| 16 | |
| 17 | **Description:** |
| 18 | When generating a QR code for token chains like `ethereum-usdt`, the code incorrectly retrieves the current address for the base chain (`ethereum`) instead of the token chain (`ethereum-usdt`). |
| 19 | |
| 20 | **Root Cause:** |
| 21 | ```javascript |
| 22 | // Line 679: Gets base chain (ethereum-usdt -> ethereum) |
| 23 | const chainName = getBaseChain(chainParam) |
| 24 | |
| 25 | // Line 684: BUG - Uses base chain to get current address |
| 26 | const currentAddr = await getCurrentAddress(chainName) // Gets 'ethereum' current |
| 27 | ``` |
| 28 | |
| 29 | **Issue:** |
| 30 | - Addresses are stored separately per chain in the database (including `ethereum`, `ethereum-usdt`, etc.) |
| 31 | - When you change the current address for USDT via the dropdown selector, it updates the current address for `ethereum-usdt` in storage |
| 32 | - But when generating the QR code for USDT, it retrieves the current address for `ethereum` |
| 33 | - This creates a mismatch: the QR shows Ethereum's current address, not USDT's current address |
| 34 | |
| 35 | **User Impact:** |
| 36 | 1. User selects USDT in the wallet |
| 37 | 2. User changes the receive address using the dropdown (e.g., from Address 0 to Address 1) |
| 38 | 3. The UI shows Address 1 is selected |
| 39 | 4. The QR code still shows Address 0 (because it's using Ethereum's current address) |
| 40 | 5. User receives USDT to the wrong address |
| 41 | |
| 42 | **Fix Required:** |
| 43 | ```javascript |
| 44 | // wallet-server.cjs line 666-689 |
| 45 | 'GET /api/qrcode/': requireAuth(async (req, res, chain) => { |
| 46 | try { |
| 47 | const walletMnemonic = loadWalletMnemonicCached() |
| 48 | if (!walletMnemonic) { |
| 49 | return sendError(res, 'Wallet not initialized', 400) |
| 50 | } |
| 51 | |
| 52 | const network = chain.startsWith('testnet') ? 'testnet' : 'mainnet' |
| 53 | const chainParam = chain.replace(/^(testnet|mainnet)-/, '') |
| 54 | |
| 55 | // For tokens, get the base chain address (ethereum-usdt -> ethereum) |
| 56 | const chainName = getBaseChain(chainParam) |
| 57 | |
| 58 | // Get current address index from storage (defaults to 0) |
| 59 | const identity = await loadIdentity() |
| 60 | await initializeStorage(identity) |
| 61 | |
| 62 | // FIX: Use original chainParam for storage lookup, not chainName |
| 63 | const currentAddr = await getCurrentAddress(chainParam) // Changed from chainName |
| 64 | const accountIndex = currentAddr ? currentAddr.index : 0 |
| 65 | |
| 66 | // Use chainName for WDK account (correct - USDT uses ETH addresses) |
| 67 | const { wdk } = await initializeWDK(walletMnemonic, { testnet: network === 'testnet' }) |
| 68 | const account = await wdk.getAccount(chainName, accountIndex) // This is correct |
| 69 | const address = await account.getAddress() |
| 70 | |
| 71 | // ... rest of function |
| 72 | } |
| 73 | }) |
| 74 | ``` |
| 75 | |
| 76 | --- |
| 77 | |
| 78 | ### Bug #2: Receive Modal Shows Wrong Address for Token Chains |
| 79 | |
| 80 | **Location:** `wallet-server.cjs:443-507` (GET /api/address/) |
| 81 | |
| 82 | **Severity:** HIGH - Same issue as Bug #1 |
| 83 | |
| 84 | **Description:** |
| 85 | The `/api/address/:chain` endpoint has the exact same bug - it uses the base chain to retrieve the current address instead of the original chain parameter. |
| 86 | |
| 87 | **Code:** |
| 88 | ```javascript |
| 89 | // Line 456: Gets base chain |
| 90 | const chainName = getBaseChain(chainParam) |
| 91 | |
| 92 | // Line 461: BUG - Uses base chain to get current address |
| 93 | const currentAddr = await getCurrentAddress(chainName) // Should use chainParam |
| 94 | ``` |
| 95 | |
| 96 | **Fix Required:** |
| 97 | ```javascript |
| 98 | // wallet-server.cjs line 443-507 |
| 99 | 'GET /api/address/': requireAuth(async (req, res, chain) => { |
| 100 | try { |
| 101 | const walletMnemonic = loadWalletMnemonicCached() |
| 102 | if (!walletMnemonic) { |
| 103 | return sendError(res, 'Wallet not initialized', 400) |
| 104 | } |
| 105 | |
| 106 | const network = chain.startsWith('testnet') ? 'testnet' : 'mainnet' |
| 107 | const chainParam = chain.replace(/^(testnet|mainnet)-/, '') |
| 108 | |
| 109 | // For tokens, get the base chain address (ethereum-usdt -> ethereum) |
| 110 | const chainName = getBaseChain(chainParam) |
| 111 | |
| 112 | // Check storage for current address first |
| 113 | const identity = await loadIdentity() |
| 114 | await initializeStorage(identity) |
| 115 | |
| 116 | // FIX: Use original chainParam for storage lookup |
| 117 | const currentAddr = await getCurrentAddress(chainParam) // Changed from chainName |
| 118 | |
| 119 | if (currentAddr) { |
| 120 | return sendJSON(res, { |
| 121 | chain: chainName, // Return base chain name for display |
| 122 | network, |
| 123 | address: currentAddr.address, |
| 124 | index: currentAddr.index, |
| 125 | label: currentAddr.label || '' |
| 126 | }) |
| 127 | } |
| 128 | |
| 129 | // ... rest of function (default address generation) |
| 130 | } |
| 131 | }) |
| 132 | ``` |
| 133 | |
| 134 | --- |
| 135 | |
| 136 | ### Bug #3: Balance Endpoint May Have Same Issue |
| 137 | |
| 138 | **Location:** `wallet-server.cjs:400-436` (GET /api/balance/) |
| 139 | |
| 140 | **Severity:** MEDIUM - May show balance for wrong address |
| 141 | |
| 142 | **Description:** |
| 143 | The balance endpoint for token chains also uses `getBaseChain()` to get the current address: |
| 144 | |
| 145 | ```javascript |
| 146 | // Line 403: Gets base chain |
| 147 | const baseChain = getBaseChain(chainParam) |
| 148 | |
| 149 | // Line 405: Uses base chain for current address |
| 150 | const currentAddr = await getCurrentAddress(baseChain) // Should use chainParam |
| 151 | ``` |
| 152 | |
| 153 | **Verification Needed:** |
| 154 | This may be intentional if token balances are queried at the account level (not address level), but it should be verified. |
| 155 | |
| 156 | **Fix (if needed):** |
| 157 | ```javascript |
| 158 | // Use chainParam instead of baseChain for getCurrentAddress() |
| 159 | const currentAddr = await getCurrentAddress(chainParam) // Changed from baseChain |
| 160 | ``` |
| 161 | |
| 162 | --- |
| 163 | |
| 164 | ## Technical Analysis |
| 165 | |
| 166 | ### Address Storage Architecture |
| 167 | |
| 168 | The wallet stores addresses separately per chain: |
| 169 | - Storage keys: `wallet:address:${chain}:${network}:${index}` |
| 170 | - Chains are stored separately: `ethereum`, `ethereum-usdt`, `bitcoin`, etc. |
| 171 | - Each chain tracks its own "current" address via the `isCurrent` flag |
| 172 | |
| 173 | ### How getCurrentAddress Works |
| 174 | |
| 175 | ```javascript |
| 176 | // lib/wallet-storage.cjs:423-430 |
| 177 | async function getCurrentAddress(chain) { |
| 178 | const addresses = await getAddresses(chain) |
| 179 | if (addresses.length === 0) return null |
| 180 | |
| 181 | // Find address marked as current, or return latest |
| 182 | const current = addresses.find(a => a.isCurrent) |
| 183 | return current || addresses[addresses.length - 1] |
| 184 | } |
| 185 | ``` |
| 186 | |
| 187 | This function is **chain-specific** - it only looks at addresses for the given chain. |
| 188 | |
| 189 | ### Why getBaseChain Exists |
| 190 | |
| 191 | The `getBaseChain()` function exists because: |
| 192 | - USDT is an ERC-20 token on Ethereum |
| 193 | - USDT uses the same address format as Ethereum (0x...) |
| 194 | - WDK operations need to use the base chain (`ethereum`) to generate addresses |
| 195 | - But storage operations should use the full chain name (`ethereum-usdt`) |
| 196 | |
| 197 | ### The Bug Pattern |
| 198 | |
| 199 | **Correct pattern:** |
| 200 | ```javascript |
| 201 | const chainName = getBaseChain(chainParam) // For WDK operations |
| 202 | const currentAddr = await getCurrentAddress(chainParam) // For storage operations |
| 203 | const account = await wdk.getAccount(chainName, accountIndex) // WDK uses base chain |
| 204 | ``` |
| 205 | |
| 206 | **Current (buggy) pattern:** |
| 207 | ```javascript |
| 208 | const chainName = getBaseChain(chainParam) |
| 209 | const currentAddr = await getCurrentAddress(chainName) // BUG: Should use chainParam |
| 210 | const account = await wdk.getAccount(chainName, accountIndex) |
| 211 | ``` |
| 212 | |
| 213 | --- |
| 214 | |
| 215 | ## Affected User Flows |
| 216 | |
| 217 | ### Scenario 1: Switching Between Ethereum and USDT |
| 218 | 1. User has Address 0 and Address 1 for both Ethereum and USDT |
| 219 | 2. User selects Ethereum, changes to Address 1 (current for ethereum = Address 1) |
| 220 | 3. User switches to USDT, changes to Address 0 (current for ethereum-usdt = Address 0) |
| 221 | 4. QR code shows Address 1 (ethereum's current) instead of Address 0 (ethereum-usdt's current) |
| 222 | |
| 223 | ### Scenario 2: Receive Modal |
| 224 | 1. User opens "Receive USDT" modal |
| 225 | 2. Address dropdown shows correct addresses for USDT with correct "current" marker |
| 226 | 3. But the displayed address in the modal is from Ethereum's current address |
| 227 | 4. User confusion: dropdown says Address 0 is current, but modal shows Address 1 |
| 228 | |
| 229 | --- |
| 230 | |
| 231 | ## Recommended Fixes |
| 232 | |
| 233 | ### Priority 1: Fix /api/qrcode/ and /api/address/ endpoints |
| 234 | |
| 235 | **File:** `wallet-server.cjs` |
| 236 | |
| 237 | **Changes:** |
| 238 | 1. Line 461: Change `getCurrentAddress(chainName)` to `getCurrentAddress(chainParam)` |
| 239 | 2. Line 684: Change `getCurrentAddress(chainName)` to `getCurrentAddress(chainParam)` |
| 240 | |
| 241 | ### Priority 2: Verify /api/balance/ endpoint |
| 242 | |
| 243 | **File:** `wallet-server.cjs` |
| 244 | |
| 245 | **Investigation needed:** |
| 246 | - Does token balance query need per-address lookup or account-level? |
| 247 | - If per-address, change line 405: `getCurrentAddress(baseChain)` to `getCurrentAddress(chainParam)` |
| 248 | |
| 249 | ### Priority 3: Add storage layer validation |
| 250 | |
| 251 | **File:** `lib/wallet-storage.cjs` |
| 252 | |
| 253 | **Enhancement:** |
| 254 | Add validation in `getCurrentAddress()` to ensure it handles token chains correctly: |
| 255 | |
| 256 | ```javascript |
| 257 | async function getCurrentAddress(chain) { |
| 258 | // Validate chain parameter is the full chain name (not base chain) |
| 259 | if (chain !== chain.toLowerCase()) { |
| 260 | console.warn(`getCurrentAddress called with non-lowercase chain: ${chain}`) |
| 261 | chain = chain.toLowerCase() |
| 262 | } |
| 263 | |
| 264 | const addresses = await getAddresses(chain) |
| 265 | if (addresses.length === 0) return null |
| 266 | |
| 267 | const current = addresses.find(a => a.isCurrent) |
| 268 | return current || addresses[addresses.length - 1] |
| 269 | } |
| 270 | ``` |
| 271 | |
| 272 | --- |
| 273 | |
| 274 | ## Testing Recommendations |
| 275 | |
| 276 | ### Test Case 1: USDT Address Selection |
| 277 | 1. Create wallet with testnet USDT |
| 278 | 2. Derive 2 addresses for USDT |
| 279 | 3. Set Address 0 as current |
| 280 | 4. Verify QR code shows Address 0 |
| 281 | 5. Change to Address 1 via dropdown |
| 282 | 6. Verify QR code now shows Address 1 (currently fails) |
| 283 | |
| 284 | ### Test Case 2: Cross-Chain Independence |
| 285 | 1. Create addresses for both Ethereum and USDT |
| 286 | 2. Set Ethereum current = Address 1 |
| 287 | 3. Set USDT current = Address 0 |
| 288 | 4. Switch to Ethereum view → verify QR shows Address 1 |
| 289 | 5. Switch to USDT view → verify QR shows Address 0 (currently fails - shows Address 1) |
| 290 | |
| 291 | ### Test Case 3: Receive Modal Consistency |
| 292 | 1. Open "Receive USDT" modal |
| 293 | 2. Verify address in input field matches address marked as current in dropdown |
| 294 | 3. Change address in dropdown |
| 295 | 4. Verify address in input field updates accordingly (currently fails) |
| 296 | |
| 297 | --- |
| 298 | |
| 299 | ## Additional Observations |
| 300 | |
| 301 | ### Storage Keys |
| 302 | |
| 303 | The address storage uses chain-specific keys: |
| 304 | ```javascript |
| 305 | // lib/wallet-storage.cjs:358 |
| 306 | const key = `wallet:address:${chain}:${network}:${index}` |
| 307 | ``` |
| 308 | |
| 309 | This confirms that `ethereum` and `ethereum-usdt` addresses are stored separately. |
| 310 | |
| 311 | ### WDK Integration |
| 312 | |
| 313 | The WDK integration correctly handles base chain mapping: |
| 314 | ```javascript |
| 315 | // lib/wdk-integration.cjs:171-177 |
| 316 | function getBaseChain(chain) { |
| 317 | if (!isTokenChain(chain)) { |
| 318 | return chain |
| 319 | } |
| 320 | // Extract base chain from 'ethereum-usdt' -> 'ethereum' |
| 321 | return chain.split('-')[0] |
| 322 | } |
| 323 | ``` |
| 324 | |
| 325 | This is working as designed - the bug is in how this is used. |
| 326 | |
| 327 | --- |
| 328 | |
| 329 | ## Conclusion |
| 330 | |
| 331 | **Summary:** |
| 332 | Two critical bugs cause QR codes and receive addresses to show the wrong address when using token chains like USDT. The bugs stem from using the base chain (`ethereum`) for storage lookups when the original chain parameter (`ethereum-usdt`) should be used. |
| 333 | |
| 334 | **Impact:** |
| 335 | Users may receive funds to unintended addresses, leading to confusion and potential loss of funds if they don't control all derived addresses. |
| 336 | |
| 337 | **Recommended Action:** |
| 338 | Apply the fixes to `/api/qrcode/` and `/api/address/` endpoints immediately, then run comprehensive tests before deploying. |
| 339 | |
| 340 | **Estimated Fix Time:** |
| 341 | - Code changes: 10 minutes |
| 342 | - Testing: 30 minutes |
| 343 | - Total: < 1 hour |