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

Remove HasNoEther, HasNoTokens, HasNoContracts, and NoOwner #1254

Merged
merged 8 commits into from
Sep 3, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
10 changes: 8 additions & 2 deletions contracts/Bounty.sol
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

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

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

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

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

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
22 changes: 0 additions & 22 deletions contracts/lifecycle/Destructible.sol

This file was deleted.

36 changes: 0 additions & 36 deletions contracts/lifecycle/TokenDestructible.sol

This file was deleted.

8 changes: 0 additions & 8 deletions contracts/mocks/DestructibleMock.sol

This file was deleted.

33 changes: 0 additions & 33 deletions contracts/mocks/ERC223TokenMock.sol

This file was deleted.

12 changes: 0 additions & 12 deletions contracts/mocks/HasNoEtherTest.sol

This file was deleted.

22 changes: 0 additions & 22 deletions contracts/ownership/Contactable.sol

This file was deleted.

22 changes: 0 additions & 22 deletions contracts/ownership/HasNoContracts.sol

This file was deleted.

41 changes: 0 additions & 41 deletions contracts/ownership/HasNoEther.sol

This file was deleted.

35 changes: 0 additions & 35 deletions contracts/ownership/HasNoTokens.sol

This file was deleted.

15 changes: 0 additions & 15 deletions contracts/ownership/NoOwner.sol

This file was deleted.

15 changes: 0 additions & 15 deletions contracts/token/ERC721/DeprecatedERC721.sol

This file was deleted.

33 changes: 0 additions & 33 deletions test/lifecycle/Destructible.test.js

This file was deleted.

39 changes: 0 additions & 39 deletions test/lifecycle/TokenDestructible.test.js

This file was deleted.

Loading