Slither should identify storage variables accessed under a different name

[schemar]

Hey, we came across this during the Trail of Bits workshop with @japesinator. When we are looking for a storage variable to be written to, slither does not find it if we access it under a different name in the solidity code.

We didn't figure out how to catch it with the currently available API. We were thinking about recursively tracking also variables where the storage variable would be read.

It should be possible for slither to identify these cases and report them as "writing to the storage variable with that name".

Minimal Example:

Python

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

slither = Slither('test.sol')

def get_msg_sender_checks(function):
    all_functions = function.all_internal_calls() + \
        [function] + function.modifiers

    all_nodes = [f.nodes for f in all_functions if isinstance(f, Function)]
    all_nodes = [item for sublist in all_nodes for item in sublist]

    all_conditional_nodes = [n for n in all_nodes if
                             n.contains_if() or n.contains_require_or_assert()]
    all_conditional_nodes_on_msg_sender = [str(n.expression) for n in all_conditional_nodes if
                                           'msg.sender' in [v.name for v in n.solidity_variables_read]]
    return all_conditional_nodes_on_msg_sender


for contract in slither.contracts:
    print('Contract: ' + contract.name)

    for function in contract.functions:

        msg_sender_condition = get_msg_sender_checks(function)

        if len(msg_sender_condition) == 0:
            for v in function.state_variables_written:
                if v.name == 'balances':
                    print('Function: {}'.format(function.name))
                    print('\tWriting to balances: {}'.format(v.name))

Solidiy

// test.sol

contract Test
{
    struct Balance {
        uint balance;
    }

    Balance public balances;

    function setBalances(uint _balances)
        public
    {
        require(msg.sender == address(0));
        balances.balance = _balances;
    }

    function setBalancesUnchecked(uint _balances) public {
        // `other` writes to storage `balances` as it is
        // uninitialized.
        Balance storage other;
        // Thus, this writes to storage variable balances,
        // but is not detected:
        other.balance = _balances;
    }
}

We later found out that this is also not found:
Balance storage other = balances;

/cc @pgev @benjaminbollen

[montyly]

Hi,

Thank you for the detailed issue! Indeed state_variables_written does not return variables written through a storage access (though it could).

I will look if that makes sense to integrate directly. In the meantime, using slithIR, you can track the assignment of storage local variables to a state variable and detect if a function writes to it:

def get_indirect_writting(function):
    # storage_mapping maps a local variable => state variable
    storage_mapping = {}

    # Collect all the mapping local storage variable -> state variable
    for node in function.nodes:
        for ir in node.irs:
            if isinstance(ir, Assignment):
                if isinstance(ir.lvalue, LocalVariable) and ir.lvalue.is_storage:
                    storage_mapping[ir.lvalue] = ir.rvalue

    # Collect all the local storage variables written
    local_vars_read = [var for var in function.variables_written if
                       isinstance(var, LocalVariable) and var.is_storage]

    # Return all the state variables that are written through a local storage variable
    # If the variable has not mapping, return the first variable of the contract
    return [storage_mapping[v] if
            v in storage_mapping else
            function.contract.state_variables[0]
            for v in local_vars_read]

You can integrate it into your script:

#... 
from slither.slithir.operations import Assignment
from slither.core.variables.local_variable import LocalVariable
# ....
def get_indirect_writing(function):
# ....

for contract in slither.contracts:
    print('Contract: ' + contract.name)

    for function in contract.functions:

        msg_sender_condition = get_msg_sender_checks(function)

        if len(msg_sender_condition) == 0:
            # collect the state variables directly and indirectly written
            all_state_vars_written = function.variables_written + get_indirect_writing(function)
            all_state_vars_written = list(set(all_state_vars_written))
            for v in all_state_vars_written:
                if v.name == 'balances':
                    print('Function: {}'.format(function.name))
                    print('\tWriting to balances: {}'.format(v.name))

Note that Slither detects your code as vulnerable, as uninitialized storage variable should not be used:

$ slither test.sol
 Uninitialized storage variables in test.sol, Contract: Test, Function: setBalancesUnchecked, Variable other

For info, another case that is not (yet) tracked by state_variables_written is the write through storage parameter (ex: function setter(mystruct storage st)).

[schemar]

Thanks @montyly ❤️

Note that Slither detects your code as vulnerable, as uninitialized storage variable should not be used:

Please note that it also works when using Balance storage other = balances; (and thus initializing the variable).

[montyly]

The alias analysis is merged in master, slither is able to detect that

    function set(uint _balances) public {
        Balance storage other = balances;
        // Thus, this writes to storage variable balances,
        // but is not detected:
        other.balance = _balances;
    }

is a write to balances

It does not consider that an uninitialized storage variable points to the first slot in memory, but I think that's ok as it does report uninitialized storage reference as a bug.