fix: ensure no duplicate signatures verified for `AbstractWeightedMultisigIsm.verify()` (#4468)

### Description

- There's a bug in the verify function of `AbstractWeightedMultisigIsm`
that an attacker might use to bypass the verification of multiple
signatures. The code tried to check the duplication of the signers if
not found, but the code does not increment validatorIndex when the
recovered signer matches to the stored signer. For instance:

- validatorsAndThresholdWeight returns [A, B, C, D]
- an attacker uses signatures as [sig from A, sig from A, sig from A,
sig from A, ...] or [sig from B, sig from B, …]

Fix is to add a ++validatorIndex at the end of the for loop implies we
don't allow the next signer to be the same as the signer we just
verified.

### Drive-by changes

None

### Related issues

From Chainlight's audit findings

### Backward compatibility

We haven't deployed these contracts yet on testnet/mainnet

### Testing

Fuzz testing

solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol

@@ -73,11 +73,14 @@ abstract contract AbstractStaticWeightedMultisigIsm is
 
         // assumes that signatures are ordered by validator
         for (
-            uint256 i = 0;
-            _totalWeight < _thresholdWeight && i < _validatorCount;
-            ++i
+            uint256 signatureIndex = 0;
+            _totalWeight < _thresholdWeight && signatureIndex < _validatorCount;
+            ++signatureIndex
         ) {
-            address _signer = ECDSA.recover(_digest, signatureAt(_metadata, i));
+            address _signer = ECDSA.recover(
+                _digest,
+                signatureAt(_metadata, signatureIndex)
+            );
             // loop through remaining validators until we find a match
             while (
                 _validatorIndex < _validatorCount &&
@@ -90,6 +93,7 @@ abstract contract AbstractStaticWeightedMultisigIsm is
 
             // add the weight of the current validator
             _totalWeight += _validators[_validatorIndex].weight;
+            ++_validatorIndex;
         }
         require(
             _totalWeight >= _thresholdWeight,

solidity/test/isms/MultisigIsm.t.sol

@@ -186,6 +186,32 @@ abstract contract AbstractMultisigIsmTest is Test {
         metadata[index] = ~metadata[index];
         assertFalse(ism.verify(metadata, message));
     }
+
+    function test_verify_revertWhen_duplicateSignatures(
+        uint32 destination,
+        bytes32 recipient,
+        bytes calldata body,
+        uint8 m,
+        uint8 n,
+        bytes32 seed
+    ) public virtual {
+        vm.assume(1 < m && m <= n && n < 10);
+        bytes memory message = getMessage(destination, recipient, body);
+        bytes memory metadata = getMetadata(m, n, seed, message);
+
+        bytes memory duplicateMetadata = new bytes(metadata.length);
+        for (uint256 i = 0; i < metadata.length - 65; i++) {
+            duplicateMetadata[i] = metadata[i];
+        }
+        for (uint256 i = 0; i < 65; i++) {
+            duplicateMetadata[metadata.length - 65 + i] = metadata[
+                metadata.length - 130 + i
+            ];
+        }
+
+        vm.expectRevert("!threshold");
+        ism.verify(duplicateMetadata, message);
+    }
 }
 
 contract MerkleRootMultisigIsmTest is AbstractMultisigIsmTest {

solidity/test/isms/WeightedMultisigIsm.t.sol

@@ -65,7 +65,6 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is
             }
         }
 
-        // ism = IInterchainSecurityModule(deployedIsm);
         ism = IInterchainSecurityModule(
             weightedFactory.deploy(validators, threshold)
         );
@@ -136,7 +135,7 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is
         return metadata;
     }
 
-    function testVerify_revertInsufficientWeight(
+    function test_verify_revertInsufficientWeight(
         uint32 destination,
         bytes32 recipient,
         bytes calldata body,
@@ -161,6 +160,34 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is
         vm.expectRevert("Insufficient validator weight");
         ism.verify(insufficientMetadata, message);
     }
+
+    function test_verify_revertWhen_duplicateSignatures(
+        uint32 destination,
+        bytes32 recipient,
+        bytes calldata body,
+        uint8 m,
+        uint8 n,
+        bytes32 seed
+    ) public virtual override {
+        vm.assume(1 < m && m <= n && n < 10);
+        bytes memory message = getMessage(destination, recipient, body);
+        bytes memory metadata = getMetadata(m, n, seed, message);
+
+        bytes memory duplicateMetadata = new bytes(metadata.length);
+        for (uint256 i = 0; i < metadata.length - 65; i++) {
+            duplicateMetadata[i] = metadata[i];
+        }
+        for (uint256 i = 0; i < 65; i++) {
+            duplicateMetadata[metadata.length - 65 + i] = metadata[
+                metadata.length - 130 + i
+            ];
+        }
+
+        if (weightedIsm.signatureCount(metadata) >= 2) {
+            vm.expectRevert("Invalid signer");
+            ism.verify(duplicateMetadata, message);
+        }
+    }
 }
 
 contract StaticMerkleRootWeightedMultisigIsmTest is
@@ -194,6 +221,28 @@ contract StaticMerkleRootWeightedMultisigIsmTest is
                 message
             );
     }
+
+    function test_verify_revertWhen_duplicateSignatures(
+        uint32 destination,
+        bytes32 recipient,
+        bytes calldata body,
+        uint8 m,
+        uint8 n,
+        bytes32 seed
+    )
+        public
+        override(AbstractMultisigIsmTest, AbstractStaticWeightedMultisigIsmTest)
+    {
+        AbstractStaticWeightedMultisigIsmTest
+            .test_verify_revertWhen_duplicateSignatures(
+                destination,
+                recipient,
+                body,
+                m,
+                n,
+                seed
+            );
+    }
 }
 
 contract StaticMessageIdWeightedMultisigIsmTest is
@@ -227,4 +276,26 @@ contract StaticMessageIdWeightedMultisigIsmTest is
                 message
             );
     }
+
+    function test_verify_revertWhen_duplicateSignatures(
+        uint32 destination,
+        bytes32 recipient,
+        bytes calldata body,
+        uint8 m,
+        uint8 n,
+        bytes32 seed
+    )
+        public
+        override(AbstractMultisigIsmTest, AbstractStaticWeightedMultisigIsmTest)
+    {
+        AbstractStaticWeightedMultisigIsmTest
+            .test_verify_revertWhen_duplicateSignatures(
+                destination,
+                recipient,
+                body,
+                m,
+                n,
+                seed
+            );
+    }
 }