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

Have Escrow forward all gas to recipient #1668

Closed
nventuro opened this issue Mar 7, 2019 · 1 comment · Fixed by #1976
Closed

Have Escrow forward all gas to recipient #1668

nventuro opened this issue Mar 7, 2019 · 1 comment · Fixed by #1976
Labels
contracts Smart contract code. on hold Put on hold for some reason that must be specified in a comment.

Comments

@nventuro
Copy link
Contributor

nventuro commented Mar 7, 2019

One of the reasons why PullPayment is useful is that by using _asyncTransfer, the 'payment' is decoupled from any possible recipient contract logic, and any code executed on an Ether transfer (e.g. a fallback function) will only run once withdrawPayments is called.

However, said function calls Escrow.withdraw, which does a transfer to the recipient. This means only 2300 gas is forwarded, which is only enough to e.g. log an event. This severely limits the usefulness of PullPayment (and Escrow): we should change this transfer for a call, along with the required checks (e.g. make sure the call actually succeeded, etc.).

@nventuro nventuro added improvement contracts Smart contract code. labels Mar 7, 2019
@nventuro nventuro added this to the v2.3 milestone Mar 7, 2019
@nventuro nventuro removed this from the v2.3 milestone Apr 17, 2019
@nventuro
Copy link
Contributor Author

We've been discussing this a bit, and it's not entirely clear whether or not this could inadvertently create unsafe reentrant scenarios, so we're pausing development on this one until we have a clearer idea of the consequences of our decision.

@frangio frangio added the on hold Put on hold for some reason that must be specified in a comment. label Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code. on hold Put on hold for some reason that must be specified in a comment.
Projects
None yet
3 participants
@frangio @nventuro and others