Best Practices

smart contract best practices

External Calls

• Use caution when making external calls

• Mark untrusted contracts

• Avoid state changes after external calls

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

• Handle errors in external calls

• Favor pull over push for external calls

• Don't delegatecall to untrusted code

Force-feeding Ether

Beware of coding an invariant that strictly checks the balance of a contract.

An attacker can forcibly send ether to any account using selfdestruct and this cannot be prevented (not even with a fallback function that does a revert()).

Also, since contract addresses can be precomputed, ether can be sent to an address before the contract is deployed.

Speed Bumps

Speed bumps slow down actions, so that if malicious actions occur, there is time to recover.

struct RequestedWithdrawal {
    uint amount;
    uint time;
}

mapping (address => uint) private balances;
mapping (address => RequestedWithdrawal) private requestedWithdrawals;
uint constant withdrawalWaitPeriod = 28 days; // 4 weeks

function requestWithdrawal() public {
    if (balances[msg.sender] > 0) {
        uint amountToWithdraw = balances[msg.sender];
        balances[msg.sender] = 0; // for simplicity, we withdraw everything;
        // presumably, the deposit function prevents new deposits when withdrawals are in progress

        requestedWithdrawals[msg.sender] = RequestedWithdrawal({
            amount: amountToWithdraw,
            time: block.timestamp
        });
    }
}

function withdraw() public {
    if(requestedWithdrawals[msg.sender].amount > 0 && block.timestamp > requestedWithdrawals[msg.sender].time + withdrawalWaitPeriod) {
        uint amountToWithdraw = requestedWithdrawals[msg.sender].amount;
        requestedWithdrawals[msg.sender].amount = 0;

        require(msg.sender.send(amountToWithdraw));
    }
}

Rate Limiting

Rate limiting halts or requires approval for substantial changes. E.g. limit deposit withdrawals, tokens issued by contract.

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();

    // Prevent overflow
    uint totalAmount = currentPeriodAmount + amount;
    require(totalAmount >= currentPeriodAmount, 'overflow');

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

Deployment

Contracts should have a substantial and prolonged testing period - before substantial money is put at risk. Ideally one should have:

• A full test suite with 100% test coverage (or close to it)

• Deploy on your own testnet

• Deploy on the public testnet with substantial testing and bug bounties

• Exhaustive testing should allow various players to interact with the contract at volume

• Deploy on the mainnet in beta, with limits to the amount at risk

Automatic Deprecation

During testing, you can force an automatic deprecation by preventing any actions, after a certain time period. For example, an alpha contract may work for several weeks and then automatically shut down all actions, except for the final withdrawal.

modifier isActive() {
    require(block.number <= SOME_BLOCK_NUMBER);
    _;
}

function deposit() public isActive {
    // some code
}

function withdraw() public {
    // some code
}

Restrict amount of ether per user/contract

Reduces risk in case of insecure contracts

Assert, Require, Revert

Enforce invariants with assert()

An assert guard triggers when an assertion fails. Assert guards should often be combined with other techniques, such as pausing the contract and allowing upgrades, else you may end up stuck, with an assertion that is always failing.

**Use assert(), require(), revert() properly

The assert function should only be used to test for internal errors, and to check invariants.

The **require** function should be used to ensure valid conditions, such as inputs, or contract state variables are met, or to validate return values from calls to external contracts.

Following this paradigm allows formal analysis tools to verify that the invalid opcode can never be reached: meaning no invariants in the code are violated and that the code is formally verified.

pragma solidity ^0.5.0;

contract Sharer {
    function sendHalf(address payable addr) public payable returns (uint balance) {
				// require() can have an optional message string
        require(msg.value % 2 == 0, "Even value required."); 
        uint balanceBeforeTransfer = address(this).balance;

        (bool success, ) = addr.call.value(msg.value / 2)("");
        require(success);

        // Since we reverted if the transfer failed, there should be
        // no way for us to still have half of the money.
        assert(address(this).balance == balanceBeforeTransfer - msg.value / 2); // used for internal error checking
        return address(this).balance;
    }
}

Modifiers as Guards

The code inside a modifier is usually executed before the function body, so any state changes or external calls will violate the Checks-Effects-Interactions pattern. Moreover, these statements may also remain unnoticed by the developer, as the code for modifier may be far from the function declaration. For example, an external call in modifier can lead to the reentrancy attack:

contract Registry {
    address owner;

    function isVoter(address _addr) external returns(bool) {
        // Code
    }
}

contract Election {
    Registry registry;

    modifier isEligible(address _addr) {
        require(registry.isVoter(_addr));
        _;
    }

    function vote() isEligible(msg.sender) public {
        // Code
    }

In this case, the Registry contract can make a reentrancy attack by calling Election.vote() inside isVoter().

Integer Division

All integer division rounds down to the nearest integer. If you need more precision, consider using a multiplier, or store both the numerator and denominator.

// bad
// Result is 2, all integer division rounds DOWN to the nearest integer
uint x = 5 / 2; 

// good 
// using a multiplier prevents rounding down, this multiplier needs to be accounted for when working with x in the future
uint multiplier = 10;
uint x = (5 * multiplier) / 2;

// good 
// store the numerator and denominator to calculate the result off-chain
uint numerator = 5;
uint denominator = 2;

Locking Pragmas

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs.

Event Monitoring

Events can be used to monitor the contract's activity after it is deployed. Transaction history itself may be insufficient, as message calls between contracts are not recorded in the blockchain.

Events also show input parameters, and can be used to trigger functions in the user interface. Events that were emitted stay in the blockchain along with the other contract data and are available for future audit.

Complex Inheritance

When a contract is deployed, the compiler will linearize the inheritance from right to left (after the keyword is the parents are listed from the most base-like to the most derived).

contract Final {
    uint public a;
    function Final(uint f) public {
        a = f;
    }
}

contract B is Final {
    int public fee;

    function B(uint f) Final(f) public {
    }
    function setFee() public {
        fee = 3;
    }
}

contract C is Final {
    int public fee;

    function C(uint f) Final(f) public {
    }
    function setFee() public {
        fee = 5;
    }
}

contract A is B, C {
  function A() public B(3) C(5) {
      setFee();
  }
}

The consequence of the linearization will yield a fee value of 5, since C is the most derived contract.

EXTCODESIZE Checks

Avoid using extcodesize to check for Externally Owned Accounts.

The following modifier (or a similar check) is often used to verify whether a call was made from an externally owned account (EOA) or a contract account:

modifier isNotContract(address _a) {
  uint size;
  assembly {
    size := extcodesize(_a)
  }
    require(size == 0);
     _;
}

The idea is straightforward: if an address contains code, it's not an EOA but a contract account. However, a contract does not have source code available during construction. This means that while the constructor is running, it can make calls to other contracts, but extcodesize for its address returns zero. Below is a minimal example that shows how this check can be circumvented:

contract OnlyForEOA {    
    uint public flag;

    // bad
    modifier isNotContract(address _a){
        uint len;
        assembly { len := extcodesize(_a) }
        require(len == 0);
        _;
    }

    function setFlag(uint i) public isNotContract(msg.sender){
        flag = i;
    }
}

contract FakeEOA {
    constructor(address _a) public {
        OnlyForEOA c = OnlyForEOA(_a);
        c.setFlag(1);
    }
}

Because contract addresses can be pre-computed, this check could also fail if it checks an address which is empty at block n, but which has a contract deployed to it at some block greater than n.