Why 'throw' is a problem in this Solidity code?

Question

I found this code at the following site:

Ethereum Griefing Wallets

for (uint i=0; i<investors.length; i++) {
  if (investors[i].invested == min_investment) {
    // Refund, and check for failure. 
    // This code looks benign but will lock the entire contract
    // if attacked by a griefing wallet.
    if (!(investors[i].address.send(investors[i].dividendAmount))) 
      { 
        throw;
      }
    investors[i] = newInvestor;
  }
}

I understand that using a loop to send balance is dangerous because if one transaction fails then all transaction would fail. Why 'throw' is considered dangerous in this code? Just because it would revert and the owner would not get any money? How it would lock the contract? Because 'throw' consumes all the gas?

Somebody please guide me.

Answer 1

Throw, itself, is not dangerous.

However, this not a good pattern for multiple reasons.

1. If investors[i].address is a contract, and that contract has fallback function that is not payable (as is the default for all contracts with current compilers), then that contract cannot receive funds this way.
2. As you have noted, if any recipient rejects the funds for any reason, then all such transfers will fail. This can happen by accident (see point 1) and it can also be an easy attack vector for a denial-of-service attack. Ouch!
3. It does not scale because the for loop will run out of gas at some number n of investors, beyond which the function would be inoperable. https://blog.b9lab.com/getting-loopy-with-solidity-1d51794622ad

You could:

1. Check for failure and carry on, but ...
2. then, you would have to create another withdrawFunds() function to offer another way to retrieve funds, if the first method fails.

If you create that withdrawFunds() function, you will find that doing so eliminates the need for the iterating function. This is why the withdrawal pattern is a best practice. All three of the original concerns vanish.

Hope it helps.