From 14896739cb21b11f94f4f9a421de279a964ecca9 Mon Sep 17 00:00:00 2001 From: rahul-0407 Date: Sat, 18 Apr 2026 00:47:46 +0530 Subject: [PATCH 1/2] fix(account): revert onInstall when module already installed with different 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 #192 --- contracts/account/modules/ERC7579Multisig.sol | 11 +++++--- .../account/modules/ERC7579Signature.sol | 13 +++++++-- test/account/modules/ERC7579Multisig.test.js | 14 ++++------ .../modules/ERC7579SignatureValidator.test.js | 28 +++++++++++++++---- 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/contracts/account/modules/ERC7579Multisig.sol b/contracts/account/modules/ERC7579Multisig.sol index 7efc8dfd..338e19d1 100644 --- a/contracts/account/modules/ERC7579Multisig.sol +++ b/contracts/account/modules/ERC7579Multisig.sol @@ -48,6 +48,9 @@ abstract contract ERC7579Multisig is ERC7579Validator { /// @dev The `threshold` is unreachable given the number of `signers`. error ERC7579MultisigUnreachableThreshold(uint64 signers, uint64 threshold); + /// @dev Thrown when attempting to install while signers are already configured. + error ERC7579MultisigAlreadyInstalled(); + mapping(address account => EnumerableSet.BytesSet) private _signersSetByAccount; mapping(address account => uint64) private _thresholdByAccount; @@ -62,12 +65,12 @@ abstract contract ERC7579Multisig is ERC7579Validator { * If no signers or threshold are provided, the multisignature functionality will be * disabled until they are added later. * - * 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: An account can only call onInstall once. Future installations will revert with + * {ERC7579MultisigAlreadyInstalled} if signers are already configured. */ function onInstall(bytes calldata initData) public virtual { - if (initData.length > 32 && getSignerCount(msg.sender) == 0) { - // More than just delay parameter + require(getSignerCount(msg.sender) == 0, ERC7579MultisigAlreadyInstalled()); + if (initData.length > 32) { (bytes[] memory signers_, uint64 threshold_) = abi.decode(initData, (bytes[], uint64)); _addSigners(msg.sender, signers_); _setThreshold(msg.sender, threshold_); diff --git a/contracts/account/modules/ERC7579Signature.sol b/contracts/account/modules/ERC7579Signature.sol index 36d0b5e9..03ba5973 100644 --- a/contracts/account/modules/ERC7579Signature.sol +++ b/contracts/account/modules/ERC7579Signature.sol @@ -27,6 +27,9 @@ contract ERC7579Signature is ERC7579Validator { /// @dev Thrown when the signer length is less than 20 bytes. error ERC7579SignatureInvalidSignerLength(); + /// @notice Thrown when attempting to install a signer while a different signer is already set + error ERC7579SignatureAlreadyInstalled(); + /// @dev Return the ERC-7913 signer (i.e. `verifier || key`). function signer(address account) public view virtual returns (bytes memory) { return _signers[account]; @@ -39,9 +42,13 @@ contract ERC7579Signature is ERC7579Validator { * the signer will be set to the provided data. Future installations will behave as a no-op. */ function onInstall(bytes calldata data) public virtual { - if (signer(msg.sender).length == 0) { - setSigner(data); - } + bytes memory currentSigner = signer(msg.sender); + require( + currentSigner.length == 0 || keccak256(currentSigner) == keccak256(data), + ERC7579SignatureAlreadyInstalled() + ); + require(data.length >= 20, ERC7579SignatureInvalidSignerLength()); + _setSigner(msg.sender, data); } /** diff --git a/test/account/modules/ERC7579Multisig.test.js b/test/account/modules/ERC7579Multisig.test.js index 90f73208..e8d3d0d6 100644 --- a/test/account/modules/ERC7579Multisig.test.js +++ b/test/account/modules/ERC7579Multisig.test.js @@ -95,14 +95,12 @@ describe('ERC7579Multisig', function () { await expect(this.mock.getSigners(this.mockAccount.address, 0, MAX_UINT64)).to.eventually.deep.equal(this.signers); await expect(this.mock.threshold(this.mockAccount.address)).to.eventually.equal(this.threshold); - // onInstall is allowed again but is a noop - await this.mockFromAccount.onInstall( - ethers.AbiCoder.defaultAbiCoder().encode(['bytes[]', 'uint256'], [[signerECDSA3.address], 2]), - ); - - // Should still have the original signers and threshold - await expect(this.mock.getSigners(this.mockAccount.address, 0, MAX_UINT64)).to.eventually.deep.equal(this.signers); - await expect(this.mock.threshold(this.mockAccount.address)).to.eventually.equal(this.threshold); + // onInstall should now revert if already installed + await expect( + this.mockFromAccount.onInstall( + ethers.AbiCoder.defaultAbiCoder().encode(['bytes[]', 'uint256'], [[signerECDSA3.address], 2]), + ), + ).to.be.revertedWithCustomError(this.mock, 'ERC7579MultisigAlreadyInstalled'); }); it('cleans up signers and threshold on uninstallation', async function () { diff --git a/test/account/modules/ERC7579SignatureValidator.test.js b/test/account/modules/ERC7579SignatureValidator.test.js index bee5c3e5..3fb80c01 100644 --- a/test/account/modules/ERC7579SignatureValidator.test.js +++ b/test/account/modules/ERC7579SignatureValidator.test.js @@ -70,14 +70,32 @@ describe('ERC7579Signature', function () { ); }); - it('behaves as a noop when the validator is already installed for an account', async function () { - // First installation should succeed + it('installs signer correctly on first call', async function () { const signerData = ethers.solidityPacked(['address'], [signerECDSA.address]); await expect(this.mockFromAccount.onInstall(signerData)).to.not.be.reverted; + await expect(this.mock.signer(this.mockAccount.address)).to.eventually.equal(signerData); + }); + + it('does not revert when onInstall is called again with the same signer', async function () { + const signerData = ethers.solidityPacked(['address'], [signerECDSA.address]); + await this.mockFromAccount.onInstall(signerData); + + // Same signer — should NOT revert + await expect(this.mockFromAccount.onInstall(signerData)).to.not.be.reverted; - // Second installation should behave as a no-op - await this.mockFromAccount.onInstall(ethers.solidityPacked(['address'], [ethers.Wallet.createRandom().address])); // Not revert - await expect(this.mock.signer(this.mockAccount.address)).to.eventually.equal(signerData); // No change in signers + // Signer unchanged + await expect(this.mock.signer(this.mockAccount.address)).to.eventually.equal(signerData); + }); + + it('reverts when onInstall is called with a different signer already set', async function () { + const signerData = ethers.solidityPacked(['address'], [signerECDSA.address]); + await this.mockFromAccount.onInstall(signerData); + + const differentSigner = ethers.solidityPacked(['address'], [ethers.Wallet.createRandom().address]); + await expect(this.mockFromAccount.onInstall(differentSigner)).to.be.revertedWithCustomError( + this.mock, + 'ERC7579SignatureAlreadyInstalled', + ); }); it('emits event on ERC7579SignatureSignerSet on both installation and uninstallation', async function () { From 09afadb1d6391bdc53751a31cef0117f9cd5d104 Mon Sep 17 00:00:00 2001 From: rahul-0407 Date: Sat, 18 Apr 2026 01:20:40 +0530 Subject: [PATCH 2/2] fix(account): address CodeRabbit review feedback - ERC7579Signature: fix require ordering, update NatSpec, short-circuit on idempotent reinstall - ERC7579MultisigWeighted: return early when already installed to avoid base class revert - Change @notice to @dev on ERC7579SignatureAlreadyInstalled error --- .../modules/ERC7579MultisigWeighted.sol | 3 ++- .../account/modules/ERC7579Signature.sol | 20 ++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/contracts/account/modules/ERC7579MultisigWeighted.sol b/contracts/account/modules/ERC7579MultisigWeighted.sol index 679b3ab7..2ee4d7ef 100644 --- a/contracts/account/modules/ERC7579MultisigWeighted.sol +++ b/contracts/account/modules/ERC7579MultisigWeighted.sol @@ -62,8 +62,9 @@ abstract contract ERC7579MultisigWeighted is ERC7579Multisig { */ function onInstall(bytes calldata initData) public virtual override { bool installed = getSignerCount(msg.sender) > 0; + if (installed) return; super.onInstall(initData); - if (initData.length > 96 && !installed) { + if (initData.length > 96) { (bytes[] memory signers, , uint64[] memory weights) = abi.decode(initData, (bytes[], uint64, uint64[])); _setSignerWeights(msg.sender, signers, weights); } diff --git a/contracts/account/modules/ERC7579Signature.sol b/contracts/account/modules/ERC7579Signature.sol index 03ba5973..b0269966 100644 --- a/contracts/account/modules/ERC7579Signature.sol +++ b/contracts/account/modules/ERC7579Signature.sol @@ -27,7 +27,7 @@ contract ERC7579Signature is ERC7579Validator { /// @dev Thrown when the signer length is less than 20 bytes. error ERC7579SignatureInvalidSignerLength(); - /// @notice Thrown when attempting to install a signer while a different signer is already set + /// @dev Thrown when attempting to install a signer while a different signer is already set error ERC7579SignatureAlreadyInstalled(); /// @dev Return the ERC-7913 signer (i.e. `verifier || key`). @@ -38,17 +38,19 @@ contract ERC7579Signature is ERC7579Validator { /** * @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 + } } /**