Narrow extraData from uint240 to uint16 and update salt types#66
Merged
Merged
Conversation
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
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
sudo-owen
added a commit
that referenced
this pull request
May 5, 2026
* wip * wip * wip * wip transpiler fixes * wip cleanup * wip * clean up transpiler * wip damage gifs * Narrow extraData from uint240 to uint16 and update salt types (#66) * 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> * clean up --------- 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
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
This PR narrows the
extraDataparameter in move execution fromuint240touint16across the codebase, and updates salt types frombytes32touint104in commit-reveal flows. These changes align with the OPT_PLAN optimization strategy for batched execution and reduce storage overhead.Key Changes
Type Narrowing
extraData: Reduced from
uint240touint16in:IMoveSet.move()interface signatureSalt: Changed from
bytes32touint104in:SignedCommitManagerdual-signed flowSignedCommitLibstructures_turnP0Salt,_turnP1Salt)Supporting Changes
_packStatBoost()and_packForceSwitch()helpers to work with 16-bit extraData_completeTurnFast()test helper to use new signature format with committer signatureDocumentation
OPT_PLAN.mddetailing the batched execution optimization strategy, buffer layout, and security model for dual-signed movesImplementation Details
[p0MoveIndex:8 | p0ExtraData:16 | p0Salt:104 | p1MoveIndex:8 | p1ExtraData:16 | p1Salt:104]https://claude.ai/code/session_01Lc98i85bMi3SWTBnaNiDuZ