Remove HasNoEther, HasNoTokens, HasNoContracts, and NoOwner

[frangio]

Reasons for removal:

• HasNoEther: it's not really possible to prevent ether from being received. As a mixin for other contracts it's not very safe because it adds a reclaimEther function that can play badly with the rest of a contract.
• HasNoTokens: it relies on ERC223 to reject token transfers, so it does not affect ERC20 and is misleading. On top of that, ERC223 doesn't seem to be in use.
• HasNoContracts: the name should rather be CanReclaimOwnership, and this doesn't seem like a generally useful feature.
• NoOwner is just all of the above put together.
• Contactable can simply be replaced by a constant public string variable.
• Destructible and TokenDestructible: selfdestruct should be used with tons of care because it's not always safe to use, so it shouldn't be as simple as inheriting a contract.

[shrugs]

L G T M

[come-maiz]

I'm +1 no everything, except on TokenDestructible.
On an audit we found that it was possible to destroy the contracts and leave the tokens inaccessible. This was easily solved by using TokenDestructible instead of Destructible. Maybe, move it to examples with some comments about the things every project should pay attention to?

[frangio]

@ElOpio That's interesting but it only motivates me to remove Destructible as well (an idea that grew on me overnight). selfdestruct should be used with tons of care, so it shouldn't be as simple as inheriting Destructible.

[come-maiz]

@frangio you might be right. But it's even more dangerous to have people destroying their stuff without any guidelines at all. I'll leave my +1 here, we can figure out later what to do. And .md on the docs site, an FAQ question or moving the contracts to the examples could be good solutions.

[nventuro]

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?

[nventuro]

(if the answer is yes, we should obviously open an issue instead of addressing this here)

[shrugs]

yeah. should probably just remove the asyncTransfer to fix this

[frangio]

It's true, that's a problem. How does removing asyncTransfer fix it though?

[shrugs]

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.

[frangio]

I had to fix conflicts. Can someone review again please?

[come-maiz]

thanks @frangio

[frangio]

Thanks!