-
Notifications
You must be signed in to change notification settings - Fork 256
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
Allow deposit from non-token owner #102
base: master
Are you sure you want to change the base?
Allow deposit from non-token owner #102
Conversation
Thanks @bogdan , we'll review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depositor
is not checked how it's related with token to deposit but used just for event. I think we need to check how depositor is related with msg.sender or tokenId
Do you think it makes sense to do? Once a predicate is approved for token, anyone can call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdan @zorancuc According to this PR, once the approval to mintableERC20Predicate is given then anyone can come and deposit on behalf of user. This is good for your use case but not in general terms.
Let say i just gave approval to mintableERC20Predicate but afterwards i decided not to bridge it. but now anyone can come and deposit on my behalf. You see, i have ERC721 today.. but tomorrow someone deposited it for me on polygon. Although it will be in my account, but i will have to spend ETH to bring it back on ethereum (root) chain
@nitinmittal23 I think this scenario is quite surreal for the following reasons:
|
Wanted to follow up and see if there are any plans on merging this functionality into production. We're in a scenario where we would like for a user to be able to deposit to polygon by interacting with our contract first (to send some metadata via state sync). However, it looks as though that would require the user to approve our contract, our contract then can transfer tokens to itself, from which it can then approve the predicate then call the deposit function - resulting in excess gas use compared to the scenario where we can specify to the bridge a different source for tokens other than msg.sender. I see the argument that can be made that this opens up the possibility of griefing by a malicious third party; however, I think it's reasonable to assume that it is a highly problematic scenario since the attacker would need to pay the entire fee (which is comparable to the amount the user would need to pay to bring the tokens back to the root chain - i.e. the attacker would be paying fees on the same order of magnitude as to the victim). Additionally, if a user has already approved the predicate transfer, wouldn't the best practice be to operate under the assumption that those tokens are no longer under your ownership anyways, since 1. we could not guarantee that the the permitted address is not invoked in some other manner, and 2. it seems unnatural to provide functionality for the case in which a user 'changes their mind' after an approval and before transferFrom can take place. It feels more like an accommodation for buyer's remorse. |
100% agree with @alexletu. We are having the same scenario and concerns. |
Fixes #101
CC @itzmeanjan