Automated Smart Contract Audit Guide. Part 2: Slither

Analyzer: Slither
Description: Open-source static analysis framework for Solidity
githib: https://github.com/trailofbits/slither

This is a static code analyzer written in python. He is able to keep track of variables, calls, and detects such a list of vulnerabilities . Each vulnerability has a link with a description, and if you are new to Solidity, it makes sense for you to familiarize yourself with everyone.

Slither can work as a python module and provide the programmer with an interface for auditing according to its own plan. A simple and illustrative example of what slither can do can be seen here .

We will return to the analysis scenarios at the end of the article, but for now let's run Slither:

git clone https://github.com/trailofbits/slither.git cd slither docker build -t slither . 

and try to analyze our contract.

Go to the directory with constructor-eth-booking and try to run slither from the docker:

 $ docker run -v $(pwd)/contracts:/slither/contracts slither contracts/flattened.sol 

we get the error “Source file requires different compiler version”, and now we need to shove the version solc=0.4.20 into the docker slither. To do this, we rule the Slither Dockerfile itself, as indicated in Clause 2 in the introduction , i.e. somewhere at the end of the Dockerfile add the line:

 COPY --from=ethereum/solc:0.4.20 /usr/bin/solc /usr/bin 

, rebuild the image, run, hooray, everything compiles.

