fix(account): revert onInstall when module already installed with different config#229
Conversation
…ferent config - ERC7579Signature: revert with ERC7579SignatureAlreadyInstalled when a different signer is already set; allow idempotent reinstall with same data - ERC7579Multisig: revert with ERC7579MultisigAlreadyInstalled when signers already configured - Update tests for both modules to expect revert instead of no-op Fixes OpenZeppelin#192
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughUpdated module installation behavior across two ERC7579 modules to revert on repeated installation attempts instead of silently succeeding. Added custom errors and validation logic to enforce single-installation semantics, with an exception in the signature module allowing idempotent calls with identical signer data. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/account/modules/ERC7579Signature.sol (1)
38-52:⚠️ Potential issue | 🟡 MinorStale docstring and unnecessary re-
_setSigneron idempotent install.
Lines 41-42 still say "Future installations will behave as a no-op", but the new behavior is: revert on mismatched data, otherwise (re)set. Please update the NOTE to reflect the revert + idempotent-same-data semantics (mirroring the NOTE you added in
ERC7579Multisig.sol).When
currentSigner.length != 0andkeccak256(currentSigner) == keccak256(data), you still call_setSigner, which re-emitsERC7579SignatureSignerSetwith identical data. That is noisy for indexers observing install events and performs a redundant SSTORE. Consider short-circuiting when the signer is already equal.Proposed change
/** * `@dev` See {IERC7579Module-onInstall}. * - * NOTE: An account can only call onInstall once. If called directly by the account, - * the signer will be set to the provided data. Future installations will behave as a no-op. + * NOTE: On first install, the signer is set to the provided `data`. Subsequent calls are + * idempotent only when `data` matches the currently installed signer; otherwise they + * revert with {ERC7579SignatureAlreadyInstalled}. */ function onInstall(bytes calldata data) public virtual { bytes memory currentSigner = signer(msg.sender); - require( - currentSigner.length == 0 || keccak256(currentSigner) == keccak256(data), - ERC7579SignatureAlreadyInstalled() - ); - require(data.length >= 20, ERC7579SignatureInvalidSignerLength()); - _setSigner(msg.sender, data); + if (currentSigner.length == 0) { + require(data.length >= 20, ERC7579SignatureInvalidSignerLength()); + _setSigner(msg.sender, data); + } else { + require(keccak256(currentSigner) == keccak256(data), ERC7579SignatureAlreadyInstalled()); + // idempotent: no-op for identical data, no redundant event/SSTORE + } }Also consider changing line 30's
@noticeto@devfor consistency with the surrounding error docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/modules/ERC7579Signature.sol` around lines 38 - 52, Update the onInstall doc comment NOTE to match the actual semantics (revert on mismatched data and idempotently accept identical data, mirroring ERC7579Multisig.sol) and change the earlier `@notice` tag to `@dev` for consistency; then modify the onInstall function so that if signer(msg.sender) exists and keccak256(currentSigner) == keccak256(data) you short-circuit and return without calling _setSigner to avoid redundant SSTORE and duplicate ERC7579SignatureSignerSet events — otherwise keep the existing require checks and call _setSigner as before.contracts/account/modules/ERC7579Multisig.sol (1)
71-78:⚠️ Potential issue | 🔴 Critical
ERC7579MultisigWeighted.onInstallbreaks with the new revert-on-reinstall semantics.The base class change at commit 1489673 switched from conditional no-op (
if (getSignerCount == 0)) to an unconditional revert (require(getSignerCount == 0)). The weighted subclass still callssuper.onInstall(initData)directly after capturing theinstalledflag, causing it to revert whenever the account already has signers. This makes the!installedguard and weight-setting branch unreachable on reinstall attempts.The test at lines 112–125 reflects the old behavior (expecting a no-op on second install) but does not expect or handle the new revert. Update
ERC7579MultisigWeighted.onInstallto return early wheninstalled, or document this as a breaking change and update tests accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/modules/ERC7579Multisig.sol` around lines 71 - 78, ERC7579MultisigWeighted.onInstall still calls super.onInstall(initData) after checking an `installed` flag, but the base ERC7579Multisig.onInstall now unconditionally reverts when signers exist; update ERC7579MultisigWeighted.onInstall to check installation state via getSignerCount(msg.sender) (or reuse the existing `installed` local) and return early if already installed before calling super.onInstall, or alternatively call super.onInstall only when not installed and perform weight-setting only in that non-installed branch so the reinstall path no longer triggers the base-class require.
🧹 Nitpick comments (2)
test/account/modules/ERC7579Multisig.test.js (1)
98-103: LGTM — good coverage of the new revert semantics.The assertion correctly exercises the new branch by passing different signer/threshold bytes after a successful install.
Minor nit (pre-existing):
installDataat line 40 encodesthresholdasuint256while the contract decodes(bytes[], uint64). This works today because ABI tail values foruint64/uint256are both 32 bytes, but it is technically a type mismatch — consider aligning touint64in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/account/modules/ERC7579Multisig.test.js` around lines 98 - 103, The test encodes installData using uint256 while the contract decodes (bytes[], uint64), so update the ABI encoding used when constructing installData (the value passed into this.mockFromAccount.onInstall and referenced in the test) to use 'uint64' instead of 'uint256' (or otherwise cast the threshold to a 64-bit value) so the encoded types match the contract decoder; adjust the call site that builds installData and any helper that generates that encoded payload to use ['bytes[]','uint64'] (or an explicit 64-bit value) to eliminate the type mismatch.test/account/modules/ERC7579SignatureValidator.test.js (1)
73-99: LGTM — the three new cases cleanly cover first-install, idempotent re-install, and mismatched re-install.One optional addition: add a case that calls
onInstallwith the same signer twice and asserts that the second call does not emitERC7579SignatureSignerSet(or alternatively, does emit — whichever semantics you land on after the_setSignershort-circuit suggestion onERC7579Signature.sol). That locks the event-emission contract for re-installs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/account/modules/ERC7579SignatureValidator.test.js` around lines 73 - 99, Add a test that calls this.mockFromAccount.onInstall(signerData) twice and asserts the second call does not (or does, depending on chosen semantics) emit the ERC7579SignatureSignerSet event: use the existing signerData, mockFromAccount and mockAccount identifiers, call onInstall once to set the signer then call it a second time and assert event emission behavior with the ERC7579SignatureSignerSet event name (or invert the assertion if you decided to keep emission on idempotent installs); ensure the test checks signer remains unchanged via this.mock.signer(this.mockAccount.address) as in the other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/account/modules/ERC7579Signature.sol`:
- Around line 46-50: The current require ordering can misreport errors: validate
input length first by checking data.length >= 20 and revert with
ERC7579SignatureInvalidSignerLength() before enforcing the install/equality
guard; then perform the equality/install check using currentSigner and
keccak256(data) and revert with ERC7579SignatureAlreadyInstalled() if needed
(swap the two require statements involving data, currentSigner,
ERC7579SignatureAlreadyInstalled, and ERC7579SignatureInvalidSignerLength).
---
Outside diff comments:
In `@contracts/account/modules/ERC7579Multisig.sol`:
- Around line 71-78: ERC7579MultisigWeighted.onInstall still calls
super.onInstall(initData) after checking an `installed` flag, but the base
ERC7579Multisig.onInstall now unconditionally reverts when signers exist; update
ERC7579MultisigWeighted.onInstall to check installation state via
getSignerCount(msg.sender) (or reuse the existing `installed` local) and return
early if already installed before calling super.onInstall, or alternatively call
super.onInstall only when not installed and perform weight-setting only in that
non-installed branch so the reinstall path no longer triggers the base-class
require.
In `@contracts/account/modules/ERC7579Signature.sol`:
- Around line 38-52: Update the onInstall doc comment NOTE to match the actual
semantics (revert on mismatched data and idempotently accept identical data,
mirroring ERC7579Multisig.sol) and change the earlier `@notice` tag to `@dev` for
consistency; then modify the onInstall function so that if signer(msg.sender)
exists and keccak256(currentSigner) == keccak256(data) you short-circuit and
return without calling _setSigner to avoid redundant SSTORE and duplicate
ERC7579SignatureSignerSet events — otherwise keep the existing require checks
and call _setSigner as before.
---
Nitpick comments:
In `@test/account/modules/ERC7579Multisig.test.js`:
- Around line 98-103: The test encodes installData using uint256 while the
contract decodes (bytes[], uint64), so update the ABI encoding used when
constructing installData (the value passed into this.mockFromAccount.onInstall
and referenced in the test) to use 'uint64' instead of 'uint256' (or otherwise
cast the threshold to a 64-bit value) so the encoded types match the contract
decoder; adjust the call site that builds installData and any helper that
generates that encoded payload to use ['bytes[]','uint64'] (or an explicit
64-bit value) to eliminate the type mismatch.
In `@test/account/modules/ERC7579SignatureValidator.test.js`:
- Around line 73-99: Add a test that calls
this.mockFromAccount.onInstall(signerData) twice and asserts the second call
does not (or does, depending on chosen semantics) emit the
ERC7579SignatureSignerSet event: use the existing signerData, mockFromAccount
and mockAccount identifiers, call onInstall once to set the signer then call it
a second time and assert event emission behavior with the
ERC7579SignatureSignerSet event name (or invert the assertion if you decided to
keep emission on idempotent installs); ensure the test checks signer remains
unchanged via this.mock.signer(this.mockAccount.address) as in the other cases.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f765e58-80fc-4be3-82ee-dc4d5898d138
📒 Files selected for processing (4)
contracts/account/modules/ERC7579Multisig.solcontracts/account/modules/ERC7579Signature.soltest/account/modules/ERC7579Multisig.test.jstest/account/modules/ERC7579SignatureValidator.test.js
1 participant
Summary
Fixes #192
The
onInstallfunction in bothERC7579SignatureandERC7579Multisigpreviously did a silent no-op when called on an already-installed module.
This is dangerous because the caller may expect the new config to be active,
leading to silent misconfigurations.
Changes
ERC7579SignatureERC7579SignatureAlreadyInstallederroronInstallnow reverts if a different signer is already setonInstallstill validates signer length (>= 20 bytes)ERC7579MultisigERC7579MultisigAlreadyInstallederroronInstallnow reverts if signers are already configuredTests
ERC7579SignatureValidator.test.js: replaced no-op test with 3 targeted testsERC7579Multisig.test.js: updated expectation from no-op to revertTest Results
Summary by CodeRabbit
Bug Fixes
Tests