Is this function good enough to refund ether or this security flaw?

Question

I mean:

function refund()
 {

 if (msg.value < 1 ether)
 {

   msg.sender.send(msg.value);

    return;

  }

I'm probably wrong but this looks as if .send will drain the money in the contract if any without checking if msg.sender had contrributed? Shouldn't this check balance first + contributed and if so: how to do it?

I saw this tutorial somewhere but can't be 100% sure it's the correct way?

Answer 1

This is questionable on several levels but the gist of it is reasonable. Roughly,

if the amount of funds received is less than one ether then send back the funds received and return from the function.

Best practices would suggest that it's seldom a good idea to silently fail. This is giving the sender no easy way to differentiate between a success and a failure.

You could fix that with:

function refund() public payable returns(bool success) {

 if (msg.value < 1 ether) {
   msg.sender.send(msg.value);
   return false;
  }
  return true;

You need to add public and payable to get it to work.

Even better. Instead of burdening the sender (which may be a contract) with checking the return result, simplify matters and "fail hard", meaning if this condition is not met then the whole transaction should revert. Reverting the transaction includes returning ether that was sent. In the case of long chains of contract functions calling other functions in other contracts, reverting like this ensures that all branches of the logic revert to their pre-transaction state because this function finds this input unacceptable.

function sendFunds() public payable {
  require(msg.value >= 1 ether); // if expression is false, then revert
  // carry on
}

I renamed it to sendFunds() because that's probably what the sender intends to do. The validation checks are about limiting it to acceptable parameters.

Hope it helps.