Use safe singleton factory #464
Conversation
b85ab59 to
f877f94
Compare
ca71aec to
97bf29c
Compare
31884bc to
ec94ac9
Compare
ec94ac9 to
73967ce
Compare
ace4a32 to
ae7886a
Compare
…e-safe-singleton-factory
| id := chainid() | ||
| } | ||
|
|
||
| if (id == 1337) { |
There was a problem hiding this comment.
Do we need chainid 31337 for hardhat support?
There was a problem hiding this comment.
We do, but it's not needed here because safe has provided the signed transaction we need for 31337, so the cost estimator address is at the same location across networks.
| /** | ||
| * This is a temporary workaround while we wait for Safe's singleton factory | ||
| * deployment signature for chain id 1337: | ||
| * https://github.com/safe-global/safe-singleton-factory/issues/97 | ||
| * | ||
| * In the meantime, this allows us to do fast testing using geth dev mode, | ||
| * just without getting the same address in this test environment. | ||
| * | ||
| * This adds: | ||
| * - 1824 L1 gas ($0.05) to the deployment of BLSOpen (114 bytes) | ||
| * - 71 L2 gas ($0.000012) to signature verification | ||
| */ | ||
| function getCostEstimator() internal pure returns (address) { | ||
| uint256 id; | ||
| assembly { | ||
| id := chainid() | ||
| } | ||
|
|
||
| if (id == 1337) { | ||
| return COST_ESTIMATOR_ADDRESS_1337; | ||
| } | ||
|
|
||
| return COST_ESTIMATOR_ADDRESS; | ||
| } |
There was a problem hiding this comment.
Rather than modifying this file, I think it would be safer to just have a script/process that updates the COST_ESTIMATOR_ADDRESS before contract deploy.
Some risks with this approach:
- There is no guarantee that another EVM network would not use
chainid1337. See this article for an example of when this happened with an existingchainidwith ETHPoW after The Merge - Although I have never seen it happen,
chainidis not guaranteed to be the same block to block. A chain could choose at some point to change itschainid. OpenZepplin accounts for this in their EIP-712 implementation - As an attacker, if I could get a chain/network to switch its
chainidto 1337 for a short time & I deployed a malicious contract toCOST_ESTIMATOR_ADDRESS_1337I could generate my ownprecompileGasCostand do Bad Things with BLS sig verification. This seems unlikely on most larger/established networks, but I could see it happening on a smaller EVM network/L2, especially if its security/consensus mechanisms are more centralized.
I also could see passing the cost estimator address as an arg to verifySingle/multiple, but that is a more significant change. IMO, asides from updating the COST_ESTIMATOR_ADDRESS, we should not update/change the BLS libs/contracts taken from Hubble.
Related: #189
There was a problem hiding this comment.
I hear you, but I think you are overlooking two things:
- The 1337 address is still hardcoded to an address which can only contain the same logic, due to the deterministic deployment. Messing with the chainid does not allow an attacker to insert a malicious cost estimator.
- This is not intended to ever deploy to a non-test network.
I wouldn't be happy with modifying the file when using 1337 deployments, because I think that leads to a lot of subtle confusion, as we experienced previously when the committed address here was correct for production but not development.
If you have any contacts at safe, it would be really nice if they just provided the signature we need. It should really only take a few minutes of their time and avoid all this mess.
There was a problem hiding this comment.
@jacque006 Moot now, Safe came through and I deleted all this code 🎉.
| @@ -22,6 +22,9 @@ | |||
| "dependencies": { | |||
| "@openzeppelin/contracts": "^4.7.3" | |||
| }, | |||
| "peerDependencies": { | |||
| "ethers": "^5.0.0" | |||
There was a problem hiding this comment.
What was this needed for?
There was a problem hiding this comment.
Fixes lint issue.
Related: #511 (comment)
| await ( | ||
| await signer.sendTransaction({ | ||
| to: deployment.signerAddress, | ||
| value: ethers.BigNumber.from(deployment.gasPrice).mul( | ||
| deployment.gasLimit, | ||
| ), | ||
| }) | ||
| ).wait(); | ||
|
|
||
| await ( | ||
| await signer.provider.sendTransaction(deployment.transaction) | ||
| ).wait(); |
There was a problem hiding this comment.
👎 Nested awaits (others spots with this, will only comment here :) )
There was a problem hiding this comment.
Conversation here: #511 (comment)
contracts/shared/deploy.ts
Outdated
| export type Deployment = { | ||
| singletonFactory: SafeSingletonFactory; | ||
| precompileCostEstimator: BNPairingPrecompileCostEstimator; | ||
| blsLibrary: BLSOpen; | ||
| verificationGateway: VerificationGateway; | ||
| blsExpander: BLSExpander; | ||
| aggregatorUtilities: AggregatorUtilities; | ||
| }; |
There was a problem hiding this comment.
Would be good to consolidate this type with BlsWalletContracts in clients
|
@jzaki should also review & determine final approval |
Co-authored-by: Jacob Caban-Tomski <jacque006@users.noreply.github.com>
de0411a to
e1e654e
Compare
…e-safe-singleton-factory
8fe7706 to
e416c13
Compare
…e-safe-singleton-factory
| @@ -61,11 +65,11 @@ contract VerificationGateway | |||
| constructor( | |||
| IBLS bls, | |||
| address blsWalletImpl, | |||
| address proxyAdmin | |||
| ProxyAdminGenerator proxyAdminGenerator | |||
There was a problem hiding this comment.
Previously the PA and VG deployment were in the same function, and when needing to deploy our own deployer, this was deterministic. With the change to using the safe singleton factory, the smaller change (preserving audit as much as possible) would have been to change PA in place to be deployed with the safe singleton factory.
Also the design of a separate PA Generator although cleaner in some ways, implies use of PAs more generally in this application which I'd rather lean away from.
There was a problem hiding this comment.
Noticed this was changed in the following commits (#536 ) ✔️
| * enough in our testing environment. This method works around that by adding | ||
| * 50% to the gas estimate. | ||
| */ | ||
| async processBundleWithExtraGas(bundle: Bundle, overrides: Overrides = {}) { |
There was a problem hiding this comment.
The contract tests were working without this, so is this regarding aggregator tests?
There was a problem hiding this comment.
The smart contract tests have changed from a simple call line to multiple lines breaking things up to sign, adding many more lines to the test functions and making them much less clear.
Since this is seems to be a typescript preference, I'm willing to concede this for v1, but we can work together on more integrated use of client code in v2 contract tests.
| * Chosen arbitrarily | ||
| * =keccak256(abi.encodePacked(uint32(0xfeedbee5))) | ||
| */ | ||
| bytes32 BLS_DOMAIN = 0x0054159611832e24cdd64c6a133e71d373c5f8553dde6c762e6bffe707ad83cc; |
There was a problem hiding this comment.
Is this still the arbitrarily generated hash? Arguably this takes more bytes than the opcodes, but VG size is not as important as wallet size. So just curious about the change. Just noticed comment in #527
What is this PR doing?
Ports our deployment to use safe's singleton factory.
This has the massive advantage of making the addresses of our deployments predetermined for all networks and for any
tx.originused to do the deployment.In making the switch, I've also fixed the deterministic generation of
VerificationGateway'sProxyAdminusing the newProxyAdminGenerator. This was critical to getting a predetermined address forVerificationGatewaybecauseproxyAdminwas a constructor parameter, which affectsVerificationGateway's address. This was a pre-existing issue: #332.Removes:
create2DeployerDEPLOYER_*variablescreate2Fixture(replaced by SafeSingletonFactory class)lazyBlsWallets(replaced byfx.createBLSWallet()) (was always a bit clunky, and was the cause of test failures)There's also a workaround called
fx.processBundleWithExtraGas. The automatic gasLimit isn't sufficient forsetPendingBLSKeyForWallet, I think this has to do with the call depth involved... ie, the gasLimit is enough for the actual amount of gas used, but not enough gas is provided to one of the deeply nested calls since 63/64ths of the gas is provided to each next layer.Finally, I've added
getCostEstimator()toBLS.solas a temporary workaround while we wait for Safe's singleton factory on 1337: safe-global/safe-singleton-factory#97. This function just selects the right cost estimator based on the chain id, which means that testing using geth doesn't require editing this file.How can these changes be manually tested?
yarn hardhat testscripts/testDoes this PR resolve or contribute to any issues?
Resolves #394, #332, #334.
Checklist
Guidelines
resolve conversationbutton is for reviewers, not authors