search

ERC725Alliance / ERC725

Repository for code and discussion around ERC725 and related standards
Apache License 2.0
127 stars 66 forks source link

Batch payable comment #195

Open zcstarr opened 1 year ago

zcstarr commented 1 year ago

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: https://github.com/Uniswap/v3-periphery/issues/52

YamenMerhi commented 1 year ago

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 commented 1 year ago

I can take a stab at it