Skip to content

Remove HasNoEther, HasNoTokens, HasNoContracts, and NoOwner #1254

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.

Sign up for GitHub

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

Merged
frangio merged 8 commits into from
Sep 3, 2018
Merged

Remove HasNoEther, HasNoTokens, HasNoContracts, and NoOwner #1254

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0a78670
remove HasNoEther, HasNoTokens, HasNoContracts, and NoOwner
frangio Aug 29, 2018
9dd1308
remove unused ERC223TokenMock
frangio Aug 29, 2018
8547879
remove Contactable
frangio Aug 29, 2018
11cbf5b
remove TokenDestructible
frangio Aug 29, 2018
4f3d67f
remove DeprecatedERC721
frangio Aug 29, 2018
d131f73
inline Destructible#destroy in Bounty
frangio Aug 30, 2018
f334bdf
remove Destructible
frangio Aug 30, 2018
0538ebd
Merge remote-tracking branch 'upstream/master' into remove-has-no-x
frangio Sep 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
inline Destructible#destroy in Bounty
  • Loading branch information
@frangio
frangio committed Aug 30, 2018
commit d131f73bb77c00c888e0fb7b98933610735194c4
10 changes: 8 additions & 2 deletions contracts/Bounty.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ pragma solidity ^0.4.24;


import "./payment/PullPayment.sol";
import "./lifecycle/Destructible.sol";


/**
* @title Bounty
* @dev This bounty will pay out to a researcher if they break invariant logic of the contract.
*/
contract Bounty is PullPayment, Destructible {
contract Bounty is PullPayment, Ownable {
bool public claimed;
mapping(address => address) public researchers;

Expand Down Expand Up @@ -47,6 +46,13 @@ contract Bounty is PullPayment, Destructible {
claimed = true;
}

/**
* @dev Transfers the current balance to the owner and terminates the contract.
*/
function destroy() public onlyOwner {
Copy link
Contributor

@nventuro nventuro Aug 31, 2018

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 calling claim, only to be front-runned by the owner, who retrieves the reward and now has information on a vulnerability on his contract?

Copy link
Contributor

@nventuro nventuro Aug 31, 2018

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)

Copy link
Contributor

@shrugs shrugs Aug 31, 2018

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

Copy link
Contributor Author

@frangio frangio Aug 31, 2018

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?

Copy link
Contributor

@shrugs shrugs Aug 31, 2018

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.

selfdestruct(owner);
}

/**
* @dev Internal function to deploy the target contract.
* @return A target contract address
Expand Down