Fallback AccountERC7579.isValidSignature to native signer#6443
Fallback AccountERC7579.isValidSignature to native signer#6443james-toussaint wants to merge 4 commits intoOpenZeppelin:masterfrom
AccountERC7579.isValidSignature to native signer#6443Conversation
🦋 Changeset detectedLatest commit: 982ced2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…t-native-validation-fallback
| } | ||
| } | ||
| return bytes4(0xffffffff); | ||
| return _rawSignatureValidation(hash, signature) ? IERC1271.isValidSignature.selector : bytes4(0xffffffff); |
There was a problem hiding this comment.
I would love to check with @ernestognw if that was not by design.
Basically, if we do that we don't have 7739's security, and that could be unsafe. Lets discuss that.
| const { shouldBehaveLikeERC1271 } = require('../../utils/cryptography/ERC1271.behavior'); | ||
|
|
||
| async function fixture() { | ||
| const fixtureWithValidator = () => fixture(true); // parameters not supported by `loadFixture` |
There was a problem hiding this comment.
this is problematique. lamdba function like that are not properly supported by loadFixture. It ends up re-running everytime, which defeats the purpose of having a fixture.
WalkthroughThis change introduces a fallback to native signature validation in Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/account/extensions/draft-AccountERC7579.sol (1)
187-201:⚠️ Potential issue | 🟠 MajorThis can bypass ERC-7739 wrapping in mixed accounts.
If a derived account resolves
ERC7739.isValidSignature(...)first and then falls back toAccountERC7579.isValidSignature(...)on0xffffffff—the same pattern already used incontracts/mocks/account/AccountMock.solLines 111-119—an unwrapped native signature now succeeds after ERC-7739 rejects it. That weakens the 7739 protection unless native fallback is made explicit/opt-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/extensions/draft-AccountERC7579.sol` around lines 187 - 201, The current isValidSignature in AccountERC7579 allows falling back to _rawSignatureValidation even when a validator module was present/checked, enabling an unwrapped native signature after ERC-7739 rejects it; change the logic so that if _extractSignatureValidator finds a module and isModuleInstalled(MODULE_TYPE_VALIDATOR, ...) is true then you must treat a failing validator call (either revert or non-matching magic) as an explicit rejection and return bytes4(0xffffffff) instead of falling back to _rawSignatureValidation; only allow fallback to _rawSignatureValidation when no validator module was found/installed. Update the code paths around isValidSignature, _extractSignatureValidator, isModuleInstalled and the try/catch of IERC7579Validator.isValidSignatureWithSender to implement this behavior and still return IERC1271.isValidSignature.selector on explicit validator success.
🧹 Nitpick comments (1)
test/account/extensions/AccountERC7579.test.js (1)
29-32: This setup never exercises the actual module→native fallback.The
withValidatorbranch deploys a module-only account and the other branch deploys a native-only account, so the newisValidSignaturebehavior is only tested in isolation. There is still no case where the same account has both a validator and native signer enabled, which leaves validator precedence and fallback after a missing/reverting validator call untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/account/extensions/AccountERC7579.test.js` around lines 29 - 32, The test currently only creates either a module-only account (AccountERC7579Mock) or a native-only account (AccountERC7579NativeValidationMock) so it never exercises validator→native fallback or precedence; update the test around helper.newAccount to add a case that enables both a validator module and a native signer on the same account (e.g. deploy the module validator via AccountERC7579Mock and also add signer.address as a native signer), then add assertions invoking isValidSignature on that combined account to verify the validator is preferred when it returns a value and that a reverting/missing validator call falls back to the native signer behavior (simulate validator revert or remove validator and confirm native signature is accepted). Ensure you reference helper.newAccount, AccountERC7579Mock, AccountERC7579NativeValidationMock, and isValidSignature when implementing the new combined-account test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/modern-taxes-exist.md:
- Line 5: Update the changeset text for `AccountERC7579` to precisely describe
the implemented behavior: state that the contract falls back to native signature
validation only when no installed validator handles the signature or when a
validator call reverts, and explicitly note that an installed validator
returning `0xffffffff` still short-circuits and is treated as invalid; replace
the broad phrase "if module validation fails" with this narrower, exact
behavior.
---
Duplicate comments:
In `@contracts/account/extensions/draft-AccountERC7579.sol`:
- Around line 187-201: The current isValidSignature in AccountERC7579 allows
falling back to _rawSignatureValidation even when a validator module was
present/checked, enabling an unwrapped native signature after ERC-7739 rejects
it; change the logic so that if _extractSignatureValidator finds a module and
isModuleInstalled(MODULE_TYPE_VALIDATOR, ...) is true then you must treat a
failing validator call (either revert or non-matching magic) as an explicit
rejection and return bytes4(0xffffffff) instead of falling back to
_rawSignatureValidation; only allow fallback to _rawSignatureValidation when no
validator module was found/installed. Update the code paths around
isValidSignature, _extractSignatureValidator, isModuleInstalled and the
try/catch of IERC7579Validator.isValidSignatureWithSender to implement this
behavior and still return IERC1271.isValidSignature.selector on explicit
validator success.
---
Nitpick comments:
In `@test/account/extensions/AccountERC7579.test.js`:
- Around line 29-32: The test currently only creates either a module-only
account (AccountERC7579Mock) or a native-only account
(AccountERC7579NativeValidationMock) so it never exercises validator→native
fallback or precedence; update the test around helper.newAccount to add a case
that enables both a validator module and a native signer on the same account
(e.g. deploy the module validator via AccountERC7579Mock and also add
signer.address as a native signer), then add assertions invoking
isValidSignature on that combined account to verify the validator is preferred
when it returns a value and that a reverting/missing validator call falls back
to the native signer behavior (simulate validator revert or remove validator and
confirm native signature is accepted). Ensure you reference helper.newAccount,
AccountERC7579Mock, AccountERC7579NativeValidationMock, and isValidSignature
when implementing the new combined-account test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c1e346e7-6276-4389-8abc-52857bc0ec9a
📒 Files selected for processing (4)
.changeset/modern-taxes-exist.mdcontracts/account/extensions/draft-AccountERC7579.solcontracts/mocks/account/AccountMock.soltest/account/extensions/AccountERC7579.test.js
| 'openzeppelin-solidity': patch | ||
| --- | ||
|
|
||
| `AccountERC7579`: Fallback to native validation if module validation fails |
There was a problem hiding this comment.
Narrow the changeset wording to the implemented cases.
This only falls back when no installed validator handles the signature or the validator call reverts; an installed validator returning 0xffffffff still short-circuits as invalid. The current note reads broader than the implementation.
✏️ Suggested wording
-`AccountERC7579`: Fallback to native validation if module validation fails
+`AccountERC7579`: Fallback to native validation when module validation is unavailable or reverts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `AccountERC7579`: Fallback to native validation if module validation fails | |
| `AccountERC7579`: Fallback to native validation when module validation is unavailable or reverts |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/modern-taxes-exist.md at line 5, Update the changeset text for
`AccountERC7579` to precisely describe the implemented behavior: state that the
contract falls back to native signature validation only when no installed
validator handles the signature or when a validator call reverts, and explicitly
note that an installed validator returning `0xffffffff` still short-circuits and
is treated as invalid; replace the broad phrase "if module validation fails"
with this narrower, exact behavior.
2 participants
Fixes L-40.
PR Checklist
npx changeset add)