Here are a few suggestions to improve the structure and clarity of the document:

1. In Recommendations for Smart Contract Security in Solidity, Known Attacks, and Software Engineering, contributors should always include a source link and a code example. For Known Attacks, there should always be an example of how to mitigate in code.
2. In Recommendations for Smart Contract Security in Solidity, each title should be more instructive, and have symmetry (all should start with a verb or noun, rather than a mixture of both)

• Instead of Proper Handling of External Calls, use Avoid external calls, when possible
• Instead of Design Patterns to avoid external calls: Push vs Pull & Asynchrony, use Favor pull payments over push payments
• Instead of "Fallback functions", use Keep fallback functions simple
• (in this case, I've chosen verbs as the starting point)

1. Keep writing succinct; favor a few words if the comprehension is the same when compared to more words; verboseness without substantial info leads to worse comprehension, and likely impacts the security of the contracts subsequently written by readers

• key management: msg.sender which publishes contract is probably stored in plain text on your machine, probably that address shouldn't be used for permissioning in the deployed system.
• Other deployment suggestions

https://blog.coinfabrik.com/smart-contract-short-address-attack-mitigation-failure/

also mentioned in solidity docs

Would probably go under the solidity specific recommendations section.

https://pdaian.com/blog/solidity-anti-patterns-fun-with-inheritance-dag-abuse/

https://github.com/raineorshine/solgraph

"Something that might not be obvious: The payable modifier only applies to calls from external contracts. If I call a non payable function in the payable function in the same contract, the non-payable function won't fail, though msg.value is still set."

from @Georgi87. Thanks for this.

Not sure this is the right forum to propose this, but it might be nice to develop a truffle box at Consensys that:

• installed with a full suite of code quality, doc generation and security tools.
• came with working CI .ymls for Circle / Travis.
• included convenience scripts for everything it shipped, e.g: npm run lint.
• was seeded with contracts & tests that exemplify bad practice and produce failures detectable by whatever tools the ecosystem has. Might also include working examples of exploits people should defend against. This could be the basis of a tutorial in which you step through the code and fix each problem, eventually getting a clean build.
• included useful eth-specific testing libraries, like the .js helper methods at Zeppelin and provided examples of less well known testing strategies like property-based, generative testing.

The box could serve several constituencies equally well: for some it would be a convenient testing-oriented starter, for others a way of learning hands on about contract security.

For tools and testing-lib developers it could be a platform to publish work to a wider audience and anyone in the community might find the repo worth GitHub watching in order to keep abreast of new tools as they become available.

Instead of sending to some unowned address like 0xDEAD, use this burner:

https://etherscan.io/address/0xb69fba56b2e67e7dda61c8aa057886a8d1468575#code

Isn't a huge issue (basically calls to nonexistent code including nonexistent fallback functions currently return some data, which is cast to "true" when checking call success), but given the strong emphasis on handling send failures it's probably worth mentioning even though this is planned to be fixed.

This guide argues that pragma be locked to a specific version.

The Ethereum Solidity guide argues that pragmas should be allowed to move up in the patch version (e.g., ^0.4.20 rather than 0.4.20):

We do not fix the exact version of the compiler, so that bugfix releases are still possible.

We should probably debate this, and then make a recommendation. Are there examples in the past where either way caused an issue? If both have caused issues, may want to just highlight tradeoff. Anyone else we should consult on this?

(cc: @ethers)

Hi,

I don't understand : // At this point, the caller will be able to execute getFirstWithdrawalBonus again

According Solidity official documentation about internal function call

These function calls are translated into simple jumps inside the EVM

So I can't figure how a caller can take control of the flow here. Can you describe how ?

// INSECURE
mapping (address => uint) private userBalances;
mapping (address => bool) private claimedBonus;
mapping (address => uint) private rewardsForA;

function withdraw(address recipient) public {
    uint amountToWithdraw = userBalances[recipient];
    rewardsForA[recipient] = 0;
    require(recipient.call.value(amountToWithdraw)());
}

function getFirstWithdrawalBonus(address recipient) public {
    require(!claimedBonus[recipient]); // Each recipient should only be able to claim the bonus once

    rewardsForA[recipient] += 100;
    withdraw(recipient); // At this point, the caller will be able to execute getFirstWithdrawalBonus again.
    claimedBonus[recipient] = true;
}

We should support this proposal:
https://github.com/ethereum/EIPs/pull/679/commits

Attacks should have a definition, it's debatable to me whether a Leech attack is an attack in the traditional sense. It's unfortunate for the oracle and it's payer, but I don't know if it causes malicious and unexpected damage to a contract (a dev sec person can probably indicate what exactly an attack is defined as).

I've also noticed that certain items are called "bugs" (e.g., Timestamp dependence). We should discuss exactly what a "bug" is vs an "attack".

The attacks and recco's sections have lots of good content, but need some organization to facilitate navigation.

I think each page could be divided into at least two top level categories:

1. Protocol specific
2. Solidity specific

This would help differentiate between the fundamental security issues, and ones specific to solidity. It might also help to prepare use for a multi-evm language future.

• state channels
• doing complex calcs off-chain, verifying them on-chain
• splitting high congestion stuff into multiple tx with smaller gas cost (ie. 0x token sale approach)

Excellent article, thank you very much.

I read somewhere that it is didactically better to first mention the good example, and only then show the bad example - because then the probability is lower that human memory falsely stores the bad example as the way to do it.

just an idea.

How about providing PDF document?

I like reading printed document. but, This document site is not providing pdf document.
So, I request you that upload pdf version document.

( I'm already converted this document to pdf using with http://www.markdowntopdf.com/ )

https://www.reddit.com/r/ethereum/comments/5zkuwj/solidity_0410_released/

Included things like transfer() which is equivalent to if(!send()) { throw; }

See https://github.com/ConsenSys/smart-contract-best-practices/blob/master/README.md#favor-pull-payments-over-push-payments

As noted, payments can fail for multiple reasons - including if the call stack exceeds 1024 or a failure on externally.

This sentence seems incomplete.

Details documented in TIB (Today I Burnt) 0.01 ETH Using Constant State Variables In A Solidity Contract.

Using a constant state variables produces unexpected results, for example:

uint256 public constant PRESALE_START_DATE = now;
uint256 public constant PRESALE_END_DATE = PRESALE_START_DATE + 15 minutes;
uint256 public constant OWNER_CLAWBACK_DATE = PRESALE_START_DATE + 20 minutes;

The variables:

• PRESALE_START_DATE will ALWAYS have the current time value
• PRESALE_END_DATE will ALWAYS have a date 15 minutes in the future
• OWNER_CLAWBACK_DATE will ALWAYS have a date 20 minutes in the future

Sample contract at 0xe67907329dafd1ff826523e3f491bec8733f7376. Refresh the page and you will see these constant variables update. DO NOT SEND FUNDS TO THIS CONTRACT

In the code for refunding multiple addresses by iterating over a loop, shouldn't Line 5 in the code be Payee[] payees; instead of Payee payees[]?

struct Payee {
address addr;
uint256 value;
}
Payee payees[];
uint256 nextPayeeIndex;

In https://github.com/ConsenSys/smart-contract-best-practices#dos-with-unexpected-throw

In the bid function, how exactly can currentLeader.send(highestBid) fail?

It seems to be a bit overly prescriptive, especially given the size of the current reward, for certain use cases timestamps should be fairly safe.

Perhaps we can quantify what values, or granularities of time are safe.

Reference:
https://ethereum.stackexchange.com/questions/413/can-a-contract-safely-rely-on-block-timestamp/432#432

In the "Using Registry contract to store the latest version of the contract" part, the example code has a missing semicolon:

modifier onlyOwner() { if (msg.sender != owner) { throw; } _ }

there should be a semicolon at the back of _

And several following examples have the same issue.

ISSUE: There are many security consideration outside of programming and architecture, which are not covered here.

PR REQUEST: A new section should be created (including modifying the [yaml config(https://github.com/ConsenSys/smart-contract-best-practices/blob/master/mkdocs.yml)), which presents the various operational considerations necessary when deploying a contract system.

These include existing issues:

[] #92
[] #85
[] #79
[] #97

As well as:
[] proper cold storage for critical private keys
[] possible recommendations for M and N on a multisig wallet
[] other

This includes #57, as well as

Using parameter types to implement both in the same contract (overloading):
doSomething(uint256)
doSomething(bytes32)

Probably quite a few others.

Need to explain this clearly

https://consensys.github.io/smart-contract-best-practices/future_improvements/

If so it needs an update.

Should we add this?

Rule 18
In compound expressions with multiple sub-expressions the intended
order of evaluation shall be made explicit with parentheses. [cf. MISRAC:2004
Rule 12.2]

http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_C.pdf

(we could add the obvious caveat that "However, do not add too many parentheses so as to clutter the code and make it unreadable." per MISRAC:2004 Rule 12.1)

I think there are some novel items in here that we need to incorporate, but I haven't look at all of them yet.

https://medium.com/@chriseth/lessons-learnt-from-the-underhanded-solidity-contest-8388960e09b1
https://github.com/Arachnid/uscc/tree/master/submissions-2017/
https://medium.com/@weka/announcing-the-winners-of-the-first-underhanded-solidity-coding-contest-282563a87079

MkDocs doesn't support language toggling/branching, so Material cannot add this functionality either. To solve this, you would need to build two separate versions of your docs, one for each language configuring the correct search language in mkdocs.yml. By customizing the theme you could link to the other language inside the header or where you want, effectively switching between the two separate builds.

squidfunk/mkdocs-material#867 (comment)

When send is used, it will only give a gas stipend for non-zero value transfer (Yellow Paper Appendix H description of "CALL" (last line) ). If the recipient is a normal account (without code), it will work and return 1. If it is a smart contract with code to be executed, such as the multisig, it will fail with an out-of-gas exception and return 0. (since it needs gas to execute this line: https://github.com/ethereum/dapp-bin/blob/d9e8f0de17db153749920c01ea77e910a4a92b45/wallet/wallet.sol#L336)

An example for this failure can be found here: https://github.com/BitySA/whetcwithdraw/blob/086170f38eef1762726c4109a3ed713d79b87c26/whetcwithdraw.sol#L116 ( White Hat Hacker Withdraw Contract). When someone will donate nothing it throws an exception since it tries to call the standard multisig.

I've been reading a lot of information about Solidity security, and I think there's something missing in nearly everything I've read. Of course, everything mentioned here is great, but there is no little or no mention of best practices as they relate to 'logging' of a smart contract.

A well-designed logging design would help with (1) the on-going monitoring of a live contract, and (2) the post-mortum in the event of a failure such as The DAO. An example is the DAO Withdraw contract which has a single function 'withdraw' that calls DAO::transferFrom and then sends ether, neither of which are recorded as transactions on the blockchain (being internal transactions). Additionally, the function call is not logged because the contract has no events. I one wished to audit the 'withdraw' contract, it would be difficult because one would have to co-ordinate transactions/internal transactions/and event logs from two different contracts.

Not to confuse the distinction between active protective security measures made prior to the deployment of a contract and back-end, after the fact, auditing, but I still think auditing/logging should be part of the security discussion, if for no other reason than be able to better understand what may have gone wrong with a contract will help improve security in the future.

Suggestion: Include a section in this document concerning making a smart contract more easily monitored while it is alive, and auditable if anything goes wrong (or periodically, say for tax purposes or something).

Contract developers may assume a newly created contract has zero balance. For example,developers may write asserts on different parts of the code that check that the contract balance equals an internal field such as expectedBalance, which is updated on every incoming/outgoing payment.

In Ethereum contract addresses can be guessed: they are built using the source address and source nonce hashed. Therefore an attacker can predict the a contract address that is to be created and send before a tiny amount of ether the that address, pre-creating an account (without any code) with the same address. The attacker has no control over it, so the account nonce will be always zero. When the contract is actually created by the authentic source, the pre-existing balance will not be destroyed: the contract will be created with a pre-existent non-zero balance. Therefore a contract cannot assume the balance is zero upon creation.

This post from our friends at Trail of Bits made reference to some outdated content in our best practices.

It definitely needs to be revisited, and in the shorter term, possibly removed or wrapped in a big fat caveat.

Lots of breaking changes with built in safety features:

https://github.com/ethereum/solidity/releases/tag/v0.4.0

The example I'm thinking of is CoinDash.

Possible options include:

• using the ENS to demonstrate ownership
• publishing addresses well in advance.
• hosting the site on IPFS

The functional programming link in the readme https://plus.google.com/u/0/events/cmqejp6d43n5cqkdl3iu0582f4k is a 404 over here, and I imagine if you open the link in an incognito window (or while you're not logged into google), you too will also get a 404

Reminder for @simondlr “I’m going to submit the details later”

Seems obvious, but I don't think it's been said.

https://twitter.com/VladZamfir/status/1044094929754980352

(optimizing compiler could have more chance of bugs)

In "Keep fallback functions simple" section,

function deposit() external { balances[msg.sender] += msg.value; }

Does't this function need "payable" modifier to work properly?

You've mentioned in your wiki that to solve race condition attack, one can use a lock variable. I quote your code here. And I don't think a lock variable can solve parallel invoking. As you are setting lockBalances to true, I can still go into the withdraw function and thus withdraw the balance again.

function deposit() public returns (bool) {
    if (!lockBalances) {
        lockBalances = true;
        balances[msg.sender] += msg.value;
        lockBalances = false;
        return true;
    }
    throw;
}

function withdraw(uint amount) public returns (bool) {
    if (!lockBalances && amount > 0 && balances[msg.sender] >= amount) {
        lockBalances = true;

        if (msg.sender.call(amount)()) { // Normally insecure, but the mutex saves it
          balances[msg.sender] -= amount;
        }

        lockBalances = false;
        return true;
    }

    throw;
}

ie. The griefing/DOS attack in uport and gnosis where a 3rd party calls an approved transaction, with insufficent gas to execute.