Bug Report Slither Printer Vars-and-auth Duplicates MessageDiscussion Category

This article addresses a bug encountered in the Slither static analysis tool, specifically within the vars-and-auth printer. The issue results in duplicated messages under the "Conditions on msg.sender" section of the output, which can lead to confusion and misinterpretation of the analysis results. This comprehensive write-up details the problem, provides a code example to reproduce the bug, pinpoints the source of the issue within the Slither codebase, and proposes a solution. This article provides insights into how the issue can be fixed by removing a line of code that causes the duplication of conditions related to msg.sender checks. This issue is important as it affects the clarity and accuracy of Slither's output, potentially leading to confusion for developers relying on the tool for security analysis.

Issue Description

The vars-and-auth printer in Slither is designed to output information about state variables written by functions and the conditions imposed on msg.sender for function execution. However, a bug causes the printer to display duplicated conditions related to msg.sender. This duplication occurs when a function uses modifiers that include require statements, particularly those checking the sender's address. This article will cover in detail the specific scenario where the bug manifests, which involves contracts that use modifiers to enforce conditions on msg.sender. This will be shown through code examples and analysis of Slither's output.

For example, the output might look like this:

+----------+-------------------------+----------------------------------------------------------------------------------------+
| Function | State variables written | Conditions on msg.sender                                                               |
+----------+-------------------------+----------------------------------------------------------------------------------------+
| func     | []                      | ['require(bool)(msg.sender != address(0))', 'require(bool)(msg.sender != address(0))'] |
+----------+-------------------------+----------------------------------------------------------------------------------------+

As you can see, the condition require(bool)(msg.sender != address(0)) is listed twice, which is incorrect. This duplication can obscure the actual conditions and make it harder to understand the authorization logic of the contract.

Root Cause Analysis

The root cause of this issue lies in the way Slither handles modifiers when generating the vars-and-auth output. The problem stems from the fact that both Function and Modifier types return True for isinstance(ir.function, Function) in the Slither codebase. More specifically, the issue is located in the authorization.py file within the slither/printers/functions directory.

# https://github.com/crytic/slither/blob/dac531400a2574b8acfcf11967375727a08f49ad/slither/printers/functions/authorization.py#L20-L29
for ir in contract.all_internal_calls():
    if isinstance(ir.function, Function):
        if ir.function.visibility in ['public', 'external']:
            for modifier in ir.function.modifiers:
                if modifier.calls_require_or_assert:
                    conditions = [str(x) for x in modifier.calls_require_or_assert]
                    rows.append([ir.function.name, [], conditions])

The code iterates through all internal calls within the contract and checks if the called function is an instance of the Function class. Due to the fact that modifiers are also considered functions in Slither's internal representation, the condition isinstance(m, Function) evaluates to True for both functions and modifiers. This leads to the conditions within the modifier being added twice: once for the function itself and once for the modifier.

The problematic code section is:

if isinstance(ir.function, Function):

This line causes the subsequent logic to be executed for both functions and modifiers, resulting in the duplication.

Code Example to Reproduce the Issue

To reproduce the issue, you can use the following Solidity code:

pragma solidity ^0.8.0;

contract Test {
    modifier mod() {
        require(msg.sender != address(0), "Sender cannot be zero address");
        _;
    }

    function func() public mod {
        // Function logic here
    }

    function func2() public {
        require(msg.sender != address(0), "Sender cannot be zero address");
    }
}

This simple contract Test includes a modifier mod that checks if the message sender is not the zero address. The function func uses this modifier. Additionally, function func2 implements the same require check directly in the function body.

To reproduce the issue, save the code as test.sol and run the following Slither command:

slither test.sol --print vars-and-auth

The output will show the duplicated condition for function func:

Contract Test
+--------+-------------------------+---------------------------------------------------------------------------------------------------------+
| Function | State variables written | Conditions on msg.sender                                                                              |
+--------+-------------------------+---------------------------------------------------------------------------------------------------------+
| func   | []                      | ['require(bool)(msg.sender != address(0), \'Sender cannot be zero address\')', 'require(bool)(msg.sender != address(0), \'Sender cannot be zero address\')'] |
+--------+-------------------------+---------------------------------------------------------------------------------------------------------+
| func2  | []                      | ['require(bool)(msg.sender != address(0), \'Sender cannot be zero address\')']                                     |
+--------+-------------------------+---------------------------------------------------------------------------------------------------------+

As demonstrated, the condition require(bool)(msg.sender != address(0), 'Sender cannot be zero address') is duplicated for the function func, while function func2 which has the require check within the function body does not exhibit this duplication.

You can also use the following Python code to confirm that isinstance(m, Function) returns True for a modifier:

from slither.slither import Slither
from slither.core.declarations.function import Function

slither = Slither('test.sol')
contract = slither.contracts[0]

for m in contract.modifiers:
    print(m.name)
    print(isinstance(m, Function))   # True

This script will print the name of the modifier and True, confirming that modifiers are treated as functions by the isinstance check.

Proposed Solution

The proposed solution is to remove line 28 in the authorization.py file:

# line 28: if isinstance(m, Function):

This line is responsible for the duplicate inclusion of modifier conditions. By removing this check, the conditions will only be added once, resolving the duplication issue. Removing this line will prevent the duplicate inclusion of modifier conditions. The corrected code will then accurately display the conditions on msg.sender without redundancy. This will provide a clearer and more accurate analysis output, improving the usability of Slither for security audits and code reviews.

Version Information

This issue was observed in Slither version 0.11.3. While this report specifically addresses version 0.11.3, the underlying logic causing the duplication may exist in other versions as well. Therefore, the proposed solution should be considered for implementation across different versions of Slither to ensure consistent and accurate output.

Relevant Log Output

The log output from running Slither with the --print vars-and-auth option on the example contract clearly shows the duplicated conditions:

INFO:Printers:
Contract Test
+----------+-------------------------+----------------------------------------------------------------------------------------+
| Function | State variables written | Conditions on msg.sender                                                               |
+----------+-------------------------+----------------------------------------------------------------------------------------+
| func     | []                      | ['require(bool)(msg.sender != address(0))', 'require(bool)(msg.sender != address(0))'] |
+----------+-------------------------+----------------------------------------------------------------------------------------+

INFO:Slither:test.sol analyzed (1 contracts)

This output confirms the presence of the duplicated require statement for the func function.

In conclusion, the duplication of msg.sender conditions in Slither's vars-and-auth printer is a bug that can lead to confusion and misinterpretation of contract authorization logic. The root cause is the incorrect handling of modifiers as functions within the printer's logic. Removing the line if isinstance(m, Function): from the authorization.py file will resolve this issue. This fix ensures that conditions are added only once, providing a clearer and more accurate output for Slither users. By addressing this bug, Slither's utility as a static analysis tool is enhanced, enabling developers to more effectively identify and understand security implications within their smart contracts. This ultimately contributes to the development of more secure and reliable decentralized applications.