add sorting check to make sure the owner array is in ascending order

src/factory/MultiOwnerMSCAFactory.sol

@@ -22,8 +22,7 @@ contract MultiOwnerMSCAFactory is Ownable2Step {
     IEntryPoint public immutable ENTRYPOINT;
 
     error OwnersArrayEmpty();
-    error ZeroAddressOwner();
-    error DuplicateOwner();
+    error InvalidOwner();
     error TransferFailed();
 
     /// @notice Constructor for the factory
@@ -111,32 +110,21 @@ contract MultiOwnerMSCAFactory is Ownable2Step {
     }
 
     /// @notice Getter for counterfactual address based on input params
-    /// @dev Owner array cannot be empty, cannot contain address(0), and cannot contain duplicates
-    /// Different order of valid owner addresses can produce different addresses. Highly recommend caller to sort
-    /// owners array before calling if that is not desired.
-    /// @param salt salt for additional entropy for create2
+    /// @dev The owner array must be sorted in ascending order. It cannot have 0 or duplicated addresses.
     /// @param owners array of addresses of the owner
     function getAddress(uint256 salt, address[] calldata owners) external view returns (address) {
         // array can't be empty
         if (owners.length == 0) {
             revert OwnersArrayEmpty();
         }
 
+        address currentOwnerValue = address(0);
         for (uint256 i = 0; i < owners.length;) {
-            // array can't contain address(0)
-            if (owners[i] == address(0)) {
-                revert ZeroAddressOwner();
+            if (owners[i] <= currentOwnerValue) {
+                revert InvalidOwner();
             }
+            currentOwnerValue = owners[i];
 
-            for (uint256 j = i + 1; j < owners.length;) {
-                // array can't have matching elements
-                if (owners[i] == owners[j]) {
-                    revert DuplicateOwner();
-                }
-                unchecked {
-                    ++j;
-                }
-            }
             unchecked {
                 ++i;
             }

src/factory/MultiOwnerTokenReceiverMSCAFactory.sol

@@ -24,8 +24,7 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable2Step {
     IEntryPoint public immutable ENTRYPOINT;
 
     error OwnersArrayEmpty();
-    error ZeroAddressOwner();
-    error DuplicateOwner();
+    error InvalidOwner();
     error TransferFailed();
 
     /// @notice Constructor for the factory
@@ -119,9 +118,7 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable2Step {
     }
 
     /// @notice Getter for counterfactual address based on input params
-    /// @dev Owner array cannot be empty, cannot contain address(0), and cannot contain duplicates
-    /// Different order of valid owner addresses can produce different addresses. Highly recommend caller to sort
-    /// owners array before calling if that is not desired.
+    /// @dev The owner array must be sorted in ascending order. It cannot have 0 or duplicated addresses.
     /// @param salt salt for additional entropy for create2
     /// @param owners array of addresses of the owner
     function getAddress(uint256 salt, address[] calldata owners) external view returns (address) {
@@ -130,21 +127,13 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable2Step {
             revert OwnersArrayEmpty();
         }
 
+        address currentOwnerValue = address(0);
         for (uint256 i = 0; i < owners.length;) {
-            // array can't contain address(0)
-            if (owners[i] == address(0)) {
-                revert ZeroAddressOwner();
+            if (owners[i] <= currentOwnerValue) {
+                revert InvalidOwner();
             }
+            currentOwnerValue = owners[i];
 
-            for (uint256 j = i + 1; j < owners.length;) {
-                // array can't have matching elements
-                if (owners[i] == owners[j]) {
-                    revert DuplicateOwner();
-                }
-                unchecked {
-                    ++j;
-                }
-            }
             unchecked {
                 ++i;
             }

src/plugins/owner/MultiOwnerPlugin.sol

@@ -78,6 +78,7 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
     // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
 
     /// @inheritdoc IMultiOwnerPlugin
+    /// @dev The owner array must be sorted in ascending order. It cannot have 0 or duplicated addresses.
     function updateOwners(address[] memory ownersToAdd, address[] memory ownersToRemove)
         public
         isInitialized(msg.sender)
@@ -190,6 +191,7 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
     // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
 
     /// @inheritdoc BasePlugin
+    /// @dev The owner array must be sorted in ascending order. It cannot have 0 or duplicated addresses.
     function _onInstall(bytes calldata data) internal override isNotInitialized(msg.sender) {
         (address[] memory initialOwners) = abi.decode(data, (address[]));
         if (initialOwners.length == 0) {
@@ -390,11 +392,19 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
         address[] memory ownersToAdd
     ) private {
         uint256 length = ownersToAdd.length;
+
+        address currentOwnerValue = address(0);
         for (uint256 i = 0; i < length;) {
+            if (ownersToAdd[i] <= currentOwnerValue) {
+                revert InvalidOwner(ownersToAdd[i]);
+            }
+
             if (!ownerSet.tryAdd(associated, CastLib.toSetValue(ownersToAdd[i]))) {
                 revert InvalidOwner(ownersToAdd[i]);
             }
 
+            currentOwnerValue = ownersToAdd[i];
+
             unchecked {
                 ++i;
             }

test/factory/MultiOwnerMSCAFactoryTest.t.sol

@@ -89,13 +89,13 @@ contract MultiOwnerMSCAFactoryTest is Test {
 
         address[] memory badOwners = new address[](2);
 
-        vm.expectRevert(MultiOwnerMSCAFactory.ZeroAddressOwner.selector);
+        vm.expectRevert(MultiOwnerMSCAFactory.InvalidOwner.selector);
         factory.getAddress(0, badOwners);
 
         badOwners[0] = address(1);
         badOwners[1] = address(1);
 
-        vm.expectRevert(MultiOwnerMSCAFactory.DuplicateOwner.selector);
+        vm.expectRevert(MultiOwnerMSCAFactory.InvalidOwner.selector);
         factory.getAddress(0, badOwners);
     }
 

test/plugin/owner/MultiOwnerPlugin.t.sol

@@ -48,9 +48,9 @@ contract MultiOwnerPluginTest is Test {
 
         // set up owners for accountA
         ownerArray = new address[](3);
-        ownerArray[0] = owner1;
-        ownerArray[1] = owner2;
-        ownerArray[2] = owner3;
+        ownerArray[0] = owner2;
+        ownerArray[1] = owner3;
+        ownerArray[2] = owner1;
 
         vm.startPrank(accountA);
         plugin.onInstall(abi.encode(ownerArray));

test/plugin/owner/MultiOwnerPluginIntegration.t.sol

@@ -68,8 +68,8 @@ contract MultiOwnerPluginIntegration is Test {
 
         // setup account with MultiOwnerMSCAFactory
         owners = new address[](2);
-        owners[0] = owner1;
-        owners[1] = owner2;
+        owners[0] = owner1 > owner2 ? owner2 : owner1;
+        owners[1] = owner2 > owner1 ? owner2 : owner1;
         account = UpgradeableModularAccount(payable(factory.getAddress(0, owners)));
         vm.deal(address(account), 100 ether);
         factory.createAccount(0, owners);

test/upgrade/MSCAToMSCA.t.sol

@@ -31,8 +31,8 @@ contract MSCAToMSCATest is Test {
     event Upgraded(address indexed implementation);
 
     function setUp() public {
-        owners.push(makeAddr("owner1"));
         owners.push(makeAddr("owner2"));
+        owners.push(makeAddr("owner1"));
         entryPoint = IEntryPoint(address(new EntryPoint()));
         mscaImpl1 = address(new UpgradeableModularAccount(entryPoint));
         mscaImpl2 = address(new UpgradeableModularAccount(entryPoint));