Conversation
* Phase 0: dual-sig security fix + extraData/salt width narrowing
Per OPT_PLAN.md §9 Phase 0, in preparation for the batched-execute work:
1. Security fix to `SignedCommitManager.executeWithDualSignedMoves`:
- Add `bytes calldata committerSignature` parameter before `revealerSignature`.
- Recover committer signature against `SignedCommit{committerMoveHash,
battleKey, turnId}`; require equality with the committer.
- Drop the fragile `msg.sender == committer` check (and `error
CallerNotCommitter`). The function is now relayer-friendly: anyone with
both valid signatures + the committer preimage can submit.
- Closes the "unilateral revealer" attack: previously a malicious revealer
could sign `DualSignedReveal{committerMoveHash: keccak(P*), …}` for any
preimage `P*` and submit (when the msg.sender check evolved away,
batching, etc.). Now the explicit committer signature binds the committer
cryptographically.
2. Width narrowing (clean break, no shims) — touched together because
`DualSignedReveal`'s EIP-712 typehash includes both fields:
- `extraData`: 240 → 16 bits across `IMoveSet`, all 50+ mon moves/abilities,
`Engine`/`IEngine` (`setMove`, `executeWithMoves`, `executeWithSingleMove`,
`validatePlayerMoveForBattle`, `MoveDecision.extraData`,
`RevealedMove.extraData`), `IValidator`/`DefaultValidator`,
`ValidatorLogic`, the commit managers, the CPU stack, and `SleepStatus`.
- `salt`: 256 → 104 bits across the same surface, plus the engine's
`_turnP{0,1}Salt` transients, `BattleConfig{,View}.{p0,p1}Salt`,
`RevealedMove.salt`, and the `MonMove` event signature.
- `SignedCommitLib.DualSignedReveal.revealerSalt` and the matching EIP-712
typehash updated to `uint104` / `uint16`.
- Random-oracle interface kept as `bytes32` (it's an arbitrary-input hash
surface, not a security boundary); `Engine` casts at the call site.
- Test mocks repacked into 16 bits with explicit layouts:
- `_packStatBoost` / `StatBoostsMove`: `[boost:8 | stat:4 | mon:3 | player:1]`
- `_packForceSwitch` / `ForceSwitchMove`: `[mon:15 | player:1]`
- `EditEffectAttack`: `[effectIdx:10 | monIdx:4 | targetIdx:2]`
- `MockKVWriterMove`: `[value:6 | key:10]`
- `MockEffectRemover`: switched from passing the effect address (no longer
fits) to passing the slot index from `getEffects`.
3. New TDD regression tests in `SignedCommitManager.t.sol`:
- `test_executeWithDualSigned_thirdPartyRelay_succeeds` — drops msg.sender
check; arbitrary relayer with both valid sigs can submit.
- `test_revert_executeWithDualSigned_unilateralRevealerAttack` — revealer
alone (forging committer sig with their own key) reverts on the
committer-sig recovery.
- `test_revert_executeWithDualSigned_wrongCommitterSigner` — committer sig
recovers to a non-committer address.
- `test_revert_executeWithDualSigned_committerSigForWrongHash` — committer
signs a different `moveHash` than the submitted preimage → engine
recomputes from preimage, mismatch reverts.
4. Imported the OPT_PLAN.md and BatchInstrumentationTest.sol scaffolding from
`feat/next` so subsequent phases land on the same plan.
Two `BetterCPU` strategy tests (`test_betterCPUSelectsHighestDamageMove`,
`test_defensiveSwitch_materialityNotMet`) needed a CPU-mon speed bump to
deterministically break the speed tie: with old 256-bit salts the
salt-derived RNG happened to give CPU priority, with 104-bit salts it
doesn't, so the original tests' assumption was implicit. The fix is to make
the priority deterministic instead of leaning on RNG.
Gas snapshots refreshed across the affected suites — dual-signed flow gains
the second `ecrecover`, partly offset by narrower calldata.
`forge build` clean. `forge test`: 352/352 passing.
https://claude.ai/code/session_01Lc98i85bMi3SWTBnaNiDuZ
* Phase 0 cleanup: dedupe test helpers, trim narrative comments
Per /simplify review of Phase 0:
1. **Consolidate EIP-712 signing helpers.** Phase 0 introduced four byte-identical
copies of `_signCommit` and grew `_signDualReveal` to four copies (it had two
pre-Phase 0). Added `test/abstract/SignedCommitHelper.sol` — a standalone
abstract that takes the verifying-contract address as a parameter (no
`EIP712` inheritance needed; `_DOMAIN_TYPEHASH` is replicated as a constant)
and is inherited by `SignedCommitManager.t.sol`, `BatchInstrumentationTest`,
`InlineEngineGasTest::FullyOptimizedInlineGasTest`, and
`StandardAttackPvPGasTest`. Drops ~280 lines and the `_domainNameAndVersion`
override boilerplate from each.
2. **Move `_packStatBoost` to `BattleHelper`.** Three byte-identical copies
(load-bearing — the bit layout must stay in lockstep with `StatBoostsMove`'s
decoder) collapsed into a single home.
3. **Use existing `_createMonWithSpeed` helper in `BetterCPUTest.sol`.** The
two Phase 0 edits open-coded `_createMon(...); mon.stats.speed = 100;` —
replaced with the helper that already exists at line 77.
4. **Trim narrative `// what` comments from `SignedCommitManager.sol` per
CLAUDE.md guidance** (default to no comments; only WHY). Kept the
stack-pressure rationale on the scoped recovery blocks and the natdoc on
the function itself (which carries the real WHY about the
unilateral-revealer attack).
5. **Add a one-line WHY for the salt truncation in `CPUMoveManager.sol`** so
the deliberate 256→104-bit narrowing isn't cargo-culted as accidental.
Build clean. `forge test`: 352/352 passing. Gas snapshots refreshed where the
helper-extraction path shifted dispatch overhead.
Skipped findings (deliberate):
- Hoisting `_getCurrentTurnSalt` calls in `Engine.sol` to save one or two
transient loads per turn — pre-existing inefficiency, scope of the later
helper-extraction phase (§9 Phase 0.5).
- Tightening `MockKVWriterMove`'s misleading `uint192 value` to a true 6-bit
type with bound checks — works for current tests, value-range guard would
be useful but the mock is test-only.
https://claude.ai/code/session_01Lc98i85bMi3SWTBnaNiDuZ
---------
Co-authored-by: Claude <noreply@anthropic.com>
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
1 participant
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.
No description provided.