0

I wrote a simple contract to split balance into 2 addresses. I made a separate withdraw function because I want to use these for mining rewards and unclear if the default function would get called on a mining reward "deposit". (Would it?)


pragma solidity ^0.4.0;

contract SplitBalance{

    address a1;
    address a2;

    function SplitBalance(address _a1, address _a2) public{
        a1 = _a1;
        a2 = _a2;
    }

    function withdraw() public {
        uint money = this.balance;

        //only withdraw in even amounts to avoid rounding loss
        if (money % 2 == 1)   //money is an odd number, make it even
           money = money - 1; //don't worry, it will stay in the balance for next time

        a1.transfer(money/2);
        a2.transfer(money/2);
    }

    function updateAddress1(address newAddress) public {
        if(msg.sender != a1)
           revert();
        a1 = newAddress;
    }

    function updateAddress2(address newAddress) public {
        if(msg.sender != a2)
           revert();
        a2 = newAddress;
    }

    //fallback function that allows ether to be sent to it
    function() payable public { }
}
1

It's unsafe to do this:

a1.transfer(money/2); 
a2.transfer(money/2); 

...because either recipient could cause an error which would revert the whole transaction and prevent the other party from withdrawing.

Also, these two successive calls to untrusted contracts are a reentrancy code smell, although in this case I can't think of a way to call withdraw() from the receiver contract without causing an error and reverting the whole thing.

You could avoid these problems by making a split function that reassigns the balance into an internal balance for each account, then letting each receiver withdraw their own balance.

5
  • "I can't think of a way to call withdraw() from the receiver contract without causing an error and reverting the whole thing." Actually, that's a huge problem. If either a1 or a2 is a contract with a fallback function that reverts(), the other party will never be able to get his money. Also the contract would be rendered unusable as the only one that can change either a1 or a2 is themselves. Unless the attacker also implements a function to call updateAddress1/2. Oct 27 '17 at 22:03
  • That's what I mean with my initial comments about causing an error that reverts the whole transaction and prevents the other party from withdrawing. The bit I couldn't think of a way to attack was for a1 to call withdraw() in its fallback function and take more money than it's entitled to. I guess that will fail because the whole thing will revert when it tries to pay a2, but like I say it's a code smell to even have to worry about this. Oct 27 '17 at 22:13
  • Thanks. So are you saying there is no way to pay more that one address safely from one function? Is there any possibility to catch the error and move on to the next operation?
    – Solomon
    Oct 29 '17 at 3:28
  • You could use send() instead which will swallow the error and return true or false. But it's easy to get this stuff wrong, so it's generally preferred to use the withdrawal pattern: solidity.readthedocs.io/en/develop/common-patterns.html Oct 29 '17 at 11:55
  • Now I wrote it with maintaining an internal balance but I get a warning in Remix saying: Fallback function of contract browser/SplitBalance1.sol:SplitBalance requires too much gas (40612). If the fallback function requires more than 2300 gas, the contract cannot receive Ether. function() payable public { uint amount = msg.value/2; account1 = account1 + amount; account2 = account2 + amount; }
    – Solomon
    Oct 30 '17 at 12:31
0

If the contract balance is 0 it will cause problems as money variable will underflow when you substract 1 from it. You should require the contract balance be >0.

1
  • 1
    They could just remove that stuff about checking for an even number and decrementing. If it's odd the division will round it down. Oct 27 '17 at 21:59

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.