What are the flaws of this example contract?

Question

I am currently writing a paper/article on tokenization, in the beginning of which I introduce smart contracts. To this end, I present the following contract as an example:

pragma solidity ^0.6.0;

contract SplitPot {
  address payable[] beneficiaries = [
    0x498898b3F52DAba1bB304a4b4D2EA31a111484B1,
    0xAcb19c763EB67ea757Efd8Cd8b6ecceb28F1284B,
    0xD5d3f3650C4DdE7B8034671129443A596Ce8ed57
  ];

  receive() external payable {
    uint individualAmount = msg.value / beneficiaries.length;

    for (uint i = 0; i < beneficiaries.length; i++) {
      beneficiaries[i].transfer(individualAmount);
    }
  }
}

Obviously, the purpose of the contract is to evenly distribute all Ether sent to it among the three beneficiaries (all of which are testnet accounts I created). It works on testnet.

For this specific purpose, i.e., explaining what smart contracts are, I believe this is a very good contract:

• It is very short and easy to understand.
• It does something that is usually facilitated by a trusted third party -- which is now obsolete.

I also know this is a very bad contract, as it has severe shortcomings. For example: What if a transaction fails? Afaik, the beneficiaries have no way of recovering their share of Ether, and the contract doesn't re-attempt to send it to them. Also: I don't know whether the contract would execute correctly on mainnet, too.

I want to inform my readers that this is, in fact, a flawed contract, and I want to let them know what specifically can go wrong. As I just started learning about Ethereum, I'd be very thankful if you helped me identifying these flaws -- especially with regards to gas economics and coding style.

The paper/article will be licensed CC BY-SA 4.0, so I am not asking for help on commercial work!

Answer 1

You should make sure that function receive cannot be re-entered, for example:

contract SplitPot {
    ...

    bool private locked = false;

    function receive() external payable {
        require(!locked, "reentrancy attempted");
        locked = true;
        ... // put your actual code here
        locked = false;
    }
}

Note that you seem to have forgotten declaring function prior to receive() external payable in your code, and that I have added it here.

---

Explaining the reentrancy-protection:

Your function executes beneficiaries[i].transfer(individualAmount);.

If beneficiaries[i] is the address of a contract, then the fallback function of that contract will be invoked; subsequently, that function can call your function and abuse its intended goal.

---

Update:

Following the comment by @blockrookie, the receive keyword was introduced in solc 0.6.x.

The unnamed function commonly referred to as “fallback function” was split up into a new fallback function that is defined using the fallback keyword and a receive ether function defined using the receive keyword.

Both functions are declared without the function keyword, hence my note above ("you seem to have forgotten declaring function prior to receive() external payable in your code") is relevant under solc 0.5.x or lower, but not under solc 0.6.x or higher.

More details about this function/keyword can be found in the official documentation.