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

Contract JackpotManager is vulnerable to the locked ether vulnerability #78

Open
thogiti opened this issue Aug 10, 2018 · 3 comments
Open

Comments

@thogiti
Copy link

thogiti commented Aug 10, 2018

I am still meditating on this issue but I have a bit uneasy feeling in the stomach about the JackpotManager. There is a possibility of locked ether vulnerability in the contract model.

As a best practice, from smart contract security perspective, in late 2018, if a contract allows users to deposit ether by calling the deposit function and it should contain a function that would allow users to withdraw the deposited ether.

@hswick
Copy link
Contributor

hswick commented Aug 10, 2018

Are you sure you meant JackpotManager? Perhaps there is a misunderstanding. Tokens going into the jackpot will always eventually get rewarded on forced errors events.

@teutsch
Copy link

teutsch commented Aug 11, 2018

The jackpot should never become empty because then the incentive for verification disappears. Aren't we depositing TRU here rather than ETH?

Welcome @thogiti! Thanks for your feedback.

@thogiti
Copy link
Author

thogiti commented Aug 12, 2018

Thx for the comments. I didn't get a chance to study the complete code yet. Will do in next few days/weeks.

  1. "Stuck Ether Vulnerability" is a bad name for the stuck ether/token vulnerability. We should change to a better name.
  2. Let's say the contract gets frozen from a new security vulnerability that will be discovered in few months. Now your contract is frozen and the amount in the contract is stuck.
  3. This design pattern needs to change and any contract that allows users to deposit money should have a function to withdraw or destroy the deposited amount and the rest of the flow and business logic needs to accommodate the additional withdraw/destroy function.
  4. Jackpot doesn't have to become empty.

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

3 participants