We see the output, warning messages about various “pragma” and incorrect names of variables like Parameter '_price' of Booking.Booking (flattened.sol#73) is not in mixedCase . Analyzers give a lot of warnings, but we are looking for real bugs, and we will not pay attention to trifles. Let's filter all messages about mixedCase, now it's not up to the style:

 $ docker run -v $(pwd)/contracts:/slither/contracts slither contracts/flattened.sol 2>&1 | fgrep -v 'mixedCase' 

True programmers skip everything green, look everything red, and here’s what besides false-positives found in the Slither contract:

 Booking.refundWithoutCancellationFee (flattened.sol#243-250) sends eth to arbirary user Dangerous calls: - client.transfer(address(this).balance) (flattened.sol#249) Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations INFO:Detectors: Booking.refundWithCancellationFee (flattened.sol#252-259) sends eth to arbirary user Dangerous calls: - owner.transfer(m_cancellationFee) (flattened.sol#257) - client.transfer(address(this).balance) (flattened.sol#258) Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations 

Now we are looking at what is wrong with this function in the contract:

  /************************** PRIVATE **********************/ function refundWithoutCancellationFee() private { address client = m_client; m_client = address(0); changeState(State.OFFER); client.transfer(address(this).balance); } function refundWithCancellationFee() private { address client = m_client; m_client = address(0); changeState(State.CANCELED); owner.transfer(m_cancellationFee); client.transfer(address(this).balance); } 

while, for example, the refundWithoutCancellationFee() function is called like this:

  function rejectPayment() external onlyOwner onlyState(State.PAID) { refundWithoutCancellationFee(); } function refund() external onlyClient onlyState(State.PAID) { refundWithoutCancellationFee(); } 

Hmm, there are no formal errors: the calls are protected by all sorts of onlyOwner , but Slither swears that, supposedly, a broadcast is sent inside refundWithoutCancellationFee () without any checks. And he is right, the function itself really has almost no limitations. Let it be private, and it is called from the “rejectPayment ()” “refund ()” wrappers with the necessary restrictions, but in this form, if you finalize the contract, there is a great risk of forgetting about the restrictions and sticking the call of refundWithoutCancellationFee() to some other place, available to the attacker. So, even if formally there is no vulnerability, the information turned out to be useful - this is at least the “warning” level, if the task is to develop the contract code further. In this case, two functions from different participants use the same code, and this decision was made to save gas - the contract is a one-time, and the cost of its calculation is an important factor.

I rechecked if it’s not just that Slither swears at any shipment, and transferred the body of the function directly to the above “rejectPayment ()” and “refund ()”, the warning disappeared, i.e. Slither realized that now the air is not sent without checking addresses. Great start!

Now let's check how Slither watches the initialization of variables, for this we comment out two initializations:

 - m_fileHash = _fileHash; + // m_fileHash = _fileHash; - m_price = _price; + // m_price = _price; 

the first is not very important, in terms of holes, except for wasting resources, because m_fileHash is not used anywhere, it is simply stored in the blockchain when creating a contract. But m_price is used, and Slither correctly swears that m_price is not initialized anywhere, although it is used:

 Booking.m_price (flattened.sol#128) is never initialized. It is used in: - fallback (flattened.sol#144-156) 

Well, this is a simple trick, as expected, everything worked fine.

Now we will add to the contract a reentrant so beloved by all: we will change the state of the contract after an external call. We make such changes:

  function refundWithoutCancellationFee() private { address client = m_client; - m_client = address(0); - changeState(State.OFFER); - client.transfer(address(this).balance); + client.call.value(address(this).balance)(); + m_client = address(0); + changeState(State.OFFER); } 

I had to replace transfer with a call, since The option with transfer Slither does not swear due to the fact that transfer sends a call with a minimum of gas, and the callback is not possible (although when switching to the Constantinople fork in Ethereum, the price of gas was changed, and this re-enabled the reentrancy attack using transfer .

Search result reentrancy:

 Reentrancy in Booking.refundWithoutCancellationFee (flattened.sol#243-253): External calls: - client.call.value(address(this).balance)() (flattened.sol#245) State variables written after the call(s): - m_client (flattened.sol#246) 

Fine, at least it will not allow to change state variables after external calls, and it is very good.

If you move through the list, the remaining vulnerabilities in the list either represent simply a search for specific methods in the code, or known patterns that, if you have access to the code markup performed by the python, work, of course, and reliably enough. Those. well-knows Slither patterns will not miss.

Now, I will make changes that perfectly show the specifics of the work of static analyzers:

 - client.transfer(address(this).balance + for (uint i=0; i < 1; i++) { + client.transfer(address(this).balance - 999999999999999999); + } 

and the result:

 Booking.refundWithoutCancellationFee has external calls inside a loop: - client.transfer(address(this).balance - 999999999999999999) (flattened.sol#252) Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description/_edit#calls-inside-a-loop 

The cycle is executed once, and is degenerate - therefore the issued warning is false positive, and the absence of a warning about dangerous arithmetic is false negative. Analysis of types, results of operations, counting calls - tasks are not for static analyzers. Therefore, clearly understand what mistakes Slither will find and which ones you need to look for using other tools.

We promised to mention the possibility of writing our own scripts for testing, and outputting any interesting information about the contract using the --print key. From this point of view, Slither is an excellent tool for CI. Developers of a large contract system know the names of security-critical variables: balances, commission sizes, flags, and can write a test script that will block any changes in the code that, for example, overwrite an important variable, or change state variables after an external call, and its perfectly predictable analysis is an excellent tool for use in hooks.
The task of Slither is to rid you of stupid bugs, find familiar dangerous patterns and warn the developer. In this variant, it is also good as a tool for a novice developer on Solidity, immediately prompting how to write the code on Solidity correctly.

Results

In my personal testing, I would put the Slither four, for versatility, simplicity and ease of use, as well as for simple and clear test scenarios and adaptability to CI.

Slither confidently found the real WARNING, associated with the use of the function that sends the air, found all the bugs introduced. He could not cope only with dynamic analysis, which formally should not be done, otherwise he would have to sacrifice universality, predictability and ease of use.

In the next article we will deal with the analyzer Mythril, but the whole table of contents of articles that are ready or planned to write:

Part 1. Introduction. Compilation, flattening, Solidity versions
Part 2. Slither (this article)
Part 3. Mythril (in the process of writing)
Part 4. Manticore (in the process of writing)
Part 5. Echidna (in the process of writing)