Branch asymmetry inside settle(): the decisive branch validates that winner is player A or B, the draw branch validates nothing about winner, yet both branches emit the same Settled(matchId, winner, draw, payout, fee) event with winner as an indexed topic (line 146: emit Settled(matchId, winner, true, stake, 0);).

On-chain transfers are unaffected (a draw always refunds A and B in full, verified), so no funds are at risk and this is within the brief's wording ("winner must be one of the two players unless draw"). The risk is only off-chain: an indexer, leaderboard or payout reconciler that filters Settled by the winner topic without also reading draw will attribute a result to an address that never played.

The contract comment at line 129 says the signed winner is retained in the event on purpose; INTEGRATION.md recommends the backend sign player A as the ignored winner. Suggested minimal fix if the author wants on-chain tightening without changing the schema: on a draw require winner == address(0) || winner == playerA || winner == playerB, or emit address(0) as the event winner for draws. This is a trust/asymmetry note, not a permission bypass; no revision is required by it.

README.md line 15 instructs verifiers to run sha256sum --check lib/SHA256SUMS, docs/SECURITY.md line 16 records that all 50 files matched, and lib/README.md line 3 states the vendored sources are unmodified. None of this is true for the committed tree: 11 of the 50 files fail the check (OpenZeppelin Address.sol, ECDSA.sol, MessageHashUtils.sol, IERC20Permit.sol; forge-std StdAssertions.sol, StdJson.sol, StdToml.sol, Vm.sol, console.sol, IERC7540.sol, IMulticall3.sol).

I downloaded the pinned upstream files (OpenZeppelin v5.0.2, forge-std v1.9.7): the manifest hashes equal upstream, so the manifest is right and the tree's copies were altered after vendoring. Diffing shows only reformatting (a forge fmt pass: joined signature lines, hex"19_01" -> hex"1901", (bool success, ) -> (bool success,)), so compiled behaviour of ECDSA/MessageHashUtils is unchanged and this is not a logic defect.

It is still a defect in the submitted artefact: the only provenance control the project ships fails, the validation record claims a pass that cannot have happened on this tree, and a later source-publication/attestation step that re-runs the documented check will fail.

Fix: either restore the 11 files byte-for-byte from the upstream release archives (and exclude lib/ from forge fmt, e.g. ignore = ["lib/**"] in the [fmt] section), or regenerate lib/SHA256SUMS from the formatted files and correct the 'unmodified' wording; then re-run the check before recording its result.

open() keys matches only by the caller-chosen matchId and refuses any later open with the same id, and a record is never cleared (cancel/refund set settled=true, playerA stays). The server hands player A a session id to open with; a mempool observer who sees A's pending open(matchId, ...) can submit open(matchId, IMD, 0.5e18, anything) with higher gas, making A's transaction revert with MatchAlreadyExists, then call cancel(matchId) in the next block to get the 0.5 IMD back.

Cost to the attacker is gas plus a one-block lock of the minimum stake; cost to the victim is a failed transaction and a new server-issued id each time. Repeated against every open this blocks lobby creation for the whole product on a public mempool. It also lets the squatter put an unexpected token/stake/opponent under an id the server already associates with A's session, which docs/INTEGRATION.md delegates to the backend to detect.

Not a loss of escrowed funds. Minimal fix that keeps the requested interface: namespace the record by the opener, e.g. bytes32 key = keccak256(abi.encode(msg.sender, matchId)) for storage/events (or key the mapping as matches[playerA][matchId]) so only A can consume A's id, or have the server co-sign the open. Either is a design decision for the author; reporting so it is made consciously.

open() keys the record only by the caller-chosen matchId and makes every ID a permanent nonce (line 13), while cancel() returns the squatter's stake at once.

Anyone watching the mempool can front-run player A's open(matchId,...) with their own open(matchId, IMD, 0.5e18, ...) followed by cancel(matchId) in the same block: the squatter loses nothing but gas, A's transaction reverts with MatchAlreadyExists, and the ID is unusable forever, so the server must mint a new ID and the attacker can repeat indefinitely (a liveness/DoS on lobby creation; with the documented zero-minimum option the squat is free of capital entirely).

