From 42fe459a0b372b3fff286db14a29f2243d905933 Mon Sep 17 00:00:00 2001 From: hazim-j Date: Thu, 7 Aug 2025 14:53:10 +1000 Subject: [PATCH 1/2] Findings 19: enforce number of owners to be >= threshold --- src/verifier/UserOpMultiSigVerifier.sol | 14 +++--- test/verifier/UserOpMultiSigVerifier.t.sol | 51 ++++++++++++++++------ 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/verifier/UserOpMultiSigVerifier.sol b/src/verifier/UserOpMultiSigVerifier.sol index dca960b..abc5080 100644 --- a/src/verifier/UserOpMultiSigVerifier.sol +++ b/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()); } diff --git a/test/verifier/UserOpMultiSigVerifier.t.sol b/test/verifier/UserOpMultiSigVerifier.t.sol index 4af29a4..1ed2bf3 100644 --- a/test/verifier/UserOpMultiSigVerifier.t.sol +++ b/test/verifier/UserOpMultiSigVerifier.t.sol @@ -82,6 +82,26 @@ contract UserOpMultiSigVerifierTest is Test { verifier.validateData(message, data, config); } + function testFuzz_validateDataNoOwners(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) }); From d4dd126a0ec114076a2d55e23299690e147049c1 Mon Sep 17 00:00:00 2001 From: hazim-j Date: Thu, 7 Aug 2025 14:56:05 +1000 Subject: [PATCH 2/2] clean --- test/verifier/UserOpMultiSigVerifier.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/verifier/UserOpMultiSigVerifier.t.sol b/test/verifier/UserOpMultiSigVerifier.t.sol index 1ed2bf3..d3f0fb7 100644 --- a/test/verifier/UserOpMultiSigVerifier.t.sol +++ b/test/verifier/UserOpMultiSigVerifier.t.sol @@ -82,7 +82,7 @@ contract UserOpMultiSigVerifierTest is Test { verifier.validateData(message, data, config); } - function testFuzz_validateDataNoOwners(bool withUserOp, uint8 threshold, uint8 size) public { + function testFuzz_validateDataMinOwners(bool withUserOp, uint8 threshold, uint8 size) public { vm.assume(threshold > 0 && size < threshold); Signer[] memory signers = _createSigners(size);