search

functionalfoundry / ethereum-htlc

HTLC for doing cross-chain atomic swaps
MIT License
7 stars 2 forks source link

Is the reentrancy protection necessary? #3

Open Jannis opened 6 years ago

Jannis commented 6 years ago

@yanivtal I noticed you added reentrancy protection to the HTLC contract:

https://github.com/functionalfoundry/ethereum-htlc/blob/ba565de54ded8e7440c7452a88f4dd7b03440a6d/contracts/HTLC.sol#L26-L34

Is this necessary at all? My current (potentially flawed) understanding is that this is only necessary when calling external contracts, allowing the execution of the message (in this case complete or reclaim) to be interrupted. Since we're not doing that I wonder if we could get away without the extra logic.

AFDudley commented 6 years ago

https://lightrains.com/blogs/smart-contract-best-practices-solidity http://hackingdistributed.com/2016/07/13/reentrancy-woes/

If the code is fully tested, we should know if the code block in question is needed or not. :)

Jannis commented 6 years ago

PR #4 includes a proposal to ditch our own reentrancy guard in favor of the one from OpenZeppelin. Regardless of whether or not the guard is necessary at all, what do you think about re-using their contracts in general?

yanivtal commented 6 years ago

@Jannis totally up for reusing industry-tested code wherever possible.

From the Consensys security best practices doc:

someAddress.send()and someAddress.transfer() are considered safe against reentrancy. While these methods still trigger code execution, the called contract is only given a stipend of 2,300 gas which is currently only enough to log an event.

https://github.com/ConsenSys/smart-contract-best-practices

I didn't know about their gas stipend so it looks like in this case you're probably right. Still worth putting a test in to verify. Haven't written a test to catch reentrancy yet but that would be a worthwhile tool to have in our tool belt.