The same race lets a squatter open the ID with a different stake (up to the current max) before A; join(bytes32) takes whatever token/stake/opponent the record holds and gives B no way to pin the expected parameters, so a B that follows the server's 'join this ID' instruction without re-reading the record deposits the squatter's stake and is locked until the 24h refund.

No funds are lost in either variant (the squatter cannot be paid unless the signer names them), which is why this is low. Reproduced in test/scratch/IdSquat.t.sol (2 passing tests).

Possible fixes, each a scope decision because the brief fixes the open/join signatures: key the record by keccak256(playerA, matchId) so an ID can only collide with the same creator; or require the server's signature over (matchId, playerA, token, stake) in open; and/or add expected token/stake/playerA arguments to join that are compared against the stored record.

README.md line 15 and lib/README.md line 10 instruct verifiers to run sha256sum --check lib/SHA256SUMS, and docs/SECURITY.md records that all 50 files matched. On the submitted tree the check reports 11 FAILED: openzeppelin IERC20Permit.sol, Address.sol, ECDSA.sol, MessageHashUtils.sol and forge-std StdAssertions.sol, StdJson.sol, StdToml.sol, Vm.sol, console.sol, interfaces/IERC7540.sol, interfaces/IMulticall3.sol.

I fetched the pinned upstream files (OpenZeppelin v5.0.2, forge-std v1.9.7): the upstream bytes hash to exactly the values in SHA256SUMS, so the sums are right and the local copies were changed, evidently by running forge fmt over lib/.

A whitespace-insensitive comparison (and hex"19_01" vs hex"1901") shows the differences are formatting only, so ECDSA/SafeERC20 behaviour is unchanged and this is not a security defect in the escrow; but the recorded validation result is untrue, the stated provenance guarantee ('original source contents are unmodified') does not hold, and any reviewer or deployer following the README gets a failed integrity check.

Fix: restore the 11 files to their upstream bytes (or regenerate SHA256SUMS from the vendored files and correct the SECURITY.md row), and keep lib/ out of future fmt runs.

matchId is chosen by the caller and is a permanent nonce (line 13: 'Match IDs are permanent nonces, including after cancellation or refund'). cancel() (line 175-183) refunds player A at once with no waiting period and no fee, so a griefer who sees Alice's pending open(matchId, ...) in the public mainnet mempool (launch chain is Ethereum, chain ID 1) can front-run it with a contract that calls open(matchId, IMD, 0.5 ether, game) then cancel(matchId) in one transaction.

The griefer's stake comes straight back; the only cost is gas. Alice's transaction then reverts with MatchAlreadyExists and the matchId is unusable forever, so a session the NIGHTSHIFT server has already bound to that id must be re-issued. Repeated against one player this blocks them from opening any match whose id is visible before inclusion.

No funds are lost; this is a targeted denial of service inherent to caller-chosen ids plus free instant cancellation.

Mitigation that keeps the spec's flow: derive the stored key from the caller as well as the supplied id (e.g. keccak256(abi.encodePacked(msg.sender, matchId))) so only Alice can occupy Alice's id, or have the frontend submit ids that are not shared before inclusion; INTEGRATION.md already asks for unpredictable ids, which is a client-side control the contract does not enforce.

This follows the brief's struct exactly (Result(bytes32 matchId,address winner,bool draw,uint256 deadline)) and is reported as a trust seam, not a code defect. join() is permissionless and the contract has no reserved opponent, so the on-chain playerB can differ from the opponent the server matched off-chain. The contract only checks that the signed winner is one of the two recorded players.

If the server signs 'matchId M, winner Alice' based on its own lobby (Alice vs Bob) while on-chain Carol front-ran Bob's join, Carol loses her stake to Alice without having played her, and 5% goes to the splitter. Conversely a result pre-signed before join (winner = playerA) makes whoever joins lose instantly. docs/INTEGRATION.md documents that the backend must read the on-chain match snapshot and reject unexpected participants before signing and must never pre-sign.

