another adversarial review's fixes

apps/simplex-badge-service/src/BadgeService/Service.hs

@@ -35,7 +35,7 @@ import Simplex.Chat.Bot (initializeBotAddress')
 import Simplex.Chat.Controller
 import Simplex.Chat.Core (sendChatCmd, simplexChatCore)
 import Simplex.Chat.Names.Protocol
-import Simplex.Chat.Names.Snrc (Intent (..), RecordKey (..), SnrcDeployment (..), intentDigest, parseRecordKey)
+import Simplex.Chat.Names.Snrc (Intent (..), RecordKey (..), SnrcDeployment (..), devChainId, intentDigest, parseRecordKey)
 import Simplex.Chat.Wallet (parseEthSignature, recoverSigner)
 import Simplex.Chat.Options (printDbOpts)
 import Simplex.Chat.Terminal (terminalChatConfig)
@@ -102,6 +102,12 @@ minCommitmentAge = 1
 minNameLength :: Int
 minNameLength = 6
 
+-- | What the contract accepts in a label. Mirrored here because the mock is
+-- what every test and every local run registers against: a mock that is more
+-- permissive than the chain makes the gates look enforced when they are not.
+validNameChar :: Char -> Bool
+validNameChar c = (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '-'
+
 reservedLabels :: Set Text
 reservedLabels = S.fromList ["simplex", "support", "admin", "acme"]
 
@@ -238,7 +244,9 @@ mockDeployment :: SnrcDeployment
 mockDeployment =
   SnrcDeployment
     { sdTld = "simplex",
-      sdChainId = 1,
+      -- Not 1: these are placeholder contracts, and a domain separator that
+      -- says "mainnet" is one that a real deployment could be made to accept.
+      sdChainId = devChainId,
       sdRegistrar = mockAddr 1,
       sdResolver = mockAddr 2
     }
@@ -280,7 +288,14 @@ handleNamesRequest chain NamesRequest {nrVersion, nrRequest}
           pure
             NRPQuote
               { nrLabel,
-                nrAvailable = maybe (not (S.member nrLabel reservedLabels) && T.length nrLabel >= minNameLength) (const False) live,
+                nrAvailable =
+                  maybe
+                    ( T.all validNameChar nrLabel
+                        && not (S.member nrLabel reservedLabels)
+                        && T.length nrLabel >= minNameLength
+                    )
+                    (const False)
+                    live,
                 nrTakenUntil = neExpiry <$> live,
                 nrReserved = S.member nrLabel reservedLabels,
                 -- $10/yr for 6+ characters, the only rung reachable while the
@@ -374,11 +389,21 @@ handleNamesRequest chain NamesRequest {nrVersion, nrRequest}
         Just r -> pure r
         Nothing -> do
           r <- act
-          modifyTVar' chain $ \c' -> c' {chainRequests = M.insert (unRequestId rid) r (chainRequests c')}
+          -- Only settled answers are replayed. Caching a failure would make a
+          -- retry with the same id permanently unable to succeed, which is the
+          -- opposite of what an idempotency key is for.
+          case r of
+            NRPError {} -> pure ()
+            _ -> modifyTVar' chain $ \c' -> c' {chainRequests = M.insert (unRequestId rid) r (chainRequests c')}
           pure r
     checkGates nm =
       let label = T.takeWhile (/= '.') nm
        in if
+            -- The contract's charset. Without this the reserved set is bypassed
+            -- by a capital letter - "Support" is not "support" to a Set, nor to
+            -- a Map key - and two names that read alike can both exist.
+            | not (T.all validNameChar label) ->
+                Just $ NRPError NECNameInvalid (Just "names use lowercase letters, digits and hyphens") Nothing
             | T.length label < minNameLength -> Just $ NRPError NECNameTooShort Nothing Nothing
             | S.member label reservedLabels -> Just $ NRPError NECNameReserved Nothing Nothing
             | otherwise -> Nothing

docs/rfcs/2026-08-18-in-app-name-purchase-mvp.md

@@ -308,6 +308,21 @@ Between commit and reveal the core waits `commitWaitMs` (1 s), and the service
 reveal whose commitment is too new is refused. Both are short in the mock;
 production is 60 s. Deposit hardening is still deferred.
 
+What this defends against is a third party watching the chain. It is not a
+defence against the relayer itself, and the difference is worth stating: a
+commitment discloses nothing, so the relayer is the first party to learn the
+name, and it is the party that decides when the reveal reaches the chain. A
+hostile relayer can stall a reveal, publish its own commitment for the same
+name, wait out the minimum age and register — returning `name_taken` to a user
+who has already spent a code that DEC6 will not replace.
+
+The answer is relay diversity rather than the commitment alone. Because the
+commitment already binds the owner, a reveal one relayer refuses to submit can
+be handed to another and the claim survives intact — no re-commit, no new
+secret, no lost priority. Acting on that needs the client to verify chain state
+for itself, so it can tell a stall from a slow block; until then the guarantee
+rests on the relayer being willing rather than on it being unable.
+
 ## Sequence
 
 ```mermaid
@@ -517,3 +532,33 @@ Each of these is a test, not a claim.
 - Redemption codes are a lookup table the issuer holds, so a code links the buyer
   to the name it bought. Unlinkable blind-signed codes are deferred to their own
   branch.
+- **A relayer can stall a reveal and take the name.** Described above. The
+  mitigation — hand the reveal to a different relayer — needs client-side chain
+  verification, which is not built. Until it is, the registrar is trusted for
+  liveness on the reveal, and the documents should not claim otherwise.
+- **A recovery scan discloses the whole derivation tree to one registrar.**
+  `scanSeed` asks one service about ~41 addresses per seed (81 for `rescan
+  more`) in a single burst, including every address that owns nothing — the
+  user's future name keys, disclosed before use. Fresh connections per request
+  do not help, because the burst correlates on timing and the addresses are
+  related by BIP-32 anyway. The intended shape is a delegated on-chain lookup:
+  resolve one address at a time through the SNRC resolver, spread over a diverse
+  set of SMP relays, so no relay sees more than a fragment. Not built.
+- **A refused purchase consumes name indices.** `deriveNameOwner` takes an index
+  before the service answers and steps over paths already recorded, so a run of
+  failures leaves gaps and the path a user is shown is not the one they would
+  predict. Indices are free; the surprise is the cost.
+- **The free-index check and the record are separate transactions.**
+  `nameKeyPathTaken` and `recordNameKey` are two stores, so two concurrent
+  purchases could pass the same check. Unreachable while commands are processed
+  serially, and worth revisiting before anything drives the API concurrently.
+- **Two profiles can be pinned to the same account.** `/name keys use <n>
+  <account>` accepts an index another profile already holds, and each profile's
+  name counter starts independently. What keeps their keys apart is the
+  device-wide path check in `deriveNameOwner`, not the binding — so that check
+  is load-bearing rather than defence in depth.
+- **Names are device-wide, not per profile.** `ownedNames` and `nameKeyOf` ignore
+  the calling profile: any profile lists, and can sign for, every name on the
+  device. Deliberate — a seed belongs to the device, `wallet_name_keys` records
+  no profile, and a recovery scan has nothing to attribute what it finds to, so
+  scoping would hide exactly the names a recovery had just restored.

src/Simplex/Chat/Controller.hs

@@ -858,7 +858,7 @@ data ChatResponse
   | CRNameQuote {user :: User, nameLabel :: Text, nameAvailable :: Bool, nameReserved :: Bool, namePriceUsdCents :: Word32, nameYears :: Word32}
   | CRNameCode {user :: User, codeMinLength :: Int, nameYears :: Word32, codeExpires :: UTCTime}
   | CRNames {user :: User, namesOwned :: [(Text, Text, UTCTime, Word32)]}
-  | CRNameInfo {user :: User, regName :: Text, regOwner :: Text, regPath :: Text, nameContact :: [Text], nameChannel :: [Text], regExpiry :: UTCTime, nameEditsLeft :: Word32}
+  | CRNameInfo {user :: User, regName :: Text, regOwner :: Text, regOwnerIsOurs :: Bool, regPath :: Text, nameContact :: [Text], nameChannel :: [Text], regExpiry :: UTCTime, nameEditsLeft :: Word32}
   | CRNameLinkSet {user :: User, regName :: Text, nameRecord :: Text, regTxHash :: TxHash}
   | CRNameRescan {user :: User, namesFound :: [(Text, Text)]}
   | CRNameKeys {user :: User, walletKeys :: [(Int, [(Maybe AccountIndex, [(Maybe NameIndex, Text, Text)])], Bool, Bool)]}

src/Simplex/Chat/Library/Commands.hs

@@ -63,7 +63,7 @@ import Simplex.Chat.Library.Subscriber
 import Simplex.Chat.Badges (BadgeCredential (..), LocalBadge (..), maxXFTPFileSize, mkBadgeStatus, verifyCredential)
 import Simplex.Chat.Names (SimplexDomainProof (..), SimplexDomainClaim (..), claimDomain, mkDomainClaim)
 import Simplex.Chat.Names.Protocol
-import Simplex.Chat.Names.Snrc (Intent (..), SignedIntent (..), SnrcDeployment (..), parseRecordKey, signSnrcIntent)
+import Simplex.Chat.Names.Snrc (Intent (..), SignedIntent (..), SnrcDeployment (..), devChainId, parseRecordKey, signSnrcIntent)
 import Simplex.Messaging.Eth.Address (Address, mkAddress)
 import Simplex.Chat.Store.Wallets (bindSeedAccount, createSeed, currentSeed, getNameKeys, getOrCreateAccountRef, listSeeds, markBackedUp, nameKeyPathTaken, raiseNextAccountIndex, raiseNextNameIndex, recordNameKey, seedOfName, setCurrentSeed, setNextNameIndex, takeNameIndex)
 import Simplex.Chat.Wallet (AccountIndex, AccountRef (..), NameIndex, SeedId, WalletAccount, WalletSeed (..), accountAddress, deriveAtPath, deriveNameKey, ethSignatureBytes, importRecoveryKey, newSeed, parseNameKeyPath, recoveryKeyPhrase, renderNameKeyPath)
@@ -1511,10 +1511,14 @@ processChatCommand cxt nm = \case
     pure $ CRNames user rows
   APINameInfo sendTarget nm' -> withUser $ \user -> do
     cReq <- resolveServiceTarget nm user sendTarget
-    (_, path, _) <- nameKeyOf nm'
+    (_, path, acc) <- nameKeyOf nm'
     namesRPC user cReq (NRResolve nm') >>= \case
-      NRPRecord {nrName, nrOwner, nrContact, nrChannel, nrExpiry, nrEditsLeft} ->
-        pure $ CRNameInfo user nrName (tshow nrOwner) path nrContact nrChannel nrExpiry nrEditsLeft
+      NRPRecord {nrName, nrOwner, nrContact, nrChannel, nrExpiry, nrEditsLeft} -> do
+        -- The service's word against our own key. A registrar that misreports
+        -- the owner cannot be caught anywhere else, and we are holding the
+        -- address it should have named.
+        let ours = accountAddress acc
+        pure $ CRNameInfo user nrName (tshow nrOwner) (nrOwner == ours) path nrContact nrChannel nrExpiry nrEditsLeft
       _ -> throwCmdError "unexpected resolve response"
   APINameSetLink sendTarget nm' record lnk -> withUser $ \user -> do
     cReq <- resolveServiceTarget nm user sendTarget
@@ -5208,7 +5212,9 @@ clientDeployment :: SnrcDeployment
 clientDeployment =
   SnrcDeployment
     { sdTld = "simplex",
-      sdChainId = 1,
+      -- Not 1: these are placeholder contracts, and a domain separator that
+      -- says "mainnet" is one that a real deployment could be made to accept.
+      sdChainId = devChainId,
       sdRegistrar = mockClientAddr 1,
       sdResolver = mockClientAddr 2
     }

src/Simplex/Chat/Names/Protocol.hs

@@ -239,6 +239,7 @@ data NamesErrorCode
   | NECInternal
   | NECNameReserved
   | NECNameTooShort
+  | NECNameInvalid
   | NECPaymentRejected
   | NECCodeSpent
   | NECCodeExpired
@@ -259,6 +260,7 @@ instance TextEncoding NamesErrorCode where
     NECInternal -> "internal"
     NECNameReserved -> "name_reserved"
     NECNameTooShort -> "name_too_short"
+    NECNameInvalid -> "name_invalid"
     NECPaymentRejected -> "payment_rejected"
     NECCodeSpent -> "code_spent"
     NECCodeExpired -> "code_expired"
@@ -276,6 +278,7 @@ instance TextEncoding NamesErrorCode where
     "internal" -> NECInternal
     "name_reserved" -> NECNameReserved
     "name_too_short" -> NECNameTooShort
+    "name_invalid" -> NECNameInvalid
     "payment_rejected" -> NECPaymentRejected
     "code_spent" -> NECCodeSpent
     "code_expired" -> NECCodeExpired

src/Simplex/Chat/Names/Snrc.hs

@@ -20,6 +20,7 @@ module Simplex.Chat.Names.Snrc
     intent712,
     intentDigest,
     signSnrcIntent,
+    devChainId,
   )
 where
 
@@ -76,6 +77,12 @@ data SignedIntent = SignedIntent
 setTextTypeString :: ByteString
 setTextTypeString = "SetText(bytes32 node,string key,string value,uint256 nonce,uint256 deadline)"
 
+-- | The chain the placeholder deployment claims. Deliberately not 1: signatures
+-- carry their chain id, and one that reads as mainnet is one that a contract
+-- deployed at the placeholder address could later be made to honour.
+devChainId :: Integer
+devChainId = 31337
+
 labelHash :: ByteString -> ByteString
 labelHash = keccak256
 

src/Simplex/Chat/View.hs

@@ -211,10 +211,10 @@ chatResponseToView hu cfg@ChatConfig {logLevel, showReactions, showFullLinks, te
     ttyUser u $ case rows of
       [] -> ["no names yet - buy one with /name buy <label> <code>"]
       _ -> map (\(n, points, expiry, edits) -> plain $ "  " <> n <> " -> " <> points <> "  expires " <> tshow expiry <> ", " <> tshow edits <> " edits left") rows
-  CRNameInfo u n owner path contact channel expiry edits ->
+  CRNameInfo u n owner ourKey path contact channel expiry edits ->
     ttyUser u $
       [ plain $ n,
-        plain $ "  owner   " <> owner,
+        plain $ "  owner   " <> owner <> (if ourKey then "  (matches your key)" else "  (NOT your key)"),
         plain $ "  path    " <> path
       ]
         <> map (\c -> plain $ "  contact " <> c) contact

src/Simplex/Chat/Wallet.hs

@@ -218,10 +218,20 @@ signIntent a Eip712Intent {eiDomain, eiTypeString, eiValues} = do
       }
 
 -- | Parse @r || s || v@ as it arrives from a client.
+-- | Half the secp256k1 group order. EIP-2 accepts only the lower half: for
+-- every signature there is a second one at @n - s@ that recovers the same
+-- signer, and accepting both means one authorisation has two identities.
+secp256k1HalfN :: Integer
+secp256k1HalfN = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0
+
 parseEthSignature :: ByteString -> Either String EthSignature
 parseEthSignature bs
   | B.length bs /= 65 = Left "signature: expected 65 bytes"
-  | otherwise = Right EthSignature {esR = B.take 32 bs, esS = B.take 32 (B.drop 32 bs), esV = B.last bs}
+  | beInteger s > secp256k1HalfN = Left "signature: s is not canonical (EIP-2)"
+  | otherwise = Right EthSignature {esR = B.take 32 bs, esS = s, esV = B.last bs}
+  where
+    s = B.take 32 (B.drop 32 bs)
+    beInteger = B.foldl' (\acc w -> acc * 256 + fromIntegral w) 0
 
 -- | Recover the address that produced a signature over a digest — what the
 -- relayer and the contracts do.

tests/Bots/NamesServiceTests.hs

@@ -22,10 +22,14 @@ import Test.Hspec hiding (it)
 import qualified Data.Text as T
 import Data.Text (Text)
 import Data.Text.Encoding (encodeUtf8)
-import Data.Word (Word32)
+import Data.Word (Word32, Word8)
 import Simplex.Messaging.Util (tshow)
 import qualified Simplex.Messaging.Crypto.BIP39 as B39
-import Simplex.Chat.Wallet (SeedId (..), WalletSeed (..), accountAddress, deriveNameKey, parseNameKeyPath, renderNameKeyPath)
+import Data.Either (isRight)
+import Data.List (isInfixOf)
+import Simplex.Chat.Names.Snrc (Intent (..), RecordKey (..), SnrcDeployment (..), devChainId, intent712)
+import Simplex.Messaging.Eth.Address (Address, mkAddress)
+import Simplex.Chat.Wallet (SeedId (..), WalletSeed (..), accountAddress, deriveNameKey, ethSignatureBytes, parseEthSignature, parseNameKeyPath, renderNameKeyPath, signIntent)
 import qualified Test.Hspec as Hspec
 
 namesServiceTests :: SpecWith TestParams
@@ -57,6 +61,24 @@ namesProtocolTests = do
     addrOf 0 1 `shouldBe` "0x6Fac4D18c912343BF86fa7049364Dd4E424Ab9C0"
     -- Ledger Live account 2 for this phrase
     addrOf 1 0 `shouldBe` "0x78839F6054d7ed13918bAe0473BA31b1Ca9D7265"
+  -- EIP-2: for every signature there is a second one at n - s that recovers the
+  -- same signer. Accepting both would give one authorisation two identities.
+  it "refuses a signature whose s is not canonical" $ \_ -> do
+    let mn = either error id $ B39.parseMnemonic "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"
+        sd = WalletSeed {wsId = SeedId 1, wsEntropy = B39.mnemonicToEntropy mn}
+        acc = either error id $ deriveNameKey sd 0 0
+        intent = SetTextRecord "alice.simplex" RKContact "simplex:/contact#/x" 0 9999999999
+        sig = either error id $ signIntent acc (intent712 clientTestDeployment intent)
+        bs = ethSignatureBytes sig
+        s0 = beInt (B.take 32 (B.drop 32 bs))
+        flipped = B.take 32 bs <> beBytes (secpN - s0) <> B.singleton (if B.last bs == 27 then 28 else 27)
+    -- the signature the wallet produced is canonical
+    isRight (parseEthSignature bs) `shouldBe` True
+    -- its mirror image is refused rather than recovered
+    case parseEthSignature flipped of
+      Left e -> ("not canonical" `isInfixOf` e) `shouldBe` True
+      Right _ -> expectationFailure "non-canonical s was accepted"
+
   -- The account a name sits under is read back out of its stored path: that is
   -- the only record of which profile owned it, and what the recovery scan reads
   -- to know which indices are taken. A path we did not generate has no account
@@ -356,11 +378,40 @@ testBuyRefusals ps =
     client <## "name another.simplex: registered"
     client <##. "name registered: another.simplex -> 0x"
     client <##. "  derivation path: m/44'/60'/0'/0/"
+    -- A capital letter is not a different name, it is an invalid one. Without
+    -- this the reserved set is bypassed by case: "Support" would register while
+    -- "support" is held, and two names that read alike would both exist.
+    client ##> ("/name buy " <> bsLink <> " Support " <> T.unpack (devCode 3) <> " simplex:/contact#/x")
+    client <## "name Support.simplex: revealing"
+    client <##. "name registration failed: name_invalid"
+    -- and the code is not spent by a refusal, so it still works
+    client ##> ("/name buy " <> bsLink <> " lowered " <> T.unpack (devCode 3) <> " simplex:/contact#/x")
+    client <## "name lowered.simplex: revealing"
+    client <## "name lowered.simplex: registered"
+    client <##. "name registered: lowered.simplex -> 0x"
+    client <##. "  derivation path: m/44'/60'/0'/0/"
     -- Expiry is a property of the key, not of the code, so a build with one
     -- cohort key cannot mint an expired code. Expiry refusal is covered by the
     -- unit test over verifyCode instead.
 
 
+secpN :: Integer
+secpN = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141
+
+beInt :: B.ByteString -> Integer
+beInt = B.foldl' (\a w -> a * 256 + fromIntegral w) 0
+
+beBytes :: Integer -> B.ByteString
+beBytes n = B.pack [fromIntegral (n `div` (256 ^ i)) | i <- [31, 30 .. 0 :: Int]]
+
+testAddr :: Word8 -> Address
+testAddr n = either error id $ mkAddress (B.replicate 19 0 <> B.singleton n)
+
+-- | Mirrors the client's placeholder deployment, so the digest is the real one.
+clientTestDeployment :: SnrcDeployment
+clientTestDeployment =
+  SnrcDeployment {sdTld = "simplex", sdChainId = devChainId, sdRegistrar = testAddr 1, sdResolver = testAddr 2}
+
 -- | The service's pre-issued table, mirrored so tests can name a code.
 devCode :: Int -> Text
 devCode i = ["SMPX-4K2P-7TQW-9XRM", "SMPX-8H3N-2VBD-6JYK", "SMPX-5L9C-4WFT-1ZQA", "SMPX-7R6M-8PGX-3NHV"] !! (i - 1)