Mask computed address in Create2 and Clones libraries#4941
Conversation
🦋 Changeset detectedLatest commit: 379f055 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Create2.computeAddressCreate2 and Clones libraries
Amxx
left a comment
There was a problem hiding this comment.
I don't really see a need to check that in hardhat, as coverage doesn't report that kind of operations
I replaced the mock and hardhat tests with a very short/simple fuzzing test.
|
Not sure about the visibility of the comment inside the resolved outdated changes so duplicating it here: Is it still a good idea to stick with |
I checked on the Solidity repository and found out this is where the assembly dialect was added. My understanding is that it was added in a non-breaking way, so we're fine keeping it as a NatSpec annotation but you're right in that we may prefer the dialect. I don't see any concrete benefit but I think we're open to discuss it further. I'm opening an issue. |
ernestognw
left a comment
There was a problem hiding this comment.
I'm merging since the memory-safe annotation can be added in another PR and I don't want to block this one.
The
Create2library has acomputeAddressfunction and theCloneslibrary has apredictDeterministicAddressfunction that return address as a result of thekeccak256. Unmasked, this results in invalid value if used later in assembly code block.Imagine that the
computeAddressorpredictDeterministicAddressfunction returns the address that is passed to another function that makes a call using that address as a parameter. If this assembly code, such as library, doesn’t clear the address, then this “address” will be invalid and the call will fail.I suggest masking the result of the
keccak256function to avoid possible problems with address usage.PR Checklist
npx changeset add)