0

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!

  • You should make sure that function receive cannot be re-entered (for example, by requiring that a boolean state variable is false at the beginning of the function, then setting it to true, then setting it back to false at the end of the function). – goodvibration May 27 at 19:46
  • You should also add function before receive() external payable, but I guess that's just a typo of yours. – goodvibration May 27 at 19:47
  • @goodvibration Would you clarify what you mean by "re-enter" and put that along with both your comments in an answer? I'd accept your answer. Thank you! – blockrookie May 29 at 14:08
0

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.

| improve this answer | |
  • Thank you so much, @goodvibration! Could you clarify what you mean by "re-enter"? As I understand your code, the function can now only be executed once at a time, since it's locked during execution. But why did you do that? How would someone be able to abuse the contract if there were no such lock? – blockrookie May 29 at 16:53
  • @blockrookie: Done; see updated answer. – goodvibration May 29 at 18:39
  • Hey, @goodvibration, I believe the function keyword should actually not be present in this case, at least according to my understanding of the latest docs: solidity.readthedocs.io/en/v0.6.8/… – blockrookie May 31 at 19:45
  • @blockrookie: Interesting! This seems to be a new feature of solc 0.6.x, which was not available up until (and including) solc 0.5.x. A good description of it, as well as the difference between that function and the fallback function, is available in this answer. – goodvibration May 31 at 19:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.