Engineering Blog #2 - Securing Smart Contracts with Slither

ENGINEERING BLOG #2 - SECURING SMART CONTRACTS WITH SLITHER

Making sure software functions correctly is a hard problem. When user money is involved, we need to go above and beyond normal practices.

To follow along with the examples in this post, download the project folder by running git clone https://gitlab.com/kirk_hutchison/solidity-security-examples. After cloning the project, run npm run install to get the dependencies and tools you'll need. The most important of these are Truffle, which provides handy tools for interacting with smart contracts, and Ganache, which lets us easily run a fake test blockchain locally. After installing dependencies, run npm run ganache to start an instance of ganache running. Now you're ready to go! Ganache will occupy the tab you ran it in, so open a new terminal tab and continue from there.

Let's try to verify the correct behavior of the following simple smart contract.

pragma solidity ^0.6.0;

	contract SimpleBankVersion1 {
	  mapping(address => uint256) internal balances;

	  receive() external payable {
		balances[msg.sender] = balances[msg.sender] + msg.value;
	  }

	  function withdraw(uint256 amount) external {
		require(balances[msg.sender] >= amount);
		msg.sender.transfer(amount);
		balances[msg.sender] = balances[msg.sender] - amount;
	  }

	  function checkBalance() external view returns(uint256) {
		return balances[msg.sender];
	  }
	}
  

If you have any questions about syntax, please see the Solidity docs. We won't be discussing Solidity syntax except as it relates to security.

This is a simple bank contract that allows any account to deposit funds, check their balance, and withdraw deposited funds. It has a critical vulnerability. Can you find it?

Our first step is to run npm run compile and check for any compilation errors or compiler warnings. It should compile successfully in this case.

Most programmers reach first for unit tests to verify their code. Here are a few tests I've written in Javascript using the Truffle test framework, which is based on Mocha.

it('should allow a user to make a deposit', async function() {
	  let balanceBefore = await myBank.checkBalance({from: accounts[0]})
	  await myBank.sendTransaction({from: accounts[0], value: 1000})
	  let balanceAfter = await myBank.checkBalance({from: accounts[0]})
	  assert(balanceAfter > balanceBefore, `Error: balance after deposit
		should be greater than ${balanceBefore} but is ${balanceAfter}`)
	})

	it('should allow a user to make a withdrawal', async function() {
	  let balanceBefore = await myBank.checkBalance({from: accounts[0]})
	  await myBank.withdraw(balanceBefore, {from: accounts[0]})
	  let balanceAfter = await myBank.checkBalance({from: accounts[0]})
	  assert(balanceAfter == 0, `Error: balance after withdrawal
		should be zero but is ${balanceAfter}`)
	})

	it('must NOT allow a user to withdraw more funds than they have deposited',
	async function() {
	  let balanceBefore = await myBank.checkBalance({from: accounts[0]})
	  try {
		await myBank.withdraw((balanceBefore + 100), {from: accounts[0]})
	  } catch (error) {
		assert(error, "Expected an error but did not get one")
	  }
	  await myBank.withdraw(balanceBefore, {from: accounts[0]})
	  let balanceAfter = await myBank.checkBalance({from: accounts[0]})
	  assert(balanceAfter == 0, `Error: balance after withdrawal
		should be zero but is ${balanceAfter}`)
	})

If you're following along, you can run these tests with npm run test1. These all pass, so our code looks good, right? Not so fast. Unit tests are good for making sure code can do what you want it to, but they're not very useful for making sure it CAN'T do anything unexpected. After all, the tests can only check for things that you think about and anticipate!

There is a better tool for discovering unexpected bugs called a static analyzer. Static analyzers check code for known vulnerabilities without executing the program. Static analysis is much faster and less computationally expensive than other tools because it doesn't have to actually run the code. However, this means static analyzers can only detect known vulnerable patterns. Complicated contracts and data structures may be beyond their ability. Don't be lulled into a sense of false security by growing dependent on tools.

Performing static analysis should be considered a necessary but not sufficient step in securing your smart contracts.

The static analysis tool we'll be using is called Slither, an open source tool released by security firm Trail of Bits. Slither can be easily installed using pip, the python package manager. Be aware that you need version 3 of both python and pip installed on your computer. The exact command may vary for your system, but will look something like pip3 install slither-analyzer. When you run this install command you may see a warning like the following:

WARNING: The script crytic-compile is installed in '/home/kirk/.local/bin' which is not on PATH. Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location. WARNING: The scripts slither, slither-check-erc, slither-check-kspec, slither-check-upgradeability, slither-find-paths, slither-flat, slither-format, slither-prop and slither-simil are installed in '/home/kirk/.local/bin' which is not on PATH. Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.

This means you need to add the line export PATH=/home/kirk/.local/bin:$PATH to your .bashrc or .bash_profile file in your home directory. Use whatever path was printed in the error message, and run source .bashrc to refresh. Then you should be able to run slither from any directory.

