Can we say a solidity function is an atomic operation?

Question

Look at this very basic solidity function:

function myfunction() external {
  address payable address1 = 0x....;
  address payable address2 = 0x....;
            
  address1.transfer(1 ether);
  address2.transfer(1 ether);
}

Let's suppose the second transfer will fail. There are many reasons for that (insuffisant funds in the contract's balance, ). I want to be sure the first transfer will be cancelled in this case.

I suppose a solidity function is "atomic". I mean if something fails in the function, every operation made in the function is canceled.

I have test and i have see the first transfer is cancelled. I am happy to see that.

But somebody told me what i was doing is a bad practice. He told me i should write a "pull-payment" pattern. My question is why should i do that.

Is there some cases where a solidity function is not "atomic" ?

Thanks a lot

*** EDIT ***

Here is what i mean by "pull payment":

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/payment/PullPayment.sol

strategy, where the paying contract doesn't interact directly with the receiver account, which must withdraw its payments itself. Pull-payments are often considered the best practice when it comes to sending Ether

Answer 1

They probably meant a pattern like this:

mapping(address => uint256) public withdrawableBalance;

function myfunction() external {
  address payable address1 = 0x....;
  address payable address2 = 0x....;
            
  withdrawableBalance[address1] += 1 ether;
  withdrawableBalance[address2] += 1 ether;
}

function withdraw() external {
  uint256 bal = withdrawableBalance[msg.sender];
  withdrawableBalance[msg.sender] = 0;
  msg.sender.transfer(bal);
}

This pattern is useful because:

• It prevents attacks where for example address1 is a contract that can be told to reject ETH transferrals, thereby also causing payments to another user address2 to fail.

• If address1 or address2 is a smart contract, it is able to know where it got the ETH from and perform some action based on that. If you simply .transfer( ETH to a contract, that contract has no ability to even store the address they received it from.

• It reduces the gas cost of myfunction() and shifts that cost to the receiver of the ETH (the caller of withdraw())

• It can reduce the total gas cost if myfunction() is called multiple times, because the balance is added up and can be transferred all at once using withdraw()

Answer 2

Just to amplify @Jesbus's answer.

Yes, it is an atomic transaction - it fails entirely if any part of it fails.

No, it is not a secure pattern. As a general heuristic, avoid interactions with more than one "untrusted" address at a time. Since msg.sender could be anyone, that's one. You could send money to msg.sender without going over-budget, i.e. the "withdrawal pattern."

That pattern prevents interference because Alice cannot interfere with Bob. More generally, it protects the contract from DoS attacks or accidental effects.

Accidents are probable. If Bob is a contract and the contract has a default, non-payable fallback function (it rejects funds) then Alice's funds would be trapped. Bob doesn't even need to be hostile to for that situation to form but everyone's funds would be trapped.

It will work as expected if you know who/what address1 and address2 are, e.g. transferring between your own contracts or addresses which is why I qualify the warning with "untrusted."

Hope it helps.