Skip to main content

Smart Contracts Review Guideline

Goals​

This document specifies some guidelines to develop smart contracts and essential considerations when we do security reviews.

The information described here applies not ONLY to strategies (our main point of risk); if not, the main goal is to help all yDoers that develop or review contracts, especially Solidity.

The main goals are:

  • Apply the best practices on developing smart contracts.
  • Document our experiences as learning lessons to mitigate risks and new attack vectors.
  • Reduce risks to get rekt on mainnet.
  • Discover potential issues, bugs, or exploits in the development or review phase.
  • Learn about the hacks or exploits that occurred in the DeFi ecosystem during the last months or years.

This document is based on:

  • The experience that the Yearn team has reached out to developing smart contracts and helping other protocols to mitigate risks.
  • Third sources (website, posts, and others). See more info in the Sources section.

Considerations​

Solidity Version​

Most smart contracts are built in Solidity (except the core contracts) and use the 0.6.12 version.

In new Solidity versions, some considerations are not required.

Formatting & Naming Conventions​

All of us are working remotely, and each of us has different ways to develop smart contracts. So, it is extremely important to get a standard formatting and naming convention for developing smart contracts.

Formatting​

The linting or formatting of the smart contract is a very important action to make them more readable, clear and easy to understand.

In our Brownie Strategy Mix repo we use Pretty Quick.

To lint the contracts, you just need to execute the command:

yarn lint

Before executing the CLI, you need to install the dependencies using yarn install.

Naming Conventions​

Internal or Private Functions​

To make the contracts more clear, a good practice is to add the _ prefix to all the internal or private functions. So, for example for this function

function myFunction() internal {
    ...
}

We should use:

function _myFunction() internal {
    ...
}

Use Constants​

Avoid using magic numbers or values. Instead, use constants with a meaningful name (in uppercase).

Instead of

function myFunction() public {
    ...
    uint256 value = aValue.mul(1234);
    ...
}

use


// Add a sentence describing what this constant means.
uint256 public constant VALUE_MULTIPLIER = 1234;
...
...
function myFunction() public {
    ...
    uint256 value = aValue.mul(VALUE_MULTIPLIER);
    ...
}

Best Practices​

Reduce Bytecode Size​

Use libraries​

One option is creating libraries because it encapsulates logic (move logic from the contract itself to the library) and reduces the strategy bytecode size.

References:

Use Interfaces​

If the contract interacts with multiple protocols, you should use interfaces to interact with them instead of using the contract itself.

Assert Returns values​

We must assert the returned values in any external call to check that the call was successful.

uint256 value = externalContract.externalFunction(param1, ...paramN);
require(value > 0, "invalid external value");

Variables - Naming Convention​

To make the code more clear and readable, we need to identify the unit in the variables that express amounts.

Example:

  • amountInWant, balanceInYShares, etc.

Multiple Routers​

In case we need to use multiple routers to swap tokens, we should use this approach:

  • Create constants for the router addresses.
  • Create a variable boolean useUniswap or similar.
  • Create a external/public function to switch the useUniswap value.
  • To get the router, use:
    • Router router = useUniswap ? uniswapRouter : sushiswapRouter;

See an example here.

Github Issues​

Use a new branch for each new strategy. Some old comments have references to lines of codes in branches that were already modified. It is more complicated to follow the comment when it happens.

Known Issues​

  • If the strategy uses another vault, we should add the maxLoss setting. See comment here.
  • Strategy.liquidatePosition function: This condition must be true. amountNeeded == liquidatedAmount + loss
  • Must have the health check address set.
  • Must use the SafeMath library to do calculations in the Solidity version 0.6.x.
    • The over/underflow issues are fixed in the Solidity version 0.8.x.
    • The BaseStrategy already imports and declares the SafeMath and SafeERC20 libraries, so you don't need to use the using SafeMath for uint statement. The same with SafeERC20.

Tools​

Required Checklists​

Each strategy must pass two checklist verifications, a general checklist and another one depending on its type. Both checklists are required to deploy the strategy on any mainnet network.

General​

These posts describe the most important considerations on security reviews.

Sources​

Last updated on