Merge pull request #693 from chriseth/sec

Security Considerations

docs/contracts.rst

@@ -356,7 +356,7 @@ all).
 
 Furthermore, this function is executed whenever the contract receives plain
 Ether (without data).  In such a context, there is very little gas available to
-the function call, so it is important to make fallback functions as cheap as
+the function call (to be precise, 2300 gas), so it is important to make fallback functions as cheap as
 possible.
 
 ::

docs/control-structures.rst

@@ -69,6 +69,18 @@ this does not execute a constructor. We could also have used ``function setFeed(
 only (locally) sets the value and amount of gas sent with the function call and only the
 parentheses at the end perform the actual call.
 
+.. warning::
+    Any interaction with another contract imposes a certain danger, especially
+    if the source code of the contract is not known in advance. The current
+    contract hands over control to the called contract and that might do
+    just about anything. Be prepared that it calls into other contracts of
+    your system and perhaps even back into the calling contract before your
+    call returns. This means
+    that the called contract can change state variables of the calling contract
+    via its functions. Write your functions in a way that e.g. calls to
+    external functions happen after any changes to state variables in your contract,
+    so your contract is not vulnerable to a recursive call exploit.
+
 Named Calls and Anonymous Function Parameters
 ---------------------------------------------
 

docs/index.rst

@@ -92,6 +92,7 @@ Contents
    installing-solidity.rst
    solidity-by-example.rst
    solidity-in-depth.rst
+   security-considerations.rst
    style-guide.rst
    common-patterns.rst
    frequently-asked-questions.rst

docs/miscellaneous.rst

@@ -145,34 +145,6 @@ Tips and Tricks
 * If you do **not** want your contracts to receive ether when called via ``send``, you can add a throwing fallback function ``function() { throw; }``.
 * Initialise storage structs with a single assignment: ``x = MyStruct({a: 1, b: 2});``
 
-********
-Pitfalls
-********
-
-Unfortunately, there are some subtleties the compiler does not yet warn you about.
-
-- In ``for (var i = 0; i < arrayName.length; i++) { ... }``, the type of ``i`` will be ``uint8``, because this is the smallest type that is required to hold the value ``0``. If the array has more than 255 elements, the loop will not terminate.
-- If a contract receives Ether (without a function being called), the fallback function is executed. The contract can only rely
-  on the "gas stipend" (2300 gas) being available to it at that time. This stipend is not enough to access storage in any way.
-  To be sure that your contract can receive Ether in that way, check the gas requirements of the fallback function.
-- If you want to send ether using ``address.send``, 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 into 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, so make sure to always check the return value of ``send``. Better yet,
-     write your contract using a pattern where the recipient can withdraw Ether instead.
-  3. Sending Ether can also fail because the recipient runs out of gas (either explicitly by using ``throw`` or
-     because the operation is just too expensive). If the return value of ``send`` is checked, this might provide a
-     means for the recipient to block progress in the sending contract. Again, the best practise here is to use
-     a "withdraw" pattern instead of a "send" pattern.
-
-- Loops that do not have a fixed number of iterations, e.g. loops that depends on storage values, have to be used carefully:
-  Due to the block gas limit, transactions can only consume a certain amount of gas. Either explicitly or just due to
-  normal operation, the number of iterations in a loop can grow beyond the block gas limit, which can cause the complete
-  contract to be stalled at a certain point. This does not apply at full extent to ``constant`` functions that are only executed
-  to read data from the blockchain. Still, such functions may be called by other contracts as part of on-chain operations
-  and stall those. Please be explicit about such cases in the documentation of your contracts.
-
 **********
 Cheatsheet
 **********

docs/security-considerations.rst

@@ -0,0 +1,221 @@
+#######################
+Security Considerations
+#######################
+
+While it is usually quite easy to build software that works as expected,
+it is much harder to check that nobody can use it in a way that was **not** anticipated.
+
+In Solidity, this is even more important because you can use smart contracts
+to handle tokens or even more valuable things. Furthermore, every execution of a smart
+contract happens in public as it is mostly open source.
+
+Of course you always have to consider how much is at stake:
+You can compare a smart contract with a web service that is open to the
+public (and thus also to malicous actors) and perhaps even open source.
+If you only store your grocery list on that web service, you might not have
+to take too much care, but if you manage your bank account using that web service,
+you should be more careful.
+
+This section will list some pitfalls and general security recommendations but
+can of course never be complete. Also keep in mind that even if your
+smart contract code is bug-free, the compiler or the platform itself might
+have a bug.
+
+As always with open source documentation, please help us extend this section
+(especially, some examples would not hurt)!
+
+********
+Pitfalls
+********
+
+Private Information and Randomness
+==================================
+
+Everything you use in a smart contract is publicly visible, even
+local variables and state variables marked ``private``.
+
+Using random numbers in smart contracts is quite tricky if you do not want
+miners to be able to cheat.
+
+Re-Entrancy
+===========
+
+Any interaction from a contract (A) with another contract (B) and any transfer
+of Ether hands over control to that contract (B). This makes it possible for B
+to call back into A before this interaction is completed. To give an example,
+the following code contains a bug (it is just a snippet and not a
+complete contract):
+
+::
+
+  // THIS CONTRACT CONTAINS A BUG - DO NOT USE
+  contract Fund {
+    /// Mapping of ether shares of the contract.
+    mapping(address => uint) shares;
+    /// Withdraw your share.
+    function withdraw() {
+      if (msg.sender.send(shares[msg.sender]))
+        shares[msg.sender] = 0;
+    }
+  }
+
+The problem is not too serious here because of the limited gas as part
+of ``send``, but it still exposes a weakness: Ether transfer always
+includes code execution, so the recipient could be a contract that calls
+back into ``withdraw``. This would enable it to get a multiple refund and
+basically retrieve all the Ether in the contract.
+
+To avoid re-entrancy, you can use the Checks-Effects-Interactions pattern as
+outlined further below:
+
+::
+
+  contract Fund {
+    /// Mapping of ether shares of the contract.
+    mapping(address => uint) shares;
+    /// Withdraw your share.
+    function withdraw() {
+      var share = shares[msg.sender];
+      shares[msg.sender] = 0;
+      if (!msg.sender.send(share))
+        throw;
+    }
+  }
+
+
+Note that re-entrancy is not only an effect of Ether transfer but of any
+function call on another contract. Furthermore, you also have to take
+multi-contract situations into account. A called contract could modify the
+state of another contract you depend on.
+
+Gas Limit and Loops
+===================
+
+Loops that do not have a fixed number of iterations, e.g. loops that depends on storage values, have to be used carefully:
+Due to the block gas limit, transactions can only consume a certain amount of gas. Either explicitly or just due to
+normal operation, the number of iterations in a loop can grow beyond the block gas limit, which can cause the complete
+contract to be stalled at a certain point. This does not apply at full extent to ``constant`` functions that are only executed
+to read data from the blockchain. Still, such functions may be called by other contracts as part of on-chain operations
+and stall those. Please be explicit about such cases in the documentation of your contracts.
+
+Sending and Receiving Ether
+===========================
+
+- If a contract receives Ether (without a function being called), the fallback function is executed. The contract can only rely
+  on the "gas stipend" (2300 gas) being available to it at that time. This stipend is not enough to access storage in any way.
+  To be sure that your contract can receive Ether in that way, check the gas requirements of the fallback function
+  (for example in the "details" section in browser-solidity).
+
+- 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)``,
+  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
+  into the sending contract or other state changes you might not have though 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:
+
+  1. If the recipient is a contract, it causes its fallback function to be executed which can in turn call back into 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, so make sure to always check the return value of ``send``. 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
+     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
+     means for the recipient to block progress in the sending contract. Again, the best practise here is to use
+     a "withdraw" pattern instead of a "send" pattern.
+
+Callstack Depth
+===============
+
+External function calls can fail any time because they exceed the maximum
+call stack of 1024. In such situations, Solidity throws an exception.
+Malicious actors might be able to force the call stack to a high value
+before they interact with your contract.
+
+Note that ``.send()`` does **not** throw an exception if the call stack is
+depleted but rather returns ``false`` in that case. The low-level functions
+``.call()``, ``.callcode()`` and ``.delegatecall()`` behave in the same way.
+
+Minor Details
+=============
+
+- In ``for (var i = 0; i < arrayName.length; i++) { ... }``, the type of ``i`` will be ``uint8``, because this is the smallest type that is required to hold the value ``0``. If the array has more than 255 elements, the loop will not terminate.
+- The ``constant`` keyword for functions is currently not enforced by the compiler.
+  Furthermore, it is not enforced by the EVM, so a contract function that "claims"
+  to be constant might still cause changes to the state.
+- Types that do not occupy the full 32 bytes might contain "dirty higher order bits".
+  This is especially important if you access ``msg.data`` - it poses a malleability risk.
+
+***************
+Recommendations
+***************
+
+Restrict the Amount of Ether
+============================
+
+Restrict the amount of Ether (or other tokens) that can be stored in a smart
+contract. If your source code, the compiler or the platform has a bug, these
+funds might be gone. If you want to limit your loss, limit the amount of Ether.
+
+Keep it Small and Modular
+=========================
+
+Keep your contracts small and easily understandable. Single out unrelated
+functionality in other contracts or into libraries. General recommendations
+about source code quality of course apply: Limit the amount of local variables,
+the length of functions and so on. Document your functions so that others
+can see what your intention was and whether it is different than what the code does.
+
+Use the Checks-Effects-Interactions pattern
+===========================================
+
+Most functions will first perform some checks (who called the function,
+are the arguments in range, did they send enough Ether, does the person
+have tokens, ...). These checks should be done first.
+
+As the second step, if all checks passed, effects to the state variables
+of the current contract should be made. Interaction with other contracts
+should be the very last step in any function.
+
+Early contracts delayed some effects and waited for external function
+calls to return in a non-error state. This is often a serious mistake,
+because of the re-entrancy problem explained above.
+
+Note that also calls to known contracts might in turn cause calls to
+unknown contracts, so it is probably better to just always apply this pattern. 
+
+Include a Failsafe-Mode
+=======================
+
+While making your system fully decentralised will remove any intermediary,
+it might be a good idea, especially for new code, to include some kind
+of fail-safe-mechanism:
+
+You can add a function in your smart contract that performs some
+self-checks like "Has any Ether leaked?",
+"Is the sum of the tokens equal to the balance of the contract?" or similar things.
+Keep in mind that you cannot use too much gas for that, so help through off-chain
+computations might be needed there.
+
+If the self-check fails, the contract automatically switches into some kind
+of "failsafe" mode, which e.g. disables most of the features, hands over
+control to a fixed and trusted third party or just converts the contract into
+a simple "give me back my money"-contract.
+
+
+*******************
+Formal Verification
+*******************
+
+Using formal verification, it is possible to perform an automated mathematical
+proof that your source code fulfills a certain formal specification.
+The specification is still formal (just as the source code), but usually much
+simpler. There is a prototype in Solidity that performs formal verification and
+it will be better documented soon.
+
+Note that formal verification itself can only help you understand the
+difference between what you did (the specification) and how you did it
+(the actual implementation). You still need to check whether the specification
+is what you wanted and that you did not miss any unintended effects of it.

