Smart Contract Anti-Patterns: Why sending Ether is not always a good idea?

Some smart contract sometimes need to send ether, for example:

1. ICO crowd sale contract sends ether to a vault, where the funds are collected;
2. Returning ether as the change for performing operations. Sender may send uneven ( or exceeding ) amount of ether to the contract, and the contract should send the change back;
3. Blockchain game makes a payout to a winner in Ether;
4. Interactive ICO returns funds to token buyers on request;
5. Distributed fund payouts dividends to participants;
6. CPA Reward Contract makes payments to affiliates;
7. Many other cases, when smart contract is responsible for distributing money between people and/or other contracts.

Apparently, the code similar to the following appears in the contract:

/// now it is time to send some_amount to receiver...
receiver.transfer(some_amount);

In most cases, this code will not cause problems, but let’s discuss the cases when it does. The problems with this code can be caused by it’s usage in the contract, and the nature of the receiver.

Problem #1: Applicable Gas Limit

Imagine, that the receiver is not an external address, but rather another contract, which has something like this:

function() public payable { 
/// do something very important... and consume gas
}

This function tells, that the contract is ready to receive ether, but it is also going to make something with it, say store some data or calculate some values based on it. This kind of functions are typical in ICO crowd sale contracts, decentralized funds, multi-signature wallets and other greedy creatures.

What is important from the sender point of view, is that this function will consume gas, and then, the line in your sending contract:

receiver.transfer(some_amount);

will fail. Transfer function can use only a limited “gas stipend” (2300) amount of gas to make a transfer. Any more or less complex processing at receiving side will just make raise the error in your smart contract.

Solution for this particular problem is to use another function, call :

receiver.call.value(some_amount)();

This is different from the transfer function is that it forwards all remaining gas as a limit for a function call. This allows the caller of your contract to raise the gas limit and still execute the transfer in case Out of gas error happens.

This function has ability to specify the gas limit, if “all remaining gas” is unsecure:

receiver.call.value(some_amount).gas(100000)();

Problem #2: Lack of check the result of transfer

If you have the following line in your contract, you may be in a big problem:

receiver.send(some_amount);

The problem is that send does not raise any errors in case the transfer did not happen ( for example, because of Out of gas). You should change it to one of the following ( the last is the best option):

require(receiver.send(some_amount); // check the result, or
receiver.transfer(some_amount); // raise exception in case, or
receiver.call.value(some_amount)(); // get rid of gas limit

Problem #3: Mixing transfer and logic in contract

Generally, mixing the logic or cycles with ether transfer is always a bad idea. But in some cases it is even worse:

/// DO NOT DO THIS!
address[] lovers;
for(uint i=0;i<lovers.length;i++){
  lovers[i].transfer(some_amount);
}

Or:

/// DO NOT DO THIS!
winner.transfer(some_amount);
publisher.transfer(other_amount);
developer.transfer(other_amount);

In general, try to avoid sending ether with logic, as the transfer operation may fail, and this is not what you expect from your logic. However, there may be more significant problems:

1. One “bad” receiver may completely block your function in contract, if it has an error in his contract on payable function ( or consuming too much gas). Thing get worse, if you rely on your smart contract to change the state of your contract, and then you will never be able to do this.
2. Ethereum has a limit of stack depth 1024. It means, that if you send ether to other contract and in summary you exceed stack trace depth, the transfer will also fail.
3. Re-entrancy problem appears, when receiving contract calls your contract back, when your function is not yet completed.

The later allows an attacker to withdraw more money than you may expect. For example, if your code looks like:

function withdraw() public {
        if (msg.sender.call.value(shares[msg.sender])())
            shares[msg.sender] = 0;
    }

and the attacker, calling withdraw() has the code like:

uint need_more;
function() public payable { 
   if(need_more<1000){
     msg.sender.withdraw();
     need_more += 1;
   }
}
function attack(address fund) public {
 fund.withdraw();
}

Then attacker can withdraw 1000x times more that you expected.

These types of errors are extremely hard to diagnose, find and test and better to completely avoid them by not mixing code and ether transfer.

What can be done instead? There are at least two solutions to the problem:

1. Use pull payment pattern
2. Transfer tokens instead of ether

Pull Payment Pattern

Idea is quite simple — when you want to transfer money to a receiver, do not make an actual transfer, but store the balance in contract store map.

Receiver will then need to call “withdraw” method to “pull” money to his address. The pattern is published in Zeppelin library, and essentially it consists of 2 functions ( simplified ):

/// make a deposit on dest account
function asyncSend(address dest, uint256 amount) internal {           payments[dest] = payments[dest].add(amount);  
}
/// let account to withdraw:
  function withdrawPayments() public { 
   .... some checks here ...
   assert(payee.send(payment));  
}

The full source is available at https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/payment/PullPayment.sol

The only real minus for this scheme is that the payee require to call non-standard contract function withdrawPayments and it may be not available in his or her favourite wallet or software.

Example of this pattern can be found in IZX game platform contract:

izetex/contracts

Transfer tokens instead of ether

Instead of transferring ether, you can give ERC-20 tokens to receiver, in the same amount as ether. The tokens will represent “the debt” to the receiver.

Receiver then can send tokens to the contract and get back the Ether using standard wallet software, supporting ERC-20 tokens.

This model used in DAO Ready interactive ICO contract:

daoready/interactive-ico

Resume

There is a number of rules that you should follow to successfully operate Ether values in your Smart Contracts:

1. Expect that the receiving side can be another contract, consuming gas, executing code, and calling your contract back;
2. Always check the result of the transfer if using send method;
3. Use call() method to pass all remaining gas to the payable function of receiver;
4. Do not mix logic and ether send. Instead use one of approaches mentioned above ( token debt transfer or pull payment patterns )