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

Batch payable comment #195

Open
zcstarr opened this issue Feb 16, 2023 · 2 comments
Open

Batch payable comment #195

zcstarr opened this issue Feb 16, 2023 · 2 comments

Comments

@zcstarr
Copy link

zcstarr commented Feb 16, 2023

Summary:
Regarding the batch version of _execute, it might be worth adding a comment about the msg.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

@YamenMerhi
Copy link
Collaborator

Hey @zcstarr , thanks for opening the issue!
I agree there should be more documentation mentioning the edge cases when msg.value is used with a batch call of delegatecall. Multicall is planned to be added to the Account version of ERC725 in LSP0-ERC725Account.
Do you want to add more documentation to the nastpec and the specs, or let someone from the team do it ?

@zcstarr
Copy link
Author

zcstarr commented Feb 22, 2023

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] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants