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/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 36d0b5e9..b0269966 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(); + /// @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`). function signer(address account) public view virtual returns (bytes memory) { return _signers[account]; @@ -35,12 +38,18 @@ 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 { - if (signer(msg.sender).length == 0) { - setSigner(data); + bytes memory currentSigner = signer(msg.sender); + 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 } } 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 () {