Is msg.sender.send(msg.value); safe?

Question

Is there any sort of reentrancy attack that could go along with using this line of code in my smart contract?

Answer 1

msg.sender is reliably the immediate, innermost sender that invoked the function. It could be another contract, so it's not necessarily the signer that kicked off the transaction. Hopefully the foregoing doesn't confuse the reliability of it. It reliably does what it is supposed to do.

msg.value is reliably the Ether amount sent to the function. It does not include gas, which is a separate issue. In the case of a chain of contracts. it is not the funds the signer sent. It is the funds that arrived in the function. The caller has to go out of their way to forward funds to another contract.

The statement itself will work as expected, and it will send 2,300 gas (the stipend) which is insufficient to launch a reentrance attack. 2,300 is just enough to emit an event, but not enough to update state, and whatever happens in the fallback function, there will be less than 2,300 leftover. The circuit-breaker depends on pricing, not code logic. It is better to rely on sound logic.

The reentrance defense relies on the order you address certain concerns so it doesn't matter how much gas you send or what the receiver does with it. Check out checks-effects-interactions: https://solidity.readthedocs.io/en/v0.4.24/security-considerations.html

1. Checks: Is this transaction acceptable? Seriously consider revert/require to "fail hard" in the case of an unacceptable request. It's usually the right thing to do.
2. Effects: Now, do all of the bookkeeping, state updates, events, etc. optimistically. This puts your house in order before you transfer flow control to an untrusted contract.
3. Interactions: Finally, interact with call, send or transfer. It ought to be the last thing the function does. If this happens to fail then you need to revert everything you did in step 2. Revert is done automatically for you if you used transfer. The compiler will complain if you do not check the return bool (success?) from send or call.

Reentance attack is possible if those concerns are approached out of order.

Also worth consideration:

send is usually the wrong choice. Consider transfer. If would be equivalent to:

require(msg.sender.send(msg.value)); // silences the unchecked return value complaint

And, why are we doing this? There must be a reason. Suppose only members are allowed to play, and that's why we want to return the money.

require(isMember(msg.sender), "You are not a member."); // fail hard

Since require reverts, the entire transaction must fail for non-members. While there are always exceptions, this is a good default approach.

Hope it helps.