Findings 19: enforce number of owners to be >= threshold

src/verifier/UserOpMultiSigVerifier.sol

@@ -10,9 +10,9 @@ import {OnlyKeystore} from "../lib/OnlyKeystore.sol";
 
 contract UserOpMultiSigVerifier is IVerifier, OnlyKeystore {
     error ZeroThresholdNotAllowed();
-    error MaxOwnersLimitExceeded();
-    error MaxSignaturesExceeded();
+    error InvalidNumberOfOwners();
     error OwnersUnsortedOrHasDuplicates();
+    error MaxSignaturesExceeded();
 
     bytes1 public constant SIGNATURES_ONLY_TAG = 0xff;
 
@@ -36,9 +36,10 @@ contract UserOpMultiSigVerifier is IVerifier, OnlyKeystore {
      * @param config The node configuration, expected to be abi.encoded as
      * (uint8 threshold, address[] owners).
      * The threshold is the minimum number of owner signatures required to pass
-     * validation.
-     * The owners array is all the valid signers on the multisig. It MUST be sorted
-     * in ascending order for efficient duplicate detection.
+     * validation. It MUST be greater than 0.
+     * The owners array is all the valid signers on the multisig. It MUST be greater
+     * than or equal to the threshold AND be sorted in ascending order for efficient
+     * duplicate detection.
      * @return validationData Returns SIG_VALIDATION_SUCCESS (0) if ok, otherwise
      * SIG_VALIDATION_FAILED (1).
      */
@@ -51,7 +52,7 @@ contract UserOpMultiSigVerifier is IVerifier, OnlyKeystore {
     {
         (uint8 threshold, address[] memory owners) = abi.decode(config, (uint8, address[]));
         require(threshold > 0, ZeroThresholdNotAllowed());
-        require(owners.length <= type(uint8).max, MaxOwnersLimitExceeded());
+        require(owners.length >= threshold && owners.length <= type(uint8).max, InvalidNumberOfOwners());
         _requireSortedAndUnique(owners);
 
         SignerData[] memory signatures;
@@ -89,7 +90,6 @@ contract UserOpMultiSigVerifier is IVerifier, OnlyKeystore {
      */
     function _requireSortedAndUnique(address[] memory owners) internal pure {
         uint256 length = owners.length;
-        if (length == 0) return;
         for (uint256 i = 1; i < length; i++) {
             require(owners[i] > owners[i - 1], OwnersUnsortedOrHasDuplicates());
         }

test/verifier/UserOpMultiSigVerifier.t.sol

@@ -82,6 +82,26 @@ contract UserOpMultiSigVerifierTest is Test {
         verifier.validateData(message, data, config);
     }
 
+    function testFuzz_validateDataMinOwners(bool withUserOp, uint8 threshold, uint8 size) public {
+        vm.assume(threshold > 0 && size < threshold);
+        Signer[] memory signers = _createSigners(size);
+
+        bytes32 message = keccak256("Signed by signer");
+        bytes memory data = _createData(message, size, 0, signers);
+        if (withUserOp) {
+            PackedUserOperation memory userOp;
+            userOp.signature = data;
+            data = abi.encode(userOp);
+        } else {
+            data = abi.encodePacked(verifier.SIGNATURES_ONLY_TAG(), data);
+        }
+
+        bytes memory config = _createConfig(threshold, signers);
+
+        vm.expectRevert(UserOpMultiSigVerifier.InvalidNumberOfOwners.selector);
+        verifier.validateData(message, data, config);
+    }
+
     function testFuzz_validateDataMaxOwners(bool withUserOp, uint8 threshold, uint8 offset, uint8 excess) public {
         uint16 size = _getSizeAndAssumeMaxOwnerLimitExceeded(threshold, offset, excess);
         Signer[] memory signers = _createSigners(size);
@@ -98,7 +118,7 @@ contract UserOpMultiSigVerifierTest is Test {
 
         bytes memory config = _createConfig(threshold, signers);
 
-        vm.expectRevert(UserOpMultiSigVerifier.MaxOwnersLimitExceeded.selector);
+        vm.expectRevert(UserOpMultiSigVerifier.InvalidNumberOfOwners.selector);
         verifier.validateData(message, data, config);
     }
 
@@ -151,23 +171,26 @@ contract UserOpMultiSigVerifierTest is Test {
         verifier.validateData(message, data, config);
     }
 
-    function testFuzz_validateDataDuplicateSignatures(bool withUserOp, uint8 threshold, uint8 dup) public {
+    function testFuzz_validateDataDuplicateSignatures(
+        bool withUserOp,
+        uint8 threshold,
+        uint8 offset,
+        uint8 size,
+        uint8 dup
+    ) public {
         // Note: set threshold > 1 to show we can't recycle the same signature
         // multiple times.
-        vm.assume(threshold > 1);
-        vm.assume(dup >= 1);
+        vm.assume(threshold > 1 && dup > 1 && dup <= threshold);
+        _assume(threshold, offset, size);
+        Signer[] memory signers = _createSigners(size);
 
-        Signer[] memory signers = _createSigners(1);
         bytes32 message = keccak256("Signed by signer");
-        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signers[0].pk, message);
-        bytes memory signature = abi.encodePacked(r, s, v);
-
-        UserOpMultiSigVerifier.SignerData[] memory sd = new UserOpMultiSigVerifier.SignerData[](dup);
-        for (uint8 i = 0; i < dup; i++) {
-            sd[i] = UserOpMultiSigVerifier.SignerData({index: 0, signature: signature});
+        bytes memory data = _createData(message, threshold, offset, signers);
+        UserOpMultiSigVerifier.SignerData[] memory sd = abi.decode(data, (UserOpMultiSigVerifier.SignerData[]));
+        for (uint8 i; i < dup; i++) {
+            sd[i] = sd[0];
         }
-
-        bytes memory data = abi.encode(sd);
+        data = abi.encode(sd);
         if (withUserOp) {
             PackedUserOperation memory userOp;
             userOp.signature = data;
@@ -287,7 +310,7 @@ contract UserOpMultiSigVerifierTest is Test {
             (uint8 v, bytes32 r, bytes32 s) = vm.sign(signers[index].pk, message);
             sd[i] = UserOpMultiSigVerifier.SignerData({
                 // Note: index will overflow back to 0 after max uint8.
-                // This is ok since a MaxOwnersLimitExceeded() error is expected.
+                // This is ok since an InvalidNumberOfOwners() error is expected.
                 index: uint8(index),
                 signature: abi.encodePacked(r, s, v)
             });