(audit 6.1.2) CatapultarFactory allows _saltPrefix() collisions

solidity/snapshots/CatapultarFactoryTest.json

@@ -1,5 +1,5 @@
 {
-  "deploy": "117786",
-  "deployUpgradeable": "143266",
-  "deployWithDigest": "143295"
+  "deploy": "117737",
+  "deployUpgradeable": "143021",
+  "deployWithDigest": "143234"
 }

solidity/snapshots/CatapultarFactoryTronTest.json

@@ -1,5 +1,5 @@
 {
-  "deploy": "117729",
-  "deployUpgradeable": "143245",
-  "deployWithDigest": "143250"
+  "deploy": "117737",
+  "deployUpgradeable": "143021",
+  "deployWithDigest": "143234"
 }

solidity/src/CatapultarFactory.sol

@@ -31,6 +31,7 @@ import { KeyedOwnable } from "./libs/KeyedOwnable.sol";
  * The owner of a deployed proxy may not be the same as the owner it was deployed with.
  */
 contract CatapultarFactory {
+    error TooManyOwners();
     address payable public immutable EXECUTOR;
 
     constructor() {
@@ -47,8 +48,8 @@ contract CatapultarFactory {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external payable ownerInSalt(salt, ktp, owner) returns (address payable proxy) {
-        proxy = payable(LibClone.cloneDeterministic_PUSH0(EXECUTOR, salt));
+    ) external payable returns (address payable proxy) {
+        proxy = payable(LibClone.cloneDeterministic_PUSH0(EXECUTOR, _salt(salt, ktp, owner)));
 
         Catapultar(payable(proxy)).init{ value: msg.value }(ktp, owner);
     }
@@ -60,8 +61,8 @@ contract CatapultarFactory {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external view ownerInSalt(salt, ktp, owner) returns (address proxy) {
-        return LibClone.predictDeterministicAddress_PUSH0(EXECUTOR, salt, address(this));
+    ) external view returns (address proxy) {
+        return LibClone.predictDeterministicAddress_PUSH0(EXECUTOR, _salt(salt, ktp, owner), address(this));
     }
 
     /// @param salt The first 20 bytes of salt has to be the owner or 0.
@@ -71,15 +72,15 @@ contract CatapultarFactory {
         bytes32 salt,
         bytes32 digest,
         bool isSignature
-    ) external payable ownerInSalt(salt, ktp, owner) returns (address payable proxy) {
+    ) external payable returns (address payable proxy) {
         uint256 nonce;
         assembly ("memory-safe") {
             // Catapultar.DigestApproval.Call == 1
             // Catapultar.DigestApproval.Signature == 2
             // isSignature + 1 == 2 if isSignature === true otherwise 1.
             nonce := add(isSignature, 1)
         }
-        bytes32 saltWithDigest = EfficientHashLib.hash(salt, digest, bytes32(nonce));
+        bytes32 saltWithDigest = EfficientHashLib.hash(_salt(salt, ktp, owner), digest, bytes32(nonce));
         proxy = payable(LibClone.cloneDeterministic_PUSH0(EXECUTOR, saltWithDigest));
 
         // forge-lint: disable-next-line(unsafe-typecast)
@@ -97,15 +98,15 @@ contract CatapultarFactory {
         bytes32 salt,
         bytes32 digest,
         bool isSignature
-    ) external view ownerInSalt(salt, ktp, owner) returns (address proxy) {
+    ) external view returns (address proxy) {
         uint256 nonce;
         assembly ("memory-safe") {
             // Catapultar.DigestApproval.Call == 1
             // Catapultar.DigestApproval.Signature == 2
             // isSignature + 1 == 2 if isSignature === true otherwise 1.
             nonce := add(isSignature, 1)
         }
-        bytes32 saltWithDigest = EfficientHashLib.hash(salt, digest, bytes32(nonce));
+        bytes32 saltWithDigest = EfficientHashLib.hash(_salt(salt, ktp, owner), digest, bytes32(nonce));
         return LibClone.predictDeterministicAddress_PUSH0(EXECUTOR, saltWithDigest, address(this));
     }
 
@@ -114,7 +115,7 @@ contract CatapultarFactory {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external payable ownerInSalt(salt, ktp, owner) returns (address payable proxy) {
+    ) external payable returns (address payable proxy) {
         proxy = payable(LibClone.deployDeterministicERC1967(EXECUTOR, salt));
 
         Catapultar(payable(proxy)).init{ value: msg.value }(ktp, owner);
@@ -129,38 +130,39 @@ contract CatapultarFactory {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external view ownerInSalt(salt, ktp, owner) returns (address proxy) {
+    ) external view returns (address proxy) {
         return LibClone.predictDeterministicAddressERC1967(EXECUTOR, salt, address(this));
     }
 
     // --- Helpers --- //
 
     /**
-     * @notice Requires that the salt contains the owner as the first 20 bytes or they are 0.
-     * @dev When deploying proxies, it may not be safe to set the first 20 bytes to 0. If this is desired, the risk is
-     * entirely up to the user.
-     * For a key larger than 20 bytes, the first 20 bytes are taken of keccak256(ktp, keccak256(owner))
+     * @notice Computes a new salt based on the provided creation parameters
      * @param salt A bytes32 value intended to pseudo-randomize the deployed address.
      * @param ktp The keytype for the account
      * @param owner A desired callable parameter for a proxy address
      */
-    modifier ownerInSalt(
-        bytes32 salt,
-        KeyedOwnable.PublicKeyType ktp,
-        bytes32[] calldata owner
-    ) {
-        LibClone.checkStartsWith(salt, _saltPrefix(ktp, owner));
-        _;
-    }
-
-    function _saltPrefix(
+    function _salt(
+        bytes32 preSalt,
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner
-    ) internal pure returns (address) {
-        if (ktp == KeyedOwnable.PublicKeyType.ECDSAOrSmartContract && owner.length == 1) {
-            return address(uint160(uint256(owner[0])));
-        } else {
-            return address(bytes20(EfficientHashLib.hash(bytes32(uint256(uint8(ktp))), EfficientHashLib.hash(owner))));
+    ) internal pure returns (bytes32 salt) {
+        uint256 numOwners = owner.length;
+        if (numOwners > type(uint8).max) revert TooManyOwners();
+        assembly ("memory-safe") {
+            let m := mload(0x40)
+
+            // In memory, store:
+            // [m .. m+31]:         preSalt
+            // [m+32]:              ktp
+            // [m+33]:              numOwners
+            // [m+34 .. m+33+N*32]: owners
+            mstore(m, preSalt)
+            mstore8(add(m, 32), ktp)
+            mstore8(add(m, 33), numOwners)
+            calldatacopy(add(m, 34), owner.offset, mul(numOwners, 32))
+
+            salt := keccak256(m, add(34, mul(numOwners, 32)))
         }
     }
 }

solidity/src/CatapultarFactory.tron.sol

@@ -31,6 +31,7 @@ import { KeyedOwnable } from "./libs/KeyedOwnable.sol";
  * The owner of a deployed proxy may not be the same as the owner it was deployed with.
  */
 contract CatapultarFactoryTron {
+    error TooManyOwners();
     address payable public immutable EXECUTOR;
 
     constructor() {
@@ -47,8 +48,8 @@ contract CatapultarFactoryTron {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external payable ownerInSalt(salt, ktp, owner) returns (address payable proxy) {
-        proxy = payable(LibCloneTron.cloneDeterministic_PUSH0(EXECUTOR, salt));
+    ) external payable returns (address payable proxy) {
+        proxy = payable(LibCloneTron.cloneDeterministic_PUSH0(EXECUTOR, _salt(salt, ktp, owner)));
 
         Catapultar(payable(proxy)).init{ value: msg.value }(ktp, owner);
     }
@@ -60,8 +61,8 @@ contract CatapultarFactoryTron {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external view ownerInSalt(salt, ktp, owner) returns (address proxy) {
-        return LibCloneTron.predictDeterministicAddress_PUSH0(EXECUTOR, salt, address(this));
+    ) external view returns (address proxy) {
+        return LibCloneTron.predictDeterministicAddress_PUSH0(EXECUTOR, _salt(salt, ktp, owner), address(this));
     }
 
     /// @param salt The first 20 bytes of salt has to be the owner or 0.
@@ -71,15 +72,15 @@ contract CatapultarFactoryTron {
         bytes32 salt,
         bytes32 digest,
         bool isSignature
-    ) external payable ownerInSalt(salt, ktp, owner) returns (address payable proxy) {
+    ) external payable returns (address payable proxy) {
         uint256 nonce;
         assembly ("memory-safe") {
             // Catapultar.DigestApproval.Call == 1
             // Catapultar.DigestApproval.Signature == 2
             // isSignature + 1 == 2 if isSignature === true otherwise 1.
             nonce := add(isSignature, 1)
         }
-        bytes32 saltWithDigest = EfficientHashLib.hash(salt, digest, bytes32(nonce));
+        bytes32 saltWithDigest = EfficientHashLib.hash(_salt(salt, ktp, owner), digest, bytes32(nonce));
         proxy = payable(LibCloneTron.cloneDeterministic_PUSH0(EXECUTOR, saltWithDigest));
 
         // forge-lint: disable-next-line(unsafe-typecast)
@@ -97,15 +98,15 @@ contract CatapultarFactoryTron {
         bytes32 salt,
         bytes32 digest,
         bool isSignature
-    ) external view ownerInSalt(salt, ktp, owner) returns (address proxy) {
+    ) external view returns (address proxy) {
         uint256 nonce;
         assembly ("memory-safe") {
             // Catapultar.DigestApproval.Call == 1
             // Catapultar.DigestApproval.Signature == 2
             // isSignature + 1 == 2 if isSignature === true otherwise 1.
             nonce := add(isSignature, 1)
         }
-        bytes32 saltWithDigest = EfficientHashLib.hash(salt, digest, bytes32(nonce));
+        bytes32 saltWithDigest = EfficientHashLib.hash(_salt(salt, ktp, owner), digest, bytes32(nonce));
         return LibCloneTron.predictDeterministicAddress_PUSH0(EXECUTOR, saltWithDigest, address(this));
     }
 
@@ -114,7 +115,7 @@ contract CatapultarFactoryTron {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external payable ownerInSalt(salt, ktp, owner) returns (address payable proxy) {
+    ) external payable returns (address payable proxy) {
         proxy = payable(LibCloneTron.deployDeterministicERC1967(EXECUTOR, salt));
 
         Catapultar(payable(proxy)).init{ value: msg.value }(ktp, owner);
@@ -129,38 +130,42 @@ contract CatapultarFactoryTron {
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner,
         bytes32 salt
-    ) external view ownerInSalt(salt, ktp, owner) returns (address proxy) {
+    ) external view returns (address proxy) {
         return LibCloneTron.predictDeterministicAddressERC1967(EXECUTOR, salt, address(this));
     }
 
     // --- Helpers --- //
 
     /**
-     * @notice Requires that the salt contains the owner as the first 20 bytes or they are 0.
+     * @notice Computes a new salt based on the provided creation parameters.
      * @dev When deploying proxies, it may not be safe to set the first 20 bytes to 0. If this is desired, the risk is
      * entirely up to the user.
      * For a key larger than 20 bytes, the first 20 bytes are taken of keccak256(ktp, keccak256(owner))
-     * @param salt A bytes32 value intended to pseudo-randomize the deployed address.
+     * @param preSalt A bytes32 value intended to pseudo-randomize the deployed address.
      * @param ktp The keytype for the account
      * @param owner A desired callable parameter for a proxy address
      */
-    modifier ownerInSalt(
-        bytes32 salt,
-        KeyedOwnable.PublicKeyType ktp,
-        bytes32[] calldata owner
-    ) {
-        LibCloneTron.checkStartsWith(salt, _saltPrefix(ktp, owner));
-        _;
-    }
-
-    function _saltPrefix(
+    function _salt(
+        bytes32 preSalt,
         KeyedOwnable.PublicKeyType ktp,
         bytes32[] calldata owner
-    ) internal pure returns (address) {
-        if (ktp == KeyedOwnable.PublicKeyType.ECDSAOrSmartContract && owner.length == 1) {
-            return address(uint160(uint256(owner[0])));
-        } else {
-            return address(bytes20(EfficientHashLib.hash(bytes32(uint256(uint8(ktp))), EfficientHashLib.hash(owner))));
+    ) internal pure returns (bytes32 salt) {
+        uint256 numOwners = owner.length;
+        if (numOwners > type(uint8).max) revert TooManyOwners();
+        assembly ("memory-safe") {
+            let m := mload(0x40)
+
+            // In memory, store:
+            // [m .. m+31]:         preSalt
+            // [m+32]:              ktp
+            // [m+33]:              numOwners
+            // [m+34 .. m+33+N*32]: owners
+            mstore(m, preSalt)
+            mstore8(add(m, 32), ktp)
+            mstore8(add(m, 33), numOwners)
+            calldatacopy(add(m, 34), owner.offset, mul(numOwners, 32))
+
+            salt := keccak256(m, add(34, mul(numOwners, 32)))
         }
     }
 }