1.4.0: Improve ProxyFactory

[mmv08]

This PR:

• Implements Improve ProxyFactory #462

Regarding dependency version changes:
I had to regenerate the lock file because the current dependencies snapshot didn't work for me with the latest Node LTS release. Tests simply didn't run. After I did rm -rf node modules && rm -rf yarn.lock && yarn, tests that expected return data in reverts failed, allegedly because of a bug in ethers, see NomicFoundation/hardhat#3438
As a result, I pinned hardhat and ethers dependencies. Furthermore, I also pinned solhint-related things because updated versions do not pass the lining check and require contract code reformatting.

Open questions:

• The original ticket says that we should check that the singleton contract exists only when initializer data is passed. IMO, it might be wise to check it regardless of the initializer because, in the end, we want the singleton to be a contract. Also, we do check for a zero address in the proxy constructor so why not. But let me know if that doesn't sound good and we should fall back to the initial plan

Gas before changes:

  Proxy - creation
           Used 112621 gas for >creation<
    ✓ with an EOA (285ms)
           Used 143730 gas for >creation<
    ✓ with a single owner Safe
           Used 149530 gas for >creation<
    ✓ with a single owner and guard Safe
           Used 150796 gas for >creation<
    ✓ with a 2 out of 23 Safe
           Used 157852 gas for >creation<
    ✓ with a 3 out of 3 Safe
           Used 157852 gas for >creation<
    ✓ with a 3 out of 5 Safe

After changes:

  Proxy - creation
           Used 115015 gas for >creation<
    ✔ with an EOA (223ms)
           Used 146133 gas for >creation<
    ✔ with a single owner Safe
           Used 151954 gas for >creation<
    ✔ with a single owner and guard Safe
           Used 153201 gas for >creation<
    ✔ with a 2 out of 23 Safe
           Used 160267 gas for >creation<
    ✔ with a 3 out of 3 Safe
           Used 160267 gas for >creation<
    ✔ with a 3 out of 5 Safe

  Proxy - chain specific creation
           Used 115167 gas for >chain specific creation<
    ✔ with an EOA (215ms)
           Used 146285 gas for >chain specific creation<
    ✔ with a single owner Safe
           Used 152106 gas for >chain specific creation<
    ✔ with a single owner and guard Safe
           Used 153353 gas for >chain specific creation<
    ✔ with a 2 out of 23 Safe
           Used 160419 gas for >chain specific creation<
    ✔ with a 3 out of 3 Safe
           Used 160407 gas for >chain specific creation<
    ✔ with a 3 out of 5 Safe

[coveralls]

Pull Request Test Coverage Report for Build 3895212699

• 14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-1.5%) to 96.963%

---

Totals
Change from base Build 3882598817:	-1.5%
Covered Lines:	301
Relevant Lines:	309

---

💛 - Coveralls

[mmv08]

I pinned some versions because of bugs/incompatibilities. Please see description for a more detailed explanation

[rmeissner]

it might be wise to check it regardless of the initializer because, in the end, we want the singleton to be a contract.

I did not see this as critical as there is no attack vector in this case (afaik). When an inititalizer is passed this is critical as the initializer depends on the singleton logic.

But yeah, it is more secure to always check and if someone wants to go the "unsecure" way they could create their own factory. So opting for the secure by default makes sense

[mmv08]

But yeah, it is more secure to always check and if someone wants to go the "unsecure" way they could create their own factory. So opting for the secure by default makes sense

Given we have this check in place, should we keep the address(0) check in the Proxy contract?

[rmeissner]

Given we have this check in place, should we keep the address(0) check in the Proxy contract?

I would keep it for now. It uses very little gas and having an additional check doesn't hurt.

[rmeissner]

ahh I know why you added this and it is not a blocker, but I would not add any more methods that use the revert approach to return data. It doesn't work well with all nodes (as they all return revert messages differently. If we want to we could refactor this into a custom error with a bytes param ... but that is not available at the solidity version we use afaik.

That being said I would remove both method to calculate the address completely (they provide no benefit). Also maybe we should remove the proxyRuntimeCode as it provides little value (as on-chain information) and breaks zkSync support.

[mmv08]

I agree with reverting methods, they can really behave in an unexpected manner, see NomicFoundation/hardhat#3438

regarding proxyRuntimeCode i'd like to hear Uxio's opinion (iirc methods were added because of him) but he's on vacation until Jan 16th

[rmeissner]

This was used in the relay service which we deprecated, therefore I would optimistically remove the methods, worst case we add them back later :P

[mmv08]

Done, please check

[rmeissner]

I also meant https://github.com/safe-global/safe-contracts/pull/468/files#diff-7a756ca403cd3005982b1a2b902059a02ed81661eae64998b42c8d51972ea5f0L28-L30 with my comment ;)

[rmeissner]

I realized that we could move this to the deployProxy method or use the createProxyWithNonce method in this function (with the saltNonce being keccak256(abi.encodePacked(saltNonce, callback)) (as it is for createProxyWithCallback). This would reduce code duplication.

[mmv08]

good call, please check it now. I had to import the runtime bytecode from solc artifacts to be used in tests though.