smithclay / etherimp Goto Github PK

The function close() calls selfdestruct(creator) on the contract, and allows the creator to steal the Ether locked up in the contract. Selfdestruct(creator) will send whatever Ether is stored in the contract to it's creator. You can use this, by first selling the etherimp, and the calling self-destruct on it, to claim what the buyer bought it for.

Due to #1, even when there's only 1 wei left in the contract, the owner will still be able to get his initial deposit back, and whatever he sold the etherimp for, when calling selfdestruct, as the leftover wei will have accumulated to that amount.

I don't know if this was intended or not, but now I don't see a point in sending Ether to your contract because of this line:
previousOwner.transfer(msg.value);

Basically, this transfers new lowest price to the previous owner. So, imagine that I sent 0.03 Ether to your contract and became a king. Then you sent 0.01 Ether and stole my crown. I will be refunded with 0.01 Ether, so my 0.02 Ether is lost and can only be retrieved by the owner of the contract (you) by calling close() function. I think that's unfair and you should fix it.

modifier onlyOwner {
    require(msg.sender == currentOwner);
    _;
}

This modifier is not used anywhere in the codebase so there is no point in keeping it.

Because of this line:
previousOwner.transfer(msg.value);
I can become a permanent owner of your contract.
Example:
I send Ether to your contract not from my wallet but from another smart contract. Inside this smart contract, I have a custom Fallback function which automatically throws when someone sends Ether to my contract.
function () payable {
revert();
}
So, when I become a king of your contract and someone wants to steal my title, they send less Ether to your contract and your contract tries to refund me. But because of my Fallback function, your contract will never succeed and the transaction will be reverted. So I am the owner of your contract forever. I hope you understood the possible malicious scenario.

A solution would be to prefer pull payments instead of push. Basically, you should write another function which let's previous owner retrieve his stake.