Skip to content

fix(OwnableValidator): bump checknsignatures to revert-4-fix#32

Open
highskore wants to merge 1 commit into
mainfrom
feat/ownable-validator-v2-redeploy
Open

fix(OwnableValidator): bump checknsignatures to revert-4-fix#32
highskore wants to merge 1 commit into
mainfrom
feat/ownable-validator-v2-redeploy

Conversation

@highskore
Copy link
Copy Markdown
Contributor

Summary

  • Pins @rhinestone/checknsignatures to the revert-4-fix/checknsignatures branch so the next OwnableValidator redeploy bakes in the un-bugged checkNSignatures (strict required-signatures count + revert on malformed contract sigs).
  • Required for fixing ERC-1271 contract-owner flows (clients with smart-contract owners hit failures on the current default OwnableValidator deployment).
  • All 51 OwnableValidator unit + integration tests pass against the new lib.

Notes

  • pnpm install also updated transitive sentinellist (e722c5cc4b03ebb5) and solady (0.1.14 → 0.1.26). The new sentinellist's popAll resets entries[SENTINEL] = SENTINEL instead of leaving it cleared, which means SentinelListLib.alreadyInitialized() returns true after popAll.
  • This does not affect OwnableValidator's behavior — its isInitialized checks threshold[smartAccount] != 0, not list state. SocialRecovery and WebAuthnValidator follow the same pattern and are also unaffected.
  • It does break OwnableExecutor whose isInitialized calls accountOwners[smartAccount].alreadyInitialized() directly (2 integration tests now fail). OwnableExecutor is not part of this redeploy; tracking the fix separately.

Test plan

  • forge build — compiles
  • forge test --mp "test/OwnableValidator/**/*.sol" — 51/51 pass
  • Run targeted redeploy of OwnableValidator from this branch
  • Verify on-chain checkNSignatures reverts on malformed contract-sig payload

🤖 Generated with Claude Code

Pins @rhinestone/checknsignatures to the revert-4-fix/checknsignatures
branch, which restores the strict signature verification semantics:

- Requires exactly `requiredSignatures` valid signatures (the previous
  main allowed any number of valid sigs >= threshold among extra junk)
- Reverts on malformed contract signatures with WrongContractSignatureFormat
  / WrongContractSignature instead of silently treating them as invalid

Required for the OwnableValidator redeploy that fixes contract-owner
(ERC-1271) signature flows. OwnableValidator imports CheckNSignatures
directly; SocialRecovery and WebAuthnValidator also pick up the fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@greg-rhinestone greg-rhinestone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Greg · ⚠️ Needs changes

Pins @rhinestone/checknsignatures to the revert-4-fix/checknsignatures branch so the next OwnableValidator redeploy picks up the strict signature-count and malformed-contract-signature fixes.

Risk: High — this dependency update also lands a known transitive regression outside OwnableValidator on main.

Comment thread package.json
"@erc7579/enumerablemap4337": "github:erc7579/enumerablemap",
"@openzeppelin/contracts": "^5.3.0",
"@rhinestone/checknsignatures": "github:rhinestonewtf/checknsignatures",
"@rhinestone/checknsignatures": "github:rhinestonewtf/checknsignatures#revert-4-fix/checknsignatures",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pin is also what pulls in the newer @rhinestone/sentinellist, and the PR notes say that version breaks OwnableExecutor because popAll() now leaves the list looking initialized. Since this merges to main, we would be landing a known regression outside OwnableValidator just to prep the redeploy. Please either pin/override the old sentinellist here or vendor the checkNSignatures fix so the repo stays green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants