Memory-Safe Assembly

[Defimatt]

🧐 Motivation Solidity 0.8.13 marked the production readiness of the Yul IR pipeline. This, amongst other things, helps to alleviate stack too deep errors by allowing the compiler to move items to memory as and when needed.

📝 Details One requirement of the pipeline is that any assembly sections be memory-safe, such that they only use memory that has been previously allocated either by high-level Solidity code or by reading from the free memory pointer at 0x40.

Any memory-safe assembly can be marked as such by annotating it assembly ("memory-safe") { ... }. However, this is only possible for code that uses 0.8.13 and above. For code that needs to be backward-compatible, the assembly block can be annotated:

/// @solidity memory-safe-assembly
assembly {
    ...
}

Details about what constitutes memory-safe code is available at https://docs.soliditylang.org/en/v0.8.13/assembly.html#memory-safety.

Would it be possible to review and annotate any uses of assembly within the OpenZeppelin libraries where it is determined to be memory-safe, and to modify then annotate when not?

[nathan-lapinski]

Hello 👋 I'm new to this repository and am looking for a good first issue to pick up. I'd like to try this issue.

It looks like there are 29 instances of the assembly {} block in the openzeppelin-contracts repo, across 13 files. Is the idea to include them all in one pull request, or split them up so that multiple people can work on this issue?

Thanks in advance!

[scyron6]

Hi @nathan-lapinski. I'm also new to this repository and would like to help out if there are multiple uses of assembly that need to be modified.

[nathan-lapinski]

Hi @scyron6 , that would be great, thanks for the help!👍

By my count, I see the assembly {} tag being used as shown in the table below. If it's alright, maybe we can use something like this table to keep track of which files are in progress?

I've gone ahead and started on contracts/mocks/SafeMathMock.sol since the assembly {} usages all look nearly identical in that file. Happy to trade though.

@Defimatt is it ok if this issue is split up over multiple PRs? Or would you prefer to have everything in one PR?

File Name	Number of Assembly Tags	Person Working on Task	Status (in-progress, in-review, completed)
contracts/mocks/SafeMathMock.sol	10	@nathan-lapinski	not-needed
contracts/utils/StorageSlot.sol	4	@nathan-lapinski	in-review
contracts/proxy/Clones.sol	3	@nathan-lapinski	in-review
contracts/utils/structs/EnumerableSet.sol	2	@nathan-lapinski	in-review
contracts/utils/cryptography/ECDSA.sol	2	@Kartik0099	completed
contracts/utils/cryptography/MerkleProof.sol	1	@Kartik0099	completed
contracts/utils/Create2.sol	1	@Kartik0099	completed
contracts/utils/Base64.sol	1	@nathan-lapinski	in-review
contracts/utils/Address.sol	1	@nathan-lapinski	in-review
contracts/token/ERC721/ERC721.sol	1	@nathan-lapinski	in-review
contracts/proxy/Proxy.sol	1	@nathan-lapinski	in-progress
contracts/metatx/MinimalForwarder.sol	1	@nathan-lapinski	in-review
contracts/metatx/ERC2771Context.sol	1	@nathan-lapinski	in-review

[frangio]

Multiple PRs are okay but please try to minimize them. Feel free to continue to coordinate in this issue.

[scyron6]

@nathan-lapinski I can work from bottom to top on that list and you go top to bottom if you'd like?

[nathan-lapinski]

@scyron6 that sounds good to me!

[Kartik0099]

Hi @nathan-lapinski can i also contribute in this. i will start pick up from middle at contracts/utils/cryptography/ECDSA.sol. okay

[nathan-lapinski]

Sounds great, thanks @Kartik0099 !

[scyron6]

Feel free to take over for me. I'm not going to be able to look at it for a while.

[frangio]

Reopening as I don't think all instances of assembly have been annotated yet.

[Kartik0099]

Hey @nathan-lapinski I have complete below files

contracts/utils/cryptography/ECDSA.sol, contracts/utils/cryptography/MerkleProof.sol, contracts/utils/Create2.sol.

Please update the status it's merged

[nathan-lapinski]

I'll add the remaining in-progress files to #3384 .

[frangio]

Fixed in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3384 and https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3392.