Skip to content

Commit

Permalink
Merge pull request ethereum#2207 from ethereum/wski-develop
Browse files Browse the repository at this point in the history
chore(Docs): Replaced instances if - throw to require() where applicable.
  • Loading branch information
chriseth authored May 3, 2017
2 parents d92fbe6 + e9458be commit 4af0451
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 127 deletions.
51 changes: 22 additions & 29 deletions docs/common-patterns.rst
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ become the new richest.

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

contract WithdrawalContract {
address public richest;
Expand All @@ -52,25 +52,20 @@ become the new richest.
}
}

function withdraw() returns (bool) {
function withdraw() {
uint amount = pendingWithdrawals[msg.sender];
// Remember to zero the pending refund before
// sending to prevent re-entrancy attacks
pendingWithdrawals[msg.sender] = 0;
if (msg.sender.send(amount)) {
return true;
} else {
pendingWithdrawals[msg.sender] = amount;
return false;
}
msg.sender.transfer(amount);
}
}

This is as opposed to the more intuitive sending pattern:

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

contract SendContract {
address public richest;
Expand All @@ -83,12 +78,8 @@ This is as opposed to the more intuitive sending pattern:

function becomeRichest() payable returns (bool) {
if (msg.value > mostSent) {
// Check if call succeeds to prevent an attacker
// from trapping the previous person's funds in
// this contract through a callstack attack
if (!richest.send(msg.value)) {
throw;
}
// This line can cause problems (explained below).
richest.transfer(msg.value);
richest = msg.sender;
mostSent = msg.value;
return true;
Expand All @@ -100,12 +91,16 @@ This is as opposed to the more intuitive sending pattern:

Notice that, in this example, an attacker could trap the
contract into an unusable state by causing ``richest`` to be
the address of a contract that has a fallback function
which consumes more than the 2300 gas stipend. That way,
whenever ``send`` is called to deliver funds to the
"poisoned" contract, it will cause execution to always fail
because there will not be enough gas to finish the execution
of the fallback function.
the address of a contract that has a fallback function
which fails (e.g. by using ``revert()`` or by just
conssuming more than the 2300 gas stipend). That way,
whenever ``transfer`` is called to deliver funds to the
"poisoned" contract, it will fail and thus also ``becomeRichest``
will fail, with the contract being stuck forever.

In contrast, if you use the "withdraw" pattern from the first example,
the attacker can only cause his or her own withdraw to fail and not the
rest of the contract's workings.

.. index:: access;restricting

Expand Down Expand Up @@ -135,7 +130,7 @@ restrictions highly readable.

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

contract AccessRestriction {
// These will be assigned at the construction
Expand All @@ -152,8 +147,7 @@ restrictions highly readable.
// a certain address.
modifier onlyBy(address _account)
{
if (msg.sender != _account)
throw;
require(msg.sender == _account);
// Do not forget the "_;"! It will
// be replaced by the actual function
// body when the modifier is used.
Expand All @@ -169,7 +163,7 @@ restrictions highly readable.
}

modifier onlyAfter(uint _time) {
if (now < _time) throw;
require(now >= _time);
_;
}

Expand All @@ -190,8 +184,7 @@ restrictions highly readable.
// This was dangerous before Solidity version 0.4.0,
// where it was possible to skip the part after `_;`.
modifier costs(uint _amount) {
if (msg.value < _amount)
throw;
require(msg.value >= _amount);
_;
if (msg.value > _amount)
msg.sender.send(msg.value - _amount);
Expand Down Expand Up @@ -276,7 +269,7 @@ function finishes.

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

contract StateMachine {
enum Stages {
Expand All @@ -293,7 +286,7 @@ function finishes.
uint public creationTime = now;

modifier atStage(Stages _stage) {
if (stage != _stage) throw;
require(stage == _stage);
_;
}

Expand Down
19 changes: 8 additions & 11 deletions docs/contracts.rst
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ inheritable properties of contracts and may be overridden by derived contracts.

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

contract owned {
function owned() { owner = msg.sender; }
Expand All @@ -341,8 +341,7 @@ inheritable properties of contracts and may be overridden by derived contracts.
// function is executed and otherwise, an exception is
// thrown.
modifier onlyOwner {
if (msg.sender != owner)
throw;
require(msg.sender == owner);
_;
}
}
Expand Down Expand Up @@ -390,7 +389,7 @@ inheritable properties of contracts and may be overridden by derived contracts.
contract Mutex {
bool locked;
modifier noReentrancy() {
if (locked) throw;
require(!locked);
locked = true;
_;
locked = false;
Expand All @@ -401,7 +400,7 @@ inheritable properties of contracts and may be overridden by derived contracts.
/// The `return 7` statement assigns 7 to the return value but still
/// executes the statement `locked = false` in the modifier.
function f() noReentrancy returns (uint) {
if (!msg.sender.call()) throw;
require(msg.sender.call());
return 7;
}
}
Expand Down Expand Up @@ -989,7 +988,7 @@ more advanced example to implement a set).

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

library Set {
// We define a new struct datatype that will be used to
Expand Down Expand Up @@ -1035,8 +1034,7 @@ more advanced example to implement a set).
// The library functions can be called without a
// specific instance of the library, since the
// "instance" will be the current contract.
if (!Set.insert(knownValues, value))
throw;
require(Set.insert(knownValues, value));
}
// In this contract, we can also directly access knownValues.flags, if we want.
}
Expand Down Expand Up @@ -1166,7 +1164,7 @@ available without having to add further code.
Let us rewrite the set example from the
:ref:`libraries` in this way::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

// This is the same code as before, just without comments
library Set {
Expand Down Expand Up @@ -1207,8 +1205,7 @@ Let us rewrite the set example from the
// corresponding member functions.
// The following function call is identical to
// Set.insert(knownValues, value)
if (!knownValues.insert(value))
throw;
require(knownValues.insert(value));
}
}

Expand Down
3 changes: 1 addition & 2 deletions docs/frequently-asked-questions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -665,8 +665,7 @@ What does the following strange check do in the Custom Token contract?

::

if (balanceOf[_to] + _value < balanceOf[_to])
throw;
require((balanceOf[_to] + _value) >= balanceOf[_to]);

Integers in Solidity (and most other machine-related programming languages) are restricted to a certain range.
For ``uint256``, this is ``0`` up to ``2**256 - 1``. If the result of some operation on those numbers
Expand Down
29 changes: 15 additions & 14 deletions docs/security-considerations.rst
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ outlined further below:

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

contract Fund {
/// Mapping of ether shares of the contract.
Expand All @@ -88,8 +88,7 @@ outlined further below:
function withdraw() {
var share = shares[msg.sender];
shares[msg.sender] = 0;
if (!msg.sender.send(share))
throw;
msg.sender.transfer(share);
}
}

Expand Down Expand Up @@ -124,22 +123,24 @@ Sending and Receiving Ether
(for example in the "details" section in Remix).

- There is a way to forward more gas to the receiving contract using
``addr.call.value(x)()``. This is essentially the same as ``addr.send(x)``,
``addr.call.value(x)()``. This is essentially the same as ``addr.transfer(x)``,
only that it forwards all remaining gas and opens up the ability for the
recipient to perform more expensive actions. This might include calling back
recipient to perform more expensive actions (and it only returns a failure code
and does not automatically propagate the error). This might include calling back
into the sending contract or other state changes you might not have thought of.
So it allows for great flexibility for honest users but also for malicious actors.

- If you want to send Ether using ``address.send``, there are certain details to be aware of:
- If you want to send Ether using ``address.transfer``, there are certain details to be aware of:

1. If the recipient is a contract, it causes its fallback function to be executed which can, in turn, call back the sending contract.
2. Sending Ether can fail due to the call depth going above 1024. Since the caller is in total control of the call
depth, they can force the transfer to fail; make sure to always check the return value of ``send``. Better yet,
depth, they can force the transfer to fail; take this possibility into account or use ``send`` and make sure to always check its return value. Better yet,
write your contract using a pattern where the recipient can withdraw Ether instead.
3. Sending Ether can also fail because the execution of the recipient contract
requires more than the allotted amount of gas (explicitly by using ``throw`` or
requires more than the allotted amount of gas (explicitly by using ``require``,
``assert``, ``revert``, ``throw`` or
because the operation is just too expensive) - it "runs out of gas" (OOG).
If the return value of ``send`` is checked, this might provide a
If you use ``transfer`` or ``send`` with a return value check, this might provide a
means for the recipient to block progress in the sending contract. Again, the best practice here is to use
a :ref:`"withdraw" pattern instead of a "send" pattern <withdrawal_pattern>`.

Expand All @@ -162,7 +163,7 @@ Never use tx.origin for authorization. Let's say you have a wallet contract like

::

pragma solidity ^0.4.0;
pragma solidity ^0.4.11;

// THIS CONTRACT CONTAINS A BUG - DO NOT USE
contract TxUserWallet {
Expand All @@ -172,9 +173,9 @@ Never use tx.origin for authorization. Let's say you have a wallet contract like
owner = msg.sender;
}

function transfer(address dest, uint amount) {
if (tx.origin != owner) { throw; }
if (!dest.call.value(amount)()) throw;
function transferTo(address dest, uint amount) {
require(tx.origin == owner);
dest.transfer(amount);
}
}

Expand All @@ -192,7 +193,7 @@ Now someone tricks you into sending ether to the address of this attack wallet:
}

function() {
TxUserWallet(msg.sender).transfer(owner, msg.sender.balance);
TxUserWallet(msg.sender).transferTo(owner, msg.sender.balance);
}
}

Expand Down
Loading

0 comments on commit 4af0451

Please sign in to comment.