Jordan Hall
Jordan Hall | oighty.eth

Jordan Hall | oighty.eth

Contract Security: Peer Review and Static Code Analysis

Contract Security: Peer Review and Static Code Analysis

Jordan Hall's photo
Jordan Hall
·Oct 18, 2021·

5 min read

Subscribe to my newsletter and never miss my upcoming articles

My main focus right now is on finishing and launching my Crypto Raiders auto-compounding vault project. I've spent a bit more time on it than I anticipated, but I want to make sure there are no hiccups before I put it out in the wild. To that end, I want to make sure the Raider LP Vault contract is sound from a security standpoint.

Given that this will be a relatively small project, I am not planning to pay for a full contract audit by one of the large firms. That being said, I do want to put the contract through its paces before putting anyone's funds at risk. The two other main options for a security review are:

  • Peer Review
  • Static Code Analysis

After going through this process, I realized I probably did them backwards this go around (Peer Review before Static Analysis), but both were helpful nonetheless. (Edit: After publishing this article, I've learned more about Fuzzing and Symbolic Execution, which are additional testing strategies to consider for ensuring contracts will behave the way you expect).

Peer Review

I recently joined the Solidity Guild, a group of web3 and Solidity developers partnering together to take on projects. Being in a group of peers who are focused on the same things as me has been awesome. I have enjoyed learning from others. Additionally, I had a couple people volunteer to review my Raider Vault contract last week (thanks Nat Eliason and Zakk!). Very few people are an expert in all parts of the Solidity language and project types. Because everyone has a different level of experience and background, they are likely to see something you didn't.

In my case, Nat recognized an issue that I knew was a problem, but didn't have a good solution for yet. I had a for loop that iterated over a mapping of users to their current balances in order to update them after the vault compounds. The issue with this is that a user list can grow indefinitely. This would have cause the loop to have more iterations and become more expensive over time. Eventually, it probably would exceed the transaction gas limit and prevent the contract from compounding anymore. Yikes, that would've been bad. Here is how the contract was managing user balances before:

// Balances
uint internal numberUsers; // Count the number of users to use with balance management
mapping(address => uint) public userBalances; // Balance of staking tokens for each vault user
mapping(uint => address) internal indexUser; // User's index in the user array for looping
...

// In compound function
// Update user balances with new LP tokens
for (uint index = 1; index <= numberUsers; index++) {
  user = indexUser[index];
  shareOfNewTokens = newStakingTokens.mul(userBalances[user]).div(stakingContractTokenBalance);
  userBalances[user] = userBalances[user].add(shareOfNewTokens);
}
...

// In deposit function
// Add user to user list and update the user's deposit amount and start weight
if(isUser(msg.sender)) {
     userStartWeights[msg.sender] = (vaultWeight.mul(userDepositAmounts[msg.sender].add(amount_))).div(getUserBalance(msg.sender).add(amount_));
} else {
    _addUser(msg.sender);
    userStartWeights[msg.sender] = vaultWeight;     
}
userDepositAmounts[msg.sender] = userDepositAmounts[msg.sender].add(amount_);

The changes to the deposit function to account for additional deposits at different weights was the most complicated piece. I had to leverage some math skills that I hadn't used in a long-time :).

Static Code Analysis with Slither

After making the updates suggested by the peer review, I wanted to make sure I wasn't missing anything small, especially that could be security-related. Zakk had posted in our guild discord a few tools for static code analysis, and I thought I'd give them a try.

Slither is a static code analysis tool written in Python that automatically runs a number of issue detectors on your contract code. It is developed and maintained by Crytic (formerly called Trail of Bits), a leading smart contract audit company.

Installing Slither was pretty easy. The only two requirements were to have a Solidity compiler installed and a Python 3 environment. I have done a bit of other python work and use pyenv to manage python environments. Pyenv makes it easy to spin up new virtualenvs and then install the libraries needed for this particular effort to not pollute my global environment.

$ pyenv local // checks the current version, change if needed
$ pyenv virtualenv solidity
$ pyenv activate solidity
(solidity)$ pip install slither solc-select

After installing Slither, you can run it from the command line from your python environment in the project directory:

(solidity)$ slither ~/project-directory

The above command runs slither on the current directory. This is helpful if you are using a development environment because it can recognize the directory structure and then use your built-in compile methods, e.g. in hardhat. If you don't do this (and run on a specific contract file), it may not handle contract imports from node_modules or other processing correctly.

There are a bunch of options to print different types of results. The basic one just prints out all of the issues it found in order of impact, but it can be hard to read.

A few other useful options are:

  • --print human-summary: Prints an overview of the results based on all the detectors run on the contract. Slither Human Summary
  • --triage-mode: Goes through each type of detected issue one by one (showing all occurrences each time). Helpful for checking the mitigation and reviewing one at a time vs. getting a big print out. You can view detailed information about each detector and recommendations to resolve in the Slither Detector Documentation.
  • --print contract-summary: Prints a list of all the contract functions and their declared function types highlighted in a specific order. One caveat here is that it doesn't take into account modifiers. E.g., In the picture below, a number of functions are flagged as external, such as setManager, but that function is also modified by onlyOwner so it's not callable by general users. Slither Contract Summary
  • --print modifiers: Prints a list of modifiers on each function to cross reference with the above. Slither Function Modifiers Table

After reviewing the detected errors in triage-mode and walking through the mitigations for the issues I found, they were either low impact/not significant or already addressed by a modifier/mitigation in place.

One critique that I've heard of Slither is that it will flag some false positives. This seemed to be the case with the reentrancy issues that were flagged as I've already modified those functions with the OpenZeppelin ReentrancyGuard nonReentrant modifier to prevent these issues.

Going through these additional checks on the contract gives me a lot more confidence as I move to release these in production. More to come!

 
Share this