fix(MultisigBase): gas usage optimizations

contracts/auth/MultisigBase.sol

@@ -42,37 +42,7 @@ contract MultisigBase is IMultisigBase {
      * @dev Given the early void return, this modifier should be used with care on functions that return data.
      */
     modifier onlySigners() {
-        if (!signers.isSigner[msg.sender]) revert NotSigner();
-
-        bytes32 topic = keccak256(msg.data);
-        Voting storage voting = votingPerTopic[signerEpoch][topic];
-
-        // Check that signer has not voted, then record that they have voted.
-        if (voting.hasVoted[msg.sender]) revert AlreadyVoted();
-
-        voting.hasVoted[msg.sender] = true;
-
-        // Determine the new vote count.
-        uint256 voteCount = voting.voteCount + 1;
-
-        // Do not proceed with operation execution if insufficient votes.
-        if (voteCount < signers.threshold) {
-            // Save updated vote count.
-            voting.voteCount = voteCount;
-            return;
-        }
-
-        // Clear vote count and voted booleans.
-        voting.voteCount = 0;
-
-        uint256 count = signers.accounts.length;
-
-        for (uint256 i; i < count; ++i) {
-            voting.hasVoted[signers.accounts[i]] = false;
-        }
-
-        emit MultisigOperationExecuted(topic);
-
+        if (_onlySigners()) return;
         _;
     }
 
@@ -117,15 +87,7 @@ contract MultisigBase is IMultisigBase {
      * @return uint256 indicating the number of votes for a topic
      */
     function getSignerVotesCount(bytes32 topic) external view override returns (uint256) {
-        uint256 length = signers.accounts.length;
-        uint256 voteCount;
-        for (uint256 i; i < length; ++i) {
-            if (votingPerTopic[signerEpoch][topic].hasVoted[signers.accounts[i]]) {
-                voteCount++;
-            }
-        }
-
-        return voteCount;
+        return votingPerTopic[signerEpoch][topic].voteCount;
     }
 
     /***********\
@@ -151,7 +113,7 @@ contract MultisigBase is IMultisigBase {
 
         // Clean up old signers.
         for (uint256 i; i < length; ++i) {
-            signers.isSigner[signers.accounts[i]] = false;
+            delete signers.isSigner[signers.accounts[i]];
         }
 
         length = newAccounts.length;
@@ -177,4 +139,42 @@ contract MultisigBase is IMultisigBase {
 
         emit SignersRotated(newAccounts, newThreshold);
     }
+
+    /**
+     * @dev Internal function that implements onlySigners logic
+     */
+    function _onlySigners() internal returns (bool) {
+        if (!signers.isSigner[msg.sender]) revert NotSigner();
+
+        bytes32 topic = keccak256(msg.data);
+        Voting storage voting = votingPerTopic[signerEpoch][topic];
+
+        // Check that signer has not voted, then record that they have voted.
+        if (voting.hasVoted[msg.sender]) revert AlreadyVoted();
+
+        voting.hasVoted[msg.sender] = true;
+
+        // Determine the new vote count.
+        uint256 voteCount = voting.voteCount + 1;
+
+        // Do not proceed with operation execution if insufficient votes.
+        if (voteCount < signers.threshold) {
+            // Save updated vote count.
+            voting.voteCount = voteCount;
+            return true;
+        }
+
+        // Clear vote count and voted booleans.
+        delete voting.voteCount;
+
+        uint256 count = signers.accounts.length;
+
+        for (uint256 i; i < count; ++i) {
+            delete voting.hasVoted[signers.accounts[i]];
+        }
+
+        emit MultisigOperationExecuted(topic);
+
+        return false;
+    }
 }

contracts/governance/InterchainGovernance.sol

@@ -39,6 +39,10 @@ contract InterchainGovernance is AxelarExecutable, TimeLock, Caller, IInterchain
         string memory governanceAddress_,
         uint256 minimumTimeDelay
     ) AxelarExecutable(gateway) TimeLock(minimumTimeDelay) {
+        if (bytes(governanceChain_).length == 0 || bytes(governanceAddress_).length == 0) {
+            revert InvalidAddress();
+        }
+
         governanceChain = governanceChain_;
         governanceAddress = governanceAddress_;
         governanceChainHash = keccak256(bytes(governanceChain_));

test/InterchainGovernance.js

@@ -46,6 +46,20 @@ describe('InterchainGovernance', () => {
         calldata = targetInterface.encodeFunctionData('callTarget');
     });
 
+    it('should revert on invalid constructor args', async () => {
+        await expect(
+            interchainGovernanceFactory.deploy(AddressZero, governanceChain, governanceAddress.address, timeDelay),
+        ).to.be.revertedWithCustomError(interchainGovernance, 'InvalidAddress');
+
+        await expect(
+            interchainGovernanceFactory.deploy(gatewayAddress.address, '', governanceAddress.address, timeDelay),
+        ).to.be.revertedWithCustomError(interchainGovernance, 'InvalidAddress');
+
+        await expect(
+            interchainGovernanceFactory.deploy(gatewayAddress.address, governanceChain, '', timeDelay),
+        ).to.be.revertedWithCustomError(interchainGovernance, 'InvalidAddress');
+    });
+
     it('should revert on invalid command', async () => {
         const commandID = 2;
         const target = targetContract.address;

test/auth/MultisigBase.js

@@ -182,5 +182,16 @@ describe('MultisigBase', () => {
             .withArgs(msgDataHash)
             .and.to.emit(multiSig, 'SignersRotated')
             .withArgs(rotatedAccounts, newThreshold);
+
+        expect(await multiSig.signerThreshold()).to.equal(newThreshold);
+        expect(await multiSig.signerAccounts()).to.deep.equal(rotatedAccounts);
+
+        for (const signer of rotatedAccounts) {
+            expect(await multiSig.isSigner(signer)).to.equal(true);
+        }
+
+        for (const signer of initAccounts) {
+            expect(await multiSig.isSigner(signer)).to.equal(false);
+        }
     });
 });