docs/solidity-by-example.rst

@@ -291,15 +291,32 @@ activate themselves.
         /// End the auction and send the highest bid
         /// to the beneficiary.
         function auctionEnd() {
+            // It is a good guideline to structure functions that interact
+            // with other contracts (i.e. they call functions or send Ether)
+            // into three phases:
+            // 1. checking conditions
+            // 2. performing actions (potentially changing conditions)
+            // 3. interacting with other contracts
+            // If these phases are mixed up, the other contract could call
+            // back into the current contract and modify the state or cause
+            // effects (ether payout) to be perfromed multiple times.
+            // If functions called internally include interaction with external
+            // contracts, they also have to be considered interaction with
+            // external contracts.
+
+            // 1. Conditions
             if (now <= auctionStart + biddingTime)
                 throw; // auction did not yet end
             if (ended)
                 throw; // this function has already been called
+
+            // 2. Effects
+            ended = true;
             AuctionEnded(highestBidder, highestBid);
 
+            // 3. Interaction
             if (!beneficiary.send(highestBid))
                 throw;
-            ended = true;
         }
 
         function () {
@@ -476,7 +493,8 @@ high or low invalid bids.
             var amount = pendingReturns[msg.sender];
             // It is important to set this to zero because the recipient
             // can call this function again as part of the receiving call
-            // before `send` returns.
+            // before `send` returns (see the remark above about
+            // conditions -> effects -> interaction).
             pendingReturns[msg.sender] = 0;
             if (!msg.sender.send(amount))
                 throw; // If anything fails, this will revert the changes above
@@ -490,11 +508,11 @@ high or low invalid bids.
             if (ended)
                 throw;
             AuctionEnded(highestBidder, highestBid);
+            ended = true;
             // We send all the money we have, because some
             // of the refunds might have failed.
             if (!beneficiary.send(this.balance))
                 throw;
-            ended = true;
         }
 
         function () {

docs/types.rst

@@ -113,6 +113,13 @@ All three functions ``call``, ``delegatecall`` and ``callcode`` are very low-lev
     All contracts inherit the members of address, so it is possible to query the balance of the
     current contract using ``this.balance``.
 
+.. warning::
+    All these functions are low-level functions and should be used with care.
+    Specifically, any unknown contract might be malicious and if you call it, you
+    hand over control to that contract which could in turn call back into
+    your contract, so be prepared for changes to your state variables
+    when the call returns.
+
 .. index:: byte array, bytes32