From 23126573af17806056c06670255a2ad165034c45 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Wed, 18 Oct 2023 10:28:21 +0300 Subject: [PATCH] :art: fixes upon review --- .../interfaces/modules/IMultiOwnedECDSAModule.sol | 1 + .../smart-account/modules/MultiOwnedECDSAModule.sol | 10 +++++++++- .../module/MultiOwnedECDSA.Module.specs.ts | 1 - 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/contracts/smart-account/interfaces/modules/IMultiOwnedECDSAModule.sol b/contracts/smart-account/interfaces/modules/IMultiOwnedECDSAModule.sol index 1405a413..8220b986 100644 --- a/contracts/smart-account/interfaces/modules/IMultiOwnedECDSAModule.sol +++ b/contracts/smart-account/interfaces/modules/IMultiOwnedECDSAModule.sol @@ -25,6 +25,7 @@ interface IMultiOwnedECDSAModule { error NotEOA(address account); error ZeroAddressNotAllowedAsOwner(); error OwnerAlreadyUsedForSmartAccount(address owner, address smartAccount); + error NotAnOwner(address owner, address smartAccount); /** * @dev Initializes the module for a Smart Account. diff --git a/contracts/smart-account/modules/MultiOwnedECDSAModule.sol b/contracts/smart-account/modules/MultiOwnedECDSAModule.sol index 6963a670..77511828 100644 --- a/contracts/smart-account/modules/MultiOwnedECDSAModule.sol +++ b/contracts/smart-account/modules/MultiOwnedECDSAModule.sol @@ -73,6 +73,10 @@ contract MultiOwnedECDSAModule is if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner(); if (owner == newOwner) revert OwnerAlreadyUsedForSmartAccount(newOwner, msg.sender); + if (!_smartAccountOwners[owner][msg.sender]) + revert NotAnOwner(owner, msg.sender); + if (_smartAccountOwners[newOwner][msg.sender]) + revert OwnerAlreadyUsedForSmartAccount(newOwner, msg.sender); _transferOwnership(msg.sender, owner, newOwner); } @@ -84,11 +88,15 @@ contract MultiOwnedECDSAModule is revert OwnerAlreadyUsedForSmartAccount(owner, msg.sender); _smartAccountOwners[owner][msg.sender] = true; - numberOfOwners[msg.sender]++; + unchecked { + ++numberOfOwners[msg.sender]; + } } /// @inheritdoc IMultiOwnedECDSAModule function removeOwner(address owner) external override { + if(!_smartAccountOwners[owner][msg.sender]) + revert NotAnOwner(owner, msg.sender); _transferOwnership(msg.sender, owner, address(0)); --numberOfOwners[msg.sender]; } diff --git a/test/bundler-integration/module/MultiOwnedECDSA.Module.specs.ts b/test/bundler-integration/module/MultiOwnedECDSA.Module.specs.ts index 2fe5f40b..92f3a7a4 100644 --- a/test/bundler-integration/module/MultiOwnedECDSA.Module.specs.ts +++ b/test/bundler-integration/module/MultiOwnedECDSA.Module.specs.ts @@ -149,7 +149,6 @@ describe("MultiOwned ECDSA Module (with Bundler):", async () => { describe("transferOwnership: ", async () => { it("Call transferOwnership from userSA and it successfully changes owner ", async () => { const { multiOwnedECDSAModule, entryPoint, userSA } = await setupTests(); - // console.log(await userSA.getImplementation()); // Calldata to set Eve as owner const txnData1 = multiOwnedECDSAModule.interface.encodeFunctionData(