[WIP] Code Jar Deployments #139
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR adds happy path tests for direct execution of Quark wallets. `QuarkWallet.t.sol` only tested revert paths for direct execution. We do already have indirect coverage of the happy path in our tests for `Multicall`.
* address 7.7 * address 7.3 * address 7.2.5 * address 7.2.2 * address 7.2.3 * fix poolkey import syntax * address 7.2.7 * address 7.6 unused code * address comments * added leftover pk variable name * address 7.2.10 * address 7.2.4 * address 7.2.11 * fix test errors as the domain separtor need to save into the proxy as well and retrieve via special getter * Revert "fix test errors as the domain separtor need to save into the proxy as well and retrieve via special getter" This reverts commit 00f3f33. * Revert "address 7.2.4" This reverts commit bf7f87a. * add test for approval refund * just put it below, and don't use else * Revert "address 7.2.7" This reverts commit 37435c9. * update gas snapshot * update snapshot * address comment to move empty code check to 153 * update snapshot
* refactor: AbstractQuarkWalletFactory.sol base contract for factories * refactor/minor: walletImplementation should be internal, cheaper deploys * feat: QuarkWalletProxyFactory for deploying wallets by minimal proxy * test+refactor: introduce abstract wallet factory test suite * test: implement abstract wallet factory test suite using proxy factory * refactor: s/ProxyDirect/QuarkMinimalProxy * infra/minor: add an ir profile to quark-proxy compilation settings * minor: update comment in abstract factory test suite * chore: ugh, forge fmt * bugfix: missing import, somehow * refactor: split QuarkWallet into Stubbed and Standalone implementations * test+refactor/minor: qualify all imports * refactor: merge stubbed into the base contract to make it not abstract * refactor: `this` is bad, don't use `this` * refactor: use an inline cast-and-call for signer() / executor() * refactor: QuarkWalletProxyFactory takes only a wallet implementation * refactor: stop abstracting quark wallet factory * refactor: replace all factories with proxy factories * doc+refactor: clean up comments in QuarkWalletProxyFactory * minor: spelling * minor: clean up test comments * chore: forge fmt
Quark wallet now emits an event when executing a Quark script. We purposely don't emit `scriptCalldata` as part of the event since that can add a lot of gas. The current gas overhead is ~1.7k.
…es (#140) SEE EDIT This gas optimized reentrancy guard (`nonReentrantOptimized`) saves around ~5k+ gas compared to the approach of writing the re-entrancy guard to `StateManager`. We leave a note suggesting that it is not as safe as the version that writes to `StateManager`. Regardless, this guard should be safe for most scripts since the modifier uses a storage slot addressed at a hash with an unknown string preimage. The guard is safe to use as long as scripts don't write directly to this storage slot in the same execution context. I originally intended to add the guard to `Ethcall.sol` as well, but decided not to due to the extra 15k gas overhead. I also believe the `Ethcall` use-case is very different and it should really only be used to call into known and safe contracts. A note of caution is added to `Ethcall` to call out that it is not reentrancy safe. EDIT: After some discussion, we decided to use the `onlyWallet` guard, which is a cheaper reentrancy guard with weaker guarantees. It protects against typical reentrancy paths that involve a `call`, but cannot protect against "recursive reentrancy" involving `delegatecall`s. That being said, reentrancy attacks rarely, if ever, involve `delegatecall`s and there are loads more to worry about when a script uses `delegatecall`.
This change will cause scripts that use `delegatecall` (e.g. `Multicall`) to revert if called directly. This is done to prevent griefers from self-destructing the contract. The approach taken in this PR uses an `initialize` function that writes the script contract's address to storage. Every call to the script will check that the `msg.sender != cachedAddress` to ensure the call is never in the context of the script contract. In terms of gas, this approach seems to introduce a 2.5-3k gas overhead. Note: `UniswapFlashLoan` and `UniswapFlashSwapExactOut` are not susceptible to the same griefing vector. This is because the `delegatecall` in these scripts can only be accessed in a codepath where `msg.sender == valid Uniswap pool`, meaning that a griefer would have to use `UniswapScript.run` as the entry point for the attack. However, the `run` function in these scripts makes an `allowCallback()` call, which reverts immediately if not executed in the context of a Quark wallet. I added [tests](6003718) to verify this revert behavior.
As long as `evmVersion = paris`, the compiler shouldn't be using push0.
This patch improves Code Jar to use CodeJarStub, which is a stub that's semi-verifiable on Etherscan for deployments, and significantly reduces the complexity/noise of how CodeJar works. This is also the basis for CodeJar deployments, which will be included in a different branch.
This patch just hacks in `datasize("CodeJarStub")` as a magic constant (derived emperically) into the contract itself, which allows us to get an optimal version of CodeJarStub that saves ~2-3K gas from using Solidity's ABI-encoding which is particularly expensive mostly due to dynamic allocations that break the optimizer. This is a weird system, but read the comments as to why it's probably an okay decision.
hayesgm
force-pushed
the
hayesgm/code-jar-deployments
branch
from
1693680 to
a480651
Compare
This patch is the remaining aspects of "Code Jar Deployments" which is experimental and doesn't need to be resolved immediately.
hayesgm
force-pushed
the
hayesgm/code-jar-deployments
branch
from
a480651 to
04a3249
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch is the remaining aspects of "Code Jar Deployments" which is experimental and doesn't need to be resolved immediately.