Assigment: Add to the ConsenSys Best Practices repository

Description

Our documentation should be updated to remain current. We are looking to work with a single contributor who can:

1. Write in a similar style as the existing content
2. Has a strong understanding of solidity and the EVM
3. Can ask well formulated questions, and perform their own research in order to flesh out the very brief descriptions in issues listed below
4. Has reviewed the existing content, understands the layout, and can make good judgements about where to add new content.

The following issues describe the work to be done.

• [x] #169
• [x] #170
• [x] #83 (the most recent comments need to be addressed)
• [x] #55
• [x] #133

About the ConsenSys Smart Contract Best Practices

Our best practices are the most popular security reference for smart contract developers. We're looking for help maintaining and expanding them.

Tasks

Create documentation for 3 unaddressed issues:

• [ ] #182
• [ ] #171
• [ ] #129

Important

We are looking to work with a single contributor who can:

1. Write in a similar style as the existing content
2. Has a strong understanding of solidity and the EVM
3. Can ask well formulated questions, and perform their own research in order to flesh out the very brief descriptions in issues listed below
4. Has reviewed the existing content, understands the layout, and can make good judgements about where to add new content.

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

Added a small write-up of how extcodesize should not be used for verifying externally owned accounts, which unfortunately is fairly common and easily exploitable [1].

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)

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.

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

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;
}

I am a Blockchain developer in VietNam. I see your document is very good. I decided to translate overall your document from English to VietNamese. Thank you !

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.

In this section, we have an example circuit breaker contract that performs a rate limiting technique. However, in my opinion, the current example is more complex than necessary, and somewhat unclear.

Here I've simplified the current example.

    uint internal period; // how many blocks before limit resets
    uint internal limit; // max ether to withdraw per period
    uint internal currentPeriodEnd; // block which the current period ends at
    uint internal currentPeriodAmount; // amount already withdrawn this period
    
    constructor(uint _period, uint _limit) public {
        period = _period;
        limit = _limit;
        
        currentPeriodEnd = block.number + period;
    }
     
    function withdraw(uint amount) public {
        // Update period before proceeding
        updatePeriod();
        
        // Disallow withdraws that exceed current rate limit
        require(currentPeriodAmount + amount < limit, 'exceeds period limit');
        currentPeriodAmount += amount;
        msg.sender.transfer(amount);
    }
    
    function updatePeriod() internal {
        if(currentPeriodEnd < block.number) {
            currentPeriodEnd = block.number + period;
            currentPeriodAmount = 0;
        }
    }

Now that "Solidity automatically reverts on integer overflow and underflow, as of version 0.8.0." shall we delete the Insecure Arithmetic page?

Reading the docs I found out that some of the state variables are left at the defaults, since this a developer guide with good securities measures shouldn't them be explicitly declared to public / private ?

Event Monitoring suggests using transfer() but External Calls warns against it.

Patterns like require(msg.sender.send(1 ether)) can also be simplified to using transfer(), as in msg.sender.transfer(1 ether).

Don't use transfer() or send().