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

malicious franchisee can lock funds for full duration of trust #1

Closed
NickErrant opened this Issue Nov 7, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@NickErrant

NickErrant commented Nov 7, 2017

After start of trust, current franchisee can set franchisee to address of a contract with fallback function that always throws, preventing withdraw from functioning. Assuming custodian does not stop uploading conversion rates (which is likely as they are designed to provide conversion rates for more than one trust), the funds will be locked for the full duration of the trust.

@skilesare

This comment has been minimized.

Owner

skilesare commented Nov 7, 2017

Awesome catch. The suggestion is to always use a withdraw pattern. Any idea of a way to catch that kind of throw in solidity and continue unmolested?

@NickErrant

This comment has been minimized.

NickErrant commented Nov 8, 2017

The most secure way would be separating withdraw into separate functions for each of the relevant parties so that they can only make their own transaction fail (as you said, always use the withdraw pattern). However, I understand how that could be suboptimal for UX purposes.

A possible workaround is changing the franchisee's--and only the franchisee's--transfer() to send(). When send() fails an exception won't be propagated, instead it returns false. This would close up the locking issue, but potentially leave a franchisee without malicious intent out of a payment if their transfer fails for whatever reason.

Perhaps the two approaches could be combined: keep track of whether franchisee's send was successful, and if it was not, allow the franchisee to withdraw the missed payment through their own withdraw function.

@skilesare skilesare closed this in a2db361 Nov 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment