fix: EIP-712 compliant hashing of NoncesToInvalidate salts array#60
fix: EIP-712 compliant hashing of NoncesToInvalidate salts array#60re1ro wants to merge 2 commits into
Conversation
hashNoncesToInvalidate encoded the dynamic bytes32[] salts field inline via abi.encode, which violates the EIP-712 spec. Per EIP-712, a dynamic array member must be encoded as the keccak256 hash of the concatenated encodings of its elements. As a result, standards-compliant wallets using eth_signTypedData_v4 produced a different digest than the contract, so a valid signature for the signed/cross-chain invalidateNonces overloads could not be produced by normal wallet tooling. Hash the salts array with keccak256(abi.encodePacked(salts)), matching the existing EIP-712-correct pattern in Permit3.hashChainPermits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OverviewThis PR corrects the EIP-712 encoding of the salts dynamic array within the NonceManager.hashNoncesToInvalidate function. The review team confirmed that applying keccak256(abi.encodePacked(...)) to the array aligns with standard EIP-712 hashing rules. No security vulnerabilities or QA issues were identified in this change. QA & Test CoverageNo issues found in this domain. Web SecurityNo issues found in this domain. Blockchain SecurityNo issues found in this domain. PerformanceSkipped: not applicable to this PR — PR is a correctness fix with no performance implications; only changes hashing logic without algorithmic changes Stats
Automated review by Eco's specialized review team. Architecture, business logic, and correctness remain with the human reviewer. |
There was a problem hiding this comment.
Pull request overview
Fixes EIP-712 struct hashing for NoncesToInvalidate(uint64 chainId,bytes32[] salts) so that the dynamic bytes32[] field is encoded per spec (hash of concatenated element encodings) rather than being inlined via abi.encode, restoring compatibility with standard eth_signTypedData_v4 tooling.
Changes:
- Update
NonceManager.hashNoncesToInvalidateto hashsaltsaskeccak256(abi.encodePacked(salts))before struct encoding. - Align nonce invalidation hashing behavior with the already-correct dynamic-array hashing pattern used elsewhere (e.g., chain permits).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function hashNoncesToInvalidate( | ||
| NoncesToInvalidate memory invalidations | ||
| ) public pure returns (bytes32) { | ||
| return keccak256(abi.encode(NONCES_TO_INVALIDATE_TYPEHASH, invalidations.chainId, invalidations.salts)); | ||
| return keccak256( | ||
| abi.encode( | ||
| NONCES_TO_INVALIDATE_TYPEHASH, invalidations.chainId, keccak256(abi.encodePacked(invalidations.salts)) | ||
| ) | ||
| ); |
The existing test_hashNoncesToInvalidate only asserted the result was non-zero, so it passed for both the buggy and the fixed hash and guarded against nothing. Add two regression tests: 1. test_hashNoncesToInvalidate_matchesEip712AndRejectsBuggyForm: computes the expected struct hash inline the way a standards-compliant EIP-712 implementation does (dynamic bytes32[] salts hashed via keccak256(abi.encodePacked(salts))) and asserts the contract matches it. Also asserts the OLD buggy inline-array encoding differs, so the test would have caught the original bug. 2. test_signedInvalidation_acceptsStandardEip712Signature: builds the full EIP-712 digest independently of the contract's hashing helpers (domain separator + \x19\x01 prefix + independently-built struct hashes), signs it, and calls the signed invalidateNonces(address,uint48,bytes32[],bytes) overload, asserting each salt is invalidated and NonceInvalidated fires. This proves a signature from standard wallet tooling is now accepted. Both tests fail against the pre-fix contract and pass after it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
NonceManager.hashNoncesToInvalidatewas not EIP-712 compliant. It encoded the dynamicbytes32[] saltsfield inline:Per the EIP-712 spec, the encoding of a struct member that is a dynamic array is
keccak256of the concatenated encodings of its elements — it must not be inlined viaabi.encode. (Forbytes32[], each element encodes to itself, so the correct encoding iskeccak256(abi.encodePacked(salts)).)Impact
Because the on-chain digest differed from what a standards-compliant wallet computes, a signature produced via
eth_signTypedData_v4against theNoncesToInvalidate(uint64 chainId,bytes32[] salts)type would not verify on-chain. This made the signed / cross-chaininvalidateNoncesoverloads —invalidateNonces(address,uint48,bytes32[],bytes)andinvalidateNonces(address,uint48,NoncesToInvalidate,bytes32[],bytes)— effectively unusable with normal wallet tooling.Fix
Hash the array field, matching the already-correct pattern in
Permit3.hashChainPermits:Breaking change
This changes the invalidation signature digest. Any invalidation signed against the old (non-compliant) encoding will no longer verify, and vice versa. In practice no such signatures should exist, since standard wallet tooling could not have produced a valid signature against the old digest to begin with.
Testing
forge test: 213 passed, 0 failed, 0 skipped.test/utils/TestBase.solandtest/NonceManager.t.solcall the on-chainpermit3.hashNoncesToInvalidate()directly rather than recomputing the digest off-chain, and no test hardcoded the old digest — so the fix propagated automatically.🤖 Generated with Claude Code