Assembly Memory Safety #859
Merged
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 does a once-over on memory Safety for the 1.5.0 contracts. In particular, there were a couple of missing `memory-safe` tags for some assembly blocks which were preventing the IR assembler from working correctly. Additionally, since the `MultiSend*` contracts changed anyway in 1.5.0, I took this opportunity to change the assembly to be memory-safe so that we can tag it. Note that it only adds more code in the revert case, so it should not have a negative impact for most use-cases. Furthermore, I added a comment explaining why we did not make the `SafeProxy` contract memory-safe (that is one intentional). Lastly, I noticed that there were some `eq(..., 0)` assembly calls which can be written as `iszero(...)` to save some gas and code.
remedcu
approved these changes
Nov 14, 2024
Co-authored-by: Shebin John <[email protected]>
mmv08
reviewed
Nov 15, 2024
| @@ -61,9 +62,10 @@ contract MultiSend { | |||
| case 1 { | |||
| success := delegatecall(gas(), to, data, dataLength, 0, 0) | |||
| } | |||
| if eq(success, 0) { | |||
There was a problem hiding this comment.
I believe the reason we didn't do memory-safe assembly here is the same as the SafeProxy contract, it gets compiled in its unit. Why do we need to make it memory-safe? (not opposed to the change, just wondering)
There was a problem hiding this comment.
it gets compiled in its unit.
It does, but if you use Foundry and deploy MultiSend with other Safe contracts in a script (how I found the issue), then it gets it would get included in the same compilation unit. This is different from the SafeProxy contract, since it gets compiled in a separate unit first for it to be used in the SafeProxyFactory implementation.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 PR does a once-over on memory Safety for the 1.5.0 contracts. In particular, there were a couple of missing
memory-safetags for some assembly blocks which were preventing the IR assembler from working correctly.Additionally, since the
MultiSend*contracts changed anyway in 1.5.0, I took this opportunity to change the assembly to be memory-safe so that we can tag it. Note that it only adds more code in the revert case, so it should not have a negative impact for most use-cases.Furthermore, I added a comment explaining why we did not make the
SafeProxycontract memory-safe (that is one intentional).Lastly, I noticed that there were some
eq(..., 0)assembly calls which can be written asiszero(...)to save some gas and code. Again, it was an opportunistic change as the affected contracts have changed anyway and will be re-audited.