-
Notifications
You must be signed in to change notification settings - Fork 67
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
Batch payable comment #195
Comments
Hey @zcstarr , thanks for opening the issue! |
I can take a stab at it |
andyogaga
pushed a commit
to andyogaga/ERC725
that referenced
this issue
Jul 12, 2023
Bumps [shell-quote](https://github.com/substack/node-shell-quote) from 1.7.2 to 1.7.3. - [Release notes](https://github.com/substack/node-shell-quote/releases) - [Changelog](https://github.com/substack/node-shell-quote/blob/master/CHANGELOG.md) - [Commits](https://github.com/substack/node-shell-quote/compare/v1.7.2...1.7.3) --- updated-dependencies: - dependency-name: shell-quote dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary:
Regarding the batch version of
_execute
, it might be worth adding a comment about themsg.value
parameter and how it interacts with the delegate call operation. If a method is looking at msg.value to check for payment and is called by_execute
, the method might receive stale information, which could result in an unintended action being enabled. At least on the surface it looks like this is the case :).I encountered this issue while working with Multicall and it seems like it could be relevant to contracts that implement the full spec such as ERC725X.
Adding a comment about msg.value to the code might be helpful, especially since it is a payable parameter. There are currently comments about the state on the delegate call, but msg.value is worth mentioning as well.
Link to a related issue: Uniswap/v3-periphery#52
The text was updated successfully, but these errors were encountered: