Merge pull request #189 from bcnmy/feat/SMA-347-MultiOwnedEcdsa-Remed…

…iations Multi owned ecdsa remediations
bcnmy · Jan 16, 2024 · 41403f8 · 41403f8
2 parents ed88551 + c5d20eb
commit 41403f8
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 11 deletions.
diff --git a/contracts/smart-account/modules/MultiOwnedECDSAModule.sol b/contracts/smart-account/modules/MultiOwnedECDSAModule.sol
@@ -29,7 +29,7 @@ contract MultiOwnedECDSAModule is
 {
     using ECDSA for bytes32;
 
-    string public constant NAME = "ECDSA Ownership Registry Module";
+    string public constant NAME = "Multiowned ECDSA Ownership Module";
     string public constant VERSION = "0.2.0";
 
     // owner => smartAccount => isOwner
@@ -45,7 +45,7 @@ contract MultiOwnedECDSAModule is
         }
         uint256 ownersToAdd = eoaOwners.length;
         if (ownersToAdd == 0) revert NoOwnersToAdd();
-        for (uint256 i; i < ownersToAdd; ) {
+        for (uint256 i; i < ownersToAdd; ++i) {
             if (eoaOwners[i] == address(0))
                 revert ZeroAddressNotAllowedAsOwner();
             if (_smartAccountOwners[eoaOwners[i]][msg.sender])
@@ -55,9 +55,7 @@ contract MultiOwnedECDSAModule is
                 );
 
             _smartAccountOwners[eoaOwners[i]][msg.sender] = true;
-            unchecked {
-                ++i;
-            }
+            emit OwnershipTransferred(msg.sender, address(0), eoaOwners[i]);
         }
         numberOfOwners[msg.sender] = ownersToAdd;
         return address(this);
@@ -70,9 +68,9 @@ contract MultiOwnedECDSAModule is
     ) external override {
         if (_isSmartContract(newOwner)) revert NotEOA(newOwner);
         if (newOwner == address(0)) revert ZeroAddressNotAllowedAsOwner();
-        if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
         if (owner == newOwner)
             revert OwnerAlreadyUsedForSmartAccount(newOwner, msg.sender);
+        //address(0) is not an owner initially as it points to the address(0) = false
         if (!_smartAccountOwners[owner][msg.sender])
             revert NotAnOwner(owner, msg.sender);
         if (_smartAccountOwners[newOwner][msg.sender])
@@ -91,6 +89,7 @@ contract MultiOwnedECDSAModule is
         unchecked {
             ++numberOfOwners[msg.sender];
         }
+        emit OwnershipTransferred(msg.sender, address(0), owner);
     }
 
     /// @inheritdoc IMultiOwnedECDSAModule
@@ -101,6 +100,7 @@ contract MultiOwnedECDSAModule is
         unchecked {
             --numberOfOwners[msg.sender];
         }
+        emit OwnershipTransferred(msg.sender, owner, address(0));
     }
 
     /// @inheritdoc IMultiOwnedECDSAModule

diff --git a/contracts/smart-account/modules/PasskeyRegistryModule.sol b/contracts/smart-account/modules/PasskeyRegistryModule.sol
@@ -38,11 +38,7 @@ contract PasskeyRegistryModule is
         if (passKeyId.pubKeyX != 0 && passKeyId.pubKeyY != 0)
             revert AlreadyInitedForSmartAccount(msg.sender);
 
-        smartAccountPasskey[msg.sender] = PassKeyId(
-            _pubKeyX,
-            _pubKeyY,
-            _keyId
-        );
+        smartAccountPasskey[msg.sender] = PassKeyId(_pubKeyX, _pubKeyY, _keyId);
 
         return address(this);
     }

diff --git a/test/bundler-integration/module/MultiOwnedECDSA.Module.specs.ts b/test/bundler-integration/module/MultiOwnedECDSA.Module.specs.ts
@@ -23,6 +23,7 @@ describe("MultiOwned ECDSA Module (with Bundler):", async () => {
   ] = [] as SignerWithAddress[];
   const smartAccountDeploymentIndex = 0;
   const SIG_VALIDATION_SUCCESS = 0;
+  const ADDRESS_ZERO = "0x0000000000000000000000000000000000000000";
   let environment: BundlerTestEnvironment;
 
   before(async function () {
@@ -172,6 +173,32 @@ describe("MultiOwned ECDSA Module (with Bundler):", async () => {
         await multiOwnedECDSAModule.isOwner(userSA.address, eve.address)
       ).to.equal(true);
     });
+
+    it("TransferOwnership from address(0) is not possible", async () => {
+      const { multiOwnedECDSAModule, entryPoint, userSA } = await setupTests();
+
+      // Calldata to set 0x00..00 as owner
+      const txnData1 = multiOwnedECDSAModule.interface.encodeFunctionData(
+        "transferOwnership",
+        [ADDRESS_ZERO, eve.address]
+      );
+      const userOp = await makeEcdsaModuleUserOp(
+        "execute_ncC",
+        [multiOwnedECDSAModule.address, 0, txnData1],
+        userSA.address,
+        smartAccountOwner1, // can be signed by any owner
+        entryPoint,
+        multiOwnedECDSAModule.address,
+        {
+          preVerificationGas: 50000,
+        }
+      );
+
+      await environment.sendUserOperation(userOp, entryPoint.address);
+      expect(
+        await multiOwnedECDSAModule.isOwner(userSA.address, eve.address)
+      ).to.equal(false);
+    });
   });
 
   describe("removeOwner():", async () => {