Fixed and More Consistent Pragmas #24
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
As discussed internally with the team, we decided to index the `signer` field of the `Created` event from the passkey factory contract. This came as a follow up to event discussions from an issue raised from the Hats audit competition: hats-finance#3 Note that this only slightly increases the deployment costs: ``` === BEFORE === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 94366 ✔ Benchmark signer deployment cost (476ms) === AFTER === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 94476 ✔ Benchmark signer deployment cost (476ms) ``` For reference, this is a 0.1% increase to the deployment gas costs.
This PR changes the pragmas in the passkeys contract to be:
1. More consistent: we use `^0.8.20` everywhere for "non-top-level"
contracts. The version was chosen as this it the first version that
supports all of the compiler features that we make use of. Notably,
we have `@custom:` NatSpec items which are only officially supported
as of v0.8.20. The choice to use floating pragmas here is to make
using these support contracts easier across other projects (namely,
the `WebAuthn` and `P256` libraries are useful outside of this
project).
2. Use fixed pragmas for "top-level" contracts that we deploy:
- `SafeWebAuthnProxyFactory`
- `SafeWebAuthnSharedSigner`
- `FCLP256Verifier`
I am aware that Solidity v0.8.20 doesn't play nice with chains like BNB
by default, however this can be worked around by explicitly by setting
the EVM version target (as we do in this project) and do not believe
this is an issue.
I noticed that the IR optimizer is better for all contracts **except** the FCL library. This PR optimizes the compiler configuration to take advantage of the IR optimizer, while adding an exception for the FCL P-256 verifier contract so that it does not suffer any regressions. ``` === BEFORE === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 94366 ✔ Benchmark signer deployment cost (476ms) ⛽ verification (FreshCryptoLib): 216907 ✔ Benchmark signer verification cost with FreshCryptoLib verifier (116ms) ⛽ verification (Daimo): 346809 ✔ Benchmark signer verification cost with Daimo verifier (103ms) ⛽ verification (Dummy): 14309 ✔ Benchmark signer verification cost with Dummy verifier (100ms) ⛽ verification (Precompile): 15098 ✔ Benchmark signer verification cost with Precompile verifier (104ms) === AFTER === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 91042 ✔ Benchmark signer deployment cost (457ms) ⛽ verification (FreshCryptoLib): 213919 ✔ Benchmark signer verification cost with FreshCryptoLib verifier (121ms) ⛽ verification (Daimo): 342642 ✔ Benchmark signer verification cost with Daimo verifier (110ms) ⛽ verification (Dummy): 12564 ✔ Benchmark signer verification cost with Dummy verifier (104ms) ⛽ verification (Precompile): 13379 ✔ Benchmark signer verification cost with Precompile verifier (98ms) ``` Furthermore, we bump the compiler version to v0.8.26 as it is the lastest stable release and brings some small gas improvements.
… estimation (#446) This PR fixes #425 by introducing a new `estimateUserOperationGas` function, which runs the simulation using AA's reference `EntryPointSimulations` contract to ensure we use correct estimations for our test user operations. Previously, we set them to arbitrarily high numbers to make the tests pass. I also improved a couple of tests where we set `maxFeePerGas` to 0 by actually obtaining the prefund value the account paid for the operation. The code was mostly borrowed from the account-abstraction reference bundler and is not suitable for use in a production environment. It also includes the `logUserOperationGas` function, which fixes the core issue of displaying the actual gas used by the user operation. Example log: ``` Gas Metering Safe Deployment + Enabling 4337 Module Used 418349 gas (Account or Paymaster) for >Safe with 4337 Module Deployment< Used 415210 gas (Transaction) for >Safe with 4337 Module Deployment< ✔ Safe with 4337 Module Deployment (137ms) Safe Deployment + Enabling 4337 Module + Native Transfers Used 449725 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + Native Transfer< Used 447665 gas (Transaction) for >Safe with 4337 Module Deployment + Native Transfer< ✔ Safe with 4337 Module Deployment + Native Transfer Used 191691 gas (Account or Paymaster) for >Safe with 4337 Module Native Transfer< Used 182078 gas (Transaction) for >Safe with 4337 Module Native Transfer< ✔ Safe with 4337 Module Native Transfer Safe Deployment + Enabling 4337 Module + Token Operations Used 435081 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC20 Transfer< Used 426148 gas (Transaction) for >Safe with 4337 Module Deployment + ERC20 Transfer< ✔ Safe with 4337 Module Deployment + ERC20 Token Transfer Used 469349 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC721 Transfer< Used 467959 gas (Transaction) for >Safe with 4337 Module Deployment + ERC721 Transfer< ✔ Safe with 4337 Module Deployment + ERC721 Token Minting Token Operations Only Used 176295 gas (Account or Paymaster) for >Safe with 4337 Module ERC20 Transfer< Used 160584 gas (Transaction) for >Safe with 4337 Module ERC20 Transfer< ✔ Safe with 4337 Module ERC20 Token Transfer Used 211369 gas (Account or Paymaster) for >Safe with 4337 Module ERC721 Transfer< Used 202383 gas (Transaction) for >Safe with 4337 Module ERC721 Transfer< ✔ Safe with 4337 Module ERC721 Token Minting ``` I ran into some types issues on CI and just working on the issue. I'll add a GitHub comment to each one.
… function via call instead (#450) In #446, I inlined the signature check function because we didn't want it to revert in unsuccessful cases. I inlined it because the `try/catch` statement didn't work for internal function calls. Little did I know that I could do `this.f()` and that would use the `CALL` opcode instead of `JUMP`, making the `try/catch` statement possible More info: https://www.perplexity.ai/search/if-i-call-1VYIfl5eQEWvJ302U7wVOA#0
Fixes hats-finance#14, also see hats-finance#22 for some additional context. This PR changes the `_sha256` implementation to check the result from the static call. There is a very subtle bug with not checking, where, for very large inputs, you would be able to get the precompile to revert but have the function finish executing successfully (and use whatever is in the scratch space as the digest). Note that **we do not check the length of the `returndata`**. This is intentional and the same thing that the Solidity compiler does for the builtin `sha256` function.
|
Re-created pre-merge branch here: #31 |
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Fixes #17
This PR changes the pragmas in the passkeys contract to be:
^0.8.20everywhere for "non-top-level" contracts. The version was chosen as this it the first version that supports all of the compiler features that we make use of. Notably, we have@custom:NatSpec items which are only officially supported as of v0.8.20 (incidentally, this is the same version used by OpenZeppelin contracts for similar reason: source). The choice to use floating pragmas here is to make using these support contracts easier across other projects (namely, theWebAuthnandP256libraries are useful outside of this project). Furthermore, we use^versions so that breaking changes introduced in a potential Solidity v0.9 would not affect the security and integrity of the contract code.SafeWebAuthnProxyFactorySafeWebAuthnSharedSignerFCLP256VerifierI am aware that Solidity v0.8.20 doesn't play nice with chains like BNB by default, however this can be worked around by explicitly by setting the EVM version target (as we do in this project) and do not believe this is an issue.