Fix submitTransaction & add test code

packages/contracts/contracts/MultiSigWallet.sol

@@ -217,26 +217,21 @@ contract MultiSigWallet is ERC165, IMultiSigWallet {
         // Check if destination is MultiSigWalletFactory
         require(_destination != factoryAddress, "Invalid destination");
 
-        // Get the first 4 bytes of _data
-        bytes4 selector;
-        assembly {
-            // Calculate the length of _data
-            let dataLength := calldataload(0)
-            // Check if _data is long enough to contain a selector
-            if lt(dataLength, 4) {
-                // If _data is too short, revert the transaction
-                revert(0, 0)
+        if (_data.length >= 4) {
+            // Get the first 4 bytes of _data
+            bytes4 selector;
+            assembly {
+                // Load the first 4 bytes of _data (the selector) into the selector variable
+                selector := calldataload(add(_data.offset, 0))
             }
-            // Load the first 4 bytes of _data (the selector) into the selector variable
-            selector := calldataload(4)
-        }
 
-        // Check if data is not calling addMember or removeMember function of MultiSigWalletFactory
-        require(
-            selector != IMultiSigWalletFactory.addMember.selector &&
-                selector != IMultiSigWalletFactory.removeMember.selector,
-            "Invalid function call"
-        );
+            // Check if data is not calling addMember or removeMember function of MultiSigWalletFactory
+            require(
+                selector != IMultiSigWalletFactory.addMember.selector &&
+                    selector != IMultiSigWalletFactory.removeMember.selector,
+                "Invalid function call"
+            );
+        }
 
         uint256 transactionId = addTransaction(_title, _description, _destination, _value, _data);
         _confirmTransaction(transactionId);

packages/contracts/package.json

@@ -19,7 +19,8 @@
         "test:ExecutionAfterRequirementsChanged": "hardhat test test/ExecutionAfterRequirementsChanged.test.ts",
         "test:ExternalCalls": "hardhat test test/ExternalCalls.test.ts",
         "test:Factory": "hardhat test test/Factory.test.ts",
-        "test:MultiSigToken": "hardhat test test/MultiSigToken.test.ts"
+        "test:MultiSigToken": "hardhat test test/MultiSigToken.test.ts",
+        "test:SubmitTransaction.test": "hardhat test test/SubmitTransaction.test.ts"
     },
     "repository": {
         "type": "git",
@@ -33,6 +34,7 @@
     "homepage": "https://github.com/bosagora/multisig-wallet#readme",
     "devDependencies": {
         "@ethersproject/constants": "^5.7.0",
+        "@ethersproject/abi": "^5.7.0",
         "@nomiclabs/hardhat-ethers": "^2.2.3",
         "@nomiclabs/hardhat-waffle": "^2.0.2",
         "@openzeppelin/contracts": "^4.9.5",

packages/contracts/test/SubmitTransaction.test.ts

@@ -0,0 +1,151 @@
+import "@nomiclabs/hardhat-ethers";
+import "@nomiclabs/hardhat-waffle";
+import { ethers } from "hardhat";
+
+import { HardhatAccount } from "../src/HardhatAccount";
+import { MultiSigWallet, MultiSigWalletFactory, TestMultiSigToken } from "../typechain-types";
+
+import assert from "assert";
+import { BigNumber, Wallet } from "ethers";
+import { ContractUtils } from "../src/utils/ContractUtils";
+
+async function deployMultiSigWalletFactory(deployer: Wallet): Promise<MultiSigWalletFactory> {
+    const factory = await ethers.getContractFactory("MultiSigWalletFactory");
+    const contract = (await factory.connect(deployer).deploy()) as MultiSigWalletFactory;
+    await contract.deployed();
+    await contract.deployTransaction.wait();
+    return contract;
+}
+
+async function deployMultiSigWallet(
+    factoryAddress: string,
+    deployer: Wallet,
+    name: string,
+    description: string,
+    owners: string[],
+    required: number
+): Promise<MultiSigWallet | undefined> {
+    const contractFactory = await ethers.getContractFactory("MultiSigWalletFactory");
+    const factoryContract = contractFactory.attach(factoryAddress) as MultiSigWalletFactory;
+
+    const address = await ContractUtils.getEventValueString(
+        await factoryContract.connect(deployer).create(name, description, owners, required),
+        factoryContract.interface,
+        "ContractInstantiation",
+        "wallet"
+    );
+
+    if (address !== undefined) {
+        return (await ethers.getContractFactory("MultiSigWallet")).attach(address) as MultiSigWallet;
+    } else return undefined;
+}
+
+describe("Test for SubmitTransaction - filter factoryAddress, IMultiSigWalletFactory.addMember, IMultiSigWalletFactory.removeMember", () => {
+    const raws = HardhatAccount.keys.map((m) => new Wallet(m, ethers.provider));
+    const [deployer, account0, account1, account2, account3, account4] = raws;
+    const owners1 = [account0, account1, account2];
+
+    let multiSigFactory0: MultiSigWalletFactory;
+    let multiSigFactory1: MultiSigWalletFactory;
+    let multiSigWallet0: MultiSigWallet | undefined;
+    let multiSigWallet1: MultiSigWallet | undefined;
+    const requiredConfirmations = 2;
+
+    before(async () => {
+        multiSigFactory0 = await deployMultiSigWalletFactory(deployer);
+        assert.ok(multiSigFactory0);
+        multiSigFactory1 = await deployMultiSigWalletFactory(deployer);
+        assert.ok(multiSigFactory1);
+    });
+
+    it("Create Wallet 0 by Factory 0", async () => {
+        multiSigWallet0 = await deployMultiSigWallet(
+            multiSigFactory0.address,
+            deployer,
+            "My Wallet 1",
+            "My first multi-sign wallet",
+            owners1.map((m) => m.address),
+            requiredConfirmations
+        );
+        assert.ok(multiSigWallet0);
+
+        assert.deepStrictEqual(
+            await multiSigWallet0.getMembers(),
+            owners1.map((m) => m.address)
+        );
+
+        assert.deepStrictEqual(await multiSigFactory0.getNumberOfWalletsForMember(account0.address), BigNumber.from(1));
+        assert.deepStrictEqual(await multiSigFactory0.getNumberOfWalletsForMember(account1.address), BigNumber.from(1));
+        assert.deepStrictEqual(await multiSigFactory0.getNumberOfWalletsForMember(account2.address), BigNumber.from(1));
+    });
+
+    it("Create Wallet 1 by Factory 1", async () => {
+        multiSigWallet1 = await deployMultiSigWallet(
+            multiSigFactory1.address,
+            deployer,
+            "My Wallet 1",
+            "My first multi-sign wallet",
+            owners1.map((m) => m.address),
+            requiredConfirmations
+        );
+        assert.ok(multiSigWallet1);
+
+        assert.deepStrictEqual(
+            await multiSigWallet1.getMembers(),
+            owners1.map((m) => m.address)
+        );
+
+        assert.deepStrictEqual(await multiSigFactory1.getNumberOfWalletsForMember(account0.address), BigNumber.from(1));
+        assert.deepStrictEqual(await multiSigFactory1.getNumberOfWalletsForMember(account1.address), BigNumber.from(1));
+        assert.deepStrictEqual(await multiSigFactory1.getNumberOfWalletsForMember(account2.address), BigNumber.from(1));
+    });
+
+    it("should reject transaction to factory address", async () => {
+        assert.ok(multiSigWallet0);
+
+        await assert.rejects(
+            multiSigWallet0.submitTransaction("Test Title", "Test Description", multiSigFactory0.address, 0, "0x"),
+            "Invalid destination"
+        );
+    });
+
+    it("should reject addMember function call", async () => {
+        assert.ok(multiSigWallet0);
+
+        const addMemberData = multiSigFactory0.interface.encodeFunctionData("addMember", [
+            account3.address,
+            multiSigWallet0.address,
+        ]);
+
+        await assert.rejects(
+            multiSigWallet0.submitTransaction(
+                "Test Title",
+                "Test Description",
+                multiSigFactory1.address,
+                0,
+                addMemberData
+            ),
+            "Invalid function call"
+        );
+    });
+
+    it("should reject removeMember function call", async () => {
+        assert.ok(multiSigWallet0);
+
+        const addMemberData = multiSigFactory0.interface.encodeFunctionData("removeMember", [
+            account3.address,
+            multiSigWallet0.address,
+        ]);
+
+        await assert.rejects(
+            multiSigWallet0.submitTransaction(
+                "Test Title",
+                "Test Description",
+                multiSigFactory1.address,
+                0,
+                addMemberData
+            ),
+            "Invalid function call"
+        );
+    });
+});