-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Remove HasNoEther, HasNoTokens, HasNoContracts, and NoOwner #1254
Changes from 7 commits
0a78670
9dd1308
8547879
11cbf5b
4f3d67f
d131f73
f334bdf
0538ebd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
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.
This is not strictly related to this PR since it's a Bounty concern, but doesn't this open for the possibility of a researcher finding a vulnerability, exploiting it (i.e. causing
_target.checkInvariant()
to return false) and then callingclaim
, only to be front-runned by the owner, who retrieves the reward and now has information on a vulnerability on his contract?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.
(if the answer is yes, we should obviously open an issue instead of addressing this here)
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.
yeah. should probably just remove the asyncTransfer to fix this
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.
It's true, that's a problem. How does removing
asyncTransfer
fix it though?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.
ah, I thought the atomic swap would prevent the frontrunning here, but they could still watch for claim transactions that would succeed and then destroy the contract ahead of time.
sounds like we need a commit reveal timeout here; when a researcher calls
createTarget
this starts a timeout that must expire before the contract can be destroyed. the researcher then calls claim, which checks for a valid commitment and checks the invariant, then pays out synchronously and invalidates the timeout.the timeout allows the owner to destroy the contract after it's expired, in the case that someone calls
createTarget
but doesn't exploit the target. A more comprehensive bounty contract would have researchers provide some stake of their own to prevent sybil attacks and infinitely blocking a bounty-supplier's ETH for the cost of gas.