1

https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/ provides a simple example related to using a Reentrancy Guard.

Another approach to preventing reentrancy is to explicitly check for and reject such calls. Here’s a simple version of a reentrancy guard so you can see the idea:

contract Guarded {
     ...

     bool locked = false;

     function withdraw() external {
         require(!locked, "Reentrant call detected!");
         locked = true;
         ... // Multiple Payments
         locked = false;
    }
}

But here I think there is another issue where if reentrant call is detected withdraw() fuction will be returned while locked remain true. Hence we will not able to re-enter into the withdraw() function in futher calls since locked remained as true.


[Q] Is it safe to do mutliple different payments in between the locks? An example will be as follows:

contract Guarded {
     ...
     bool locked = false; 

     function withdraw() external {             
         require(!locked, "Reentrant call detected!");
         locked = true;

         // Payment-1         
         (bool success, ) = msg.sender.call.value(balanceOfPayment1[msg.sender])("");
         require(success, "Transfer failed.");
         balanceOfPayment1[msg.sender] = 0;

         // Payment-2         
         (bool success, ) = msg.sender.call.value(balanceOfPayment2[msg.sender])("");
         require(success, "Transfer failed.");
         balanceOfPayment2[msg.sender] = 0;

         locked = false;
    }
}
2

No.

Your Mutex (locked) will prevent reentrance but reentrance is not the only vulnerability.

A DoS is possible and it might not be intentional.

This line, after payment 2:

require(success, "Transfer failed.");

Have you considered that payment 1 will not happen if the transaction aborts at this stage? That means player1 doesn't get paid unless player2 cooperates.

Many devs assume that a payment cannot be rejected and that doing so is undesirable so it probably wouldn't happen, but those assumptions will get you into trouble.

  1. The default behaviour for a contract is to reject funds that arrive at the fallback function because contracts usually have accounting requirements and might require extra data - things that are not included in such a transfer. That would be a .call. The safest thing for a contract to do when unexpected money arrives without explanation is to revert, and in doing so, probably make the whole transaction revert. Your function will completely revert the whole transaction (both transfers) if either transfer fails.
  2. There might be a financial incentive to make your contract fail. For example, it might lock up the funds and cause embarrassment or disaster to a competitor or just for entertainment.
  3. If this sort of thing is in a loop over a list of contributors (or unrolled) an attacker just has to arrange things so one of those recipients is a contract, e.g. enrolling the contract and making a small refundable contribution.
  4. I'm aware you will read elsewhere about techniques for detecting and avoiding contracts but that method will not defend against the attack and will probably diminish the usefulness of your project, so avoid discriminating on that basis.

The Withdraw pattern is recommended. That is, do the accounting and have the users claim their funds with a function built for that purpose. That way, there is only one "untrusted" participant in each interaction, the msg.sender. Doing so separates concerns so they are less able to interfere with each other.

For clarity. The lock should do its job, namely to prevent reentrance, but multiple transfers in a single transaction is a bad direction because it brings its own considerable hazards.

Hope it helps.

|improve this answer|||||
  • I got your point, but what if paymen-1 and payment-2 are dependent to each other, and their payment is made to the same user (msg.sender). Thus if one is failed, all should be failed seems fair to me. – alper Jan 4 at 12:35
  • 1
    Right. You have to reason about what is correct. Common cases include "send everyone refunds, royalties, dividends, whatever" ... "Send everyone" is a giveaway that DoS is a possible problem and withdraw pattern is a solution. – Rob Hitchens Jan 4 at 20:28
  • Could you also share some links/tutorial related to withdrawn pattern? So basically, you recommend recipient to send a transaction to withdraw and obtain their funds.Thus will eliminate the mutliptle transactions all once – alper Jan 4 at 20:43
  • 1
    The subtraction is guarded by a simple require so there is no problematic scenario. SafeMath would catch the same condition. My reason for not using SafeMath was mainly to keep the example on point for folks who aren't familiar with libraries, and using, etc. Non-trivial apps with many paths get harder to reason about so a good habit is to use SafeMath for everything unless you are certain there is no possibility of trouble and you want to squeeze a little gas out of it - usually in the latter stages of development when you are optimizing. – Rob Hitchens Jan 5 at 1:33
  • 1
    You could drop the require that guards it and go with balances[ms.sender] = balances[msg.sender].sub(amount); and that would catch the NSF issue. As long as you understand the options and the problem with doing nothing, you're in good shape. – Rob Hitchens Jan 5 at 1:36

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.