View profile

Contract Security: Peer Review and Static Code Analysis

Web3 Development
Web3 Development
I took some steps to ensure the security of an upcoming production contract by getting a peer review and doing static code analysis.

I missed a couple days on the newsletter last week. I went out of town for a wedding and didn’t keep it up. This week I’m going to play some catch-up.
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.
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);
}
Nat had encountered this with a staking contract he wrote previously and suggested using a global multiplier variable that could be updated with each compound and then be referenced when calculating a user’s balances from their deposit amounts.
Using his feedback, I rewrote the user balance tracking to use a vaultWeight variable and a mapping of userStartWeights to track what the weight was when they deposited their tokens. This way, I can compute their current balance using only the weights and their initial deposit, removing the for loop entirely.
// Balances
uint public vaultWeight; // Vault weight, increases as vault compounds rewards. Used for determining user vault allocations.
mapping(address => uint) public userStartWeights; // Vault weight when user deposited.
mapping(address => uint) public userDepositAmounts; // Amount of tokens that the user has deposited in the contract.
...
// In compound function
// Update vault weight based on new staking tokens added
if (stakingContractTokenBalance > 0) {
  vaultWeight = vaultWeight.mul(stakingContractTokenBalance.add(newStakingTokens)).div(stakingContractTokenBalance);
}
...
// 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
pip install slither solc-select
After installing Slither, you can run it from the command line from your python environment in the project directory:
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
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
Slither Contract Summary
  • –print modifiers: Prints a list of modifiers on each function to cross reference with the above.
Slither Function Modifiers Table
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!
Did you enjoy this issue? Yes No
Web3 Development
Web3 Development @oightytag

Learning Solidity and building on Web3.

In order to unsubscribe, click here.
If you were forwarded this newsletter and you like it, you can subscribe here.
Created with Revue by Twitter.