The contract cannot enforce that. Changing the struct to include playerA, playerB, token and stake would close the seam on-chain but changes the agreed signing schema, so it is a scope decision for the requester; otherwise the server-side checks in INTEGRATION.md are load-bearing for fund safety.

Merged from audit_math, audit_flow and audit_economics (same root cause). open() keys a match only by the caller-supplied matchId, refuses any later open with that id, and no outcome ever clears the record (cancel/refund/settle set settled=true and playerA stays). cancel() returns player A's stake immediately with no delay or fee.

A mempool observer who sees Alice's pending open(id, ...) can submit, with higher priority, a contract call doing open(id, IMD, 0.5e18, x) then cancel(id) in one transaction: the squatter's stake comes straight back, Alice's transaction reverts with MatchAlreadyExists, and the id is unusable forever, so the server must issue a new id and the attack can repeat.

Variant: the squatter opens the id with a different stake (up to the current max); join(bytes32) has no expected token/stake/opponent parameter, so a player B who follows the server's 'join this id' instruction without re-reading matches(id) deposits the squatter's stake and is locked until settle or the 24-hour refund.

No escrowed funds can be taken (payout goes only to recorded players or the splitter), so this is a liveness/denial-of-service and UX defect, not a loss of funds.

The behaviour follows the brief's 'matchId must be new' literally, so any fix is a scope decision for the requester: key the record by keccak256(abi.encode(msg.sender, matchId)) so only Alice can consume Alice's id (keeps the open/join signatures), or require a server signature over (matchId, playerA, token, stake) in open, and/or give join expected token/stake/playerA arguments compared against the record. docs/INTEGRATION.md already delegates id unpredictability and participant checks to the backend; the contract does not enforce them.

Merged from audit_math and audit_flow. README.md line 15 and lib/README.md instruct verifiers to run sha256sum --check lib/SHA256SUMS, and docs/SECURITY.md line 16 records that all 50 files matched. On the committed tree 11 files fail: OpenZeppelin Address.sol, ECDSA.sol, MessageHashUtils.sol, IERC20Permit.sol and forge-std StdAssertions.sol, StdJson.sol, StdToml.sol, Vm.sol, console.sol, interfaces/IERC7540.sol, interfaces/IMulticall3.sol.

I fetched the pinned upstream OpenZeppelin v5.0.2 files: their hashes equal the values in lib/SHA256SUMS, so the manifest is right and the tree's copies were altered after vendoring. diff -w against upstream shows only forge fmt style changes (re-wrapped function signatures, hex"19_01" -> hex"1901"), so the compiled ECDSA/EIP-712/SafeERC20 behaviour is unchanged and this is not a logic or security defect in the escrow.

It is a defect in the submitted artefact: the only provenance control the project ships fails, the validation table claims a pass that cannot have happened on this tree, and lib/README's 'unmodified' statement is false.

Fix: restore the 11 files byte-for-byte from the upstream releases and exclude lib/ from fmt (e.g. ignore = ["lib/**"] under [fmt]), or regenerate lib/SHA256SUMS from the formatted files and correct the wording; then re-run the check before recording its result.

From audit_economics. The struct follows the brief exactly, so this is a documented trust assumption rather than a code defect. join() is permissionless and there is no reserved opponent, so the on-chain playerB can differ from the opponent the server matched off-chain, and the contract only checks that the signed winner is one of the two recorded players.

If the server signs 'matchId M, winner Alice' from its own lobby record (Alice vs Bob) while Carol front-ran Bob's join, Carol loses her stake to Alice without having played her.

A result pre-signed before join (winner = playerA) makes whoever joins lose instantly. docs/INTEGRATION.md requires the backend to read matches(M) and reject unexpected participants before signing and never to pre-sign; those server-side checks are load-bearing for fund safety and the contract cannot enforce them.

Binding playerA, playerB, token and stake into the signed struct would close the seam on-chain but changes the agreed signing schema, so it is a scope decision for the requester.