Slither checks for known bugs and vulnerable code patterns. It's possible to configure which types of error it checks for, but for now we'll just check everything. From your main project directory, run slither contracts/SimpleBankVersion1.sol to perform an analysis of our SimpleBank contract.

Note that Truffle projects always include a Migrations.sol contract which helps in deploying your other contracts to the test blockchain. Don't worry about any security warnings on the migrations contract, because it is only used for testing, not deployed to production.

Let's consider the results one by one.

Different versions of Solidity is used in :
		- Version used: ['>=0.4.21<0.7.0', '^0.6.0']
		- ^0.6.0 (SimpleBankStep1.sol#1)
		- >=0.4.21<0.7.0 (Migrations.sol#2)
    
	Reference: 

This is good to keep in mind. It's unwise to use different Solidity versions between contracts because it introduces needless complexity to the security analysis. We won't worry about this for the sake of this example.

Pragma version^0.6.0 (SimpleBankStep1.sol#1) necessitates versions too recent to be trusted.
 Consider deploying with 0.5.11
	Pragma version>=0.4.21<0.7.0 (Migrations.sol#2) allows old versions
	Reference: 

Production contracts should avoid using the latest versions of Solidity which may include uncaught bugs. There is a trade off between older versions (known bugs) vs newer versions (old bugs fixed, possible unknown bugs). With solidity currently on version 0.6.X, choosing 0.5.11 is a good choice to avoid any bugs introduced in the latest version. We don’t need to worry about this for the sake of our example.

Variable Migrations.last_completed_migration (Migrations.sol#6) is not in mixedCase
	Reference: 

We can ignore this because our migrations contract which is just a testing tool.

Reentrancy in SimpleBankStep1.withdraw(uint256) (SimpleBankStep1.sol#10-14):
		External calls:
		- msg.sender.transfer(balances[msg.sender]) (SimpleBankStep1.sol#12)
		State variables written after the call(s):
		- balances[msg.sender] = 0 (SimpleBankStep1.sol#13)
	Reference: 

Uh oh! We've found the big one. Reentrancy is the classic smart contract bug behind notorious hacks such as the DAO, which ended up causing an Ethereum hard fork. This is not old news, the recent Uniswap hack was also caused by the same kind of bug.

To understand how the reentrancy bug works in depth, read more here.

To summarize, reentrancy can occur when you make a call to an external contract or address. When this happens, you transfer the control flow of your code to that external party. This gives them the opportunity to perform nefarious actions. For example, by calling withdraw() numerous times before their balance is updated, the attacker can drain the entire balance of the smart contract, not just the balance remaining in their own account.

This is exactly the kind of thing that is our job to detect and avoid as smart contract developers.

Let's fix it! To do so, we need to remove any state changes that happen after an external call.

As a reminder, here's our old withdraw function with the offending lines highlighted:

function withdraw(uint256 amount) external {
		require(balances[msg.sender] >= amount);
		msg.sender.transfer(amount);
		balances[msg.sender] = balances[msg.sender] - amount;
	  }
    

All we need to do to fix this is switch the order of these lines.

function withdraw(uint256 amount) external {
		require(balances[msg.sender] >= amount);
		balances[msg.sender] = balances[msg.sender] - amount;
		msg.sender.transfer(amount);
	  }

By putting the external call at the end of the function, we prevent any changes from being made after the transfer of control.

Note that in Solidity, the address.transfer() function will throw a revert on failure, causing the entire transaction to be invalidated. Thus, if anything goes wrong with the transfer, no changes will be made to the program's state.

If we were making an external call to a smart contract's function, we'd want to wrap it in a require() statement to ensure that if the call fails, any state changes made will be reverted. address.transfer() has this built in so we don't have to worry about it in our code.

Let's run slither again and see if our change fixed the problem. You can try changing the contract code directly, I've created a copy of the contract with modified code. Rerun slither by calling slither contracts/SimpleBankVersion2.sol. Looks good! Thanks to slither, we've found and fixed a vulnerability that could have resulted in a total loss of user funds to an attacker. All without actually executing or deploying our code to the blockchain.

See how much more powerful a tool like Slither can be than handwritten unit tests? I hope this article demonstrates the merit of static analysis tools and why they should be more widely used in software generally and the blockchain industry in particular.

At Tacen, we've incorporated Slither into our Gitlab CI/CD pipeline so that any change to our smart contracts is checked for known vulnerabilities before it can be approved.

Between unit tests and Slither, we can guarantee that our contracts:

1. Are capable of performing their expected behaviors.
2. Are free of vulnerabilities that are known in the industry.

This leaves out any kind of novel error that can't be detected by Slither. In my next post, we'll go over some approaches to finding the "unknown unknowns".

‍