Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mask computed address in Create2 and Clones libraries #4941

Merged

Conversation

byshape
Copy link
Contributor

@byshape byshape commented Mar 5, 2024

The Create2 library has a computeAddress function and the Clones library has a predictDeterministicAddress function that return address as a result of the keccak256. Unmasked, this results in invalid value if used later in assembly code block.

Imagine that the computeAddress or predictDeterministicAddress function 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 keccak256 function to avoid possible problems with address usage.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Mar 5, 2024

🦋 Changeset detected

Latest commit: 379f055

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Patch

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

@byshape byshape changed the title Mask computed address in Create2.computeAddress Mask computed address in Create2 and Clones libraries Mar 5, 2024
ernestognw
ernestognw previously approved these changes Mar 15, 2024
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution @byshape!
Let's wait on @Amxx approval.

Amxx
Amxx previously approved these changes Mar 15, 2024
@Amxx Amxx dismissed stale reviews from ernestognw and themself via 66df9c6 March 17, 2024 12:37
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ZumZoom
Copy link
Contributor

ZumZoom commented Mar 20, 2024

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 /// @solidity memory-safe-assembly annotations instead of assembly ("memory-safe")?

@ernestognw
Copy link
Member

Is it still a good idea to stick with /// @solidity memory-safe-assembly annotations instead of assembly ("memory-safe")?
That's a good question.

I checked on the Solidity repository and found out this is where the assembly dialect was added.
ethereum/solidity#12620

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.

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm merging since the memory-safe annotation can be added in another PR and I don't want to block this one.

@ernestognw ernestognw merged commit d398d68 into OpenZeppelin:master Mar 25, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants