[Bug]: Slither does not distinguish between types and type expressions

[kevinclancy]

Describe the issue:

Slither doesn't distinguish between type expressions, of which uint[NUM_PHASES] is a member, and types, of which uint[NUM_PHASES] is not a member but uint[3] is. This causes it to miss applications of using/for.

In the example source, Slither does not convert the type expression uint[NUM_PHASES] to the type uint[3], and so IR generation is unable to recognize z.proj0 as a library call.

Code example to reproduce the issue:

library Lib {
    function proj0(uint[3] memory a) external returns(uint) {
        return a[0];
    }
}

contract Test {
    uint constant NUM_PHASES = 3;

    using Lib for uint[3];

    function test(uint[NUM_PHASES] memory z) external {
        z.proj0();
    }
}

Version:

0.9.6

Relevant log output:

INFO:Printers:Contract Lib
        Function Lib.proj0(uint256[3]) (*)
                Expression: a[0]
                IRs:
                        REF_0(uint256) -> a[0]
                        RETURN REF_0
Contract Test
        Function Test.test(uint256[3]) (*)
                Expression: z.proj0()
                IRs:
                        TMP_0(None) = HIGH_LEVEL_CALL, dest:z(uint256[3]), function:proj0, arguments:[]  
        Function Test.slitherConstructorConstantVariables() (*)
                Expression: NUM_PHASES = 3
                IRs:
                        NUM_PHASES(uint256) := 3(uint256)

[0xalpharush]

This happens because when ArrayType.__eq__ is called here the Literal values are compared and one has a value with type int and the other str https://github.com/crytic/slither/blob/0de7a251ebe5067082e12d67d71b5169bf6befeb/slither/slithir/convert.py#L547

[kevinclancy]

__eq__ actually compares an Identifier with a Literal. NUM_PHASES is an identifier, while 3 is a literal.

[0xalpharush]

My mistake. I had changed it to the folded value, length_value, locally and was commenting on that. Can you explain what this unresolved issue, https://github.com/CertiKProject/slither-task/issues/689, from https://github.com/CertiKProject/slither-certik/pull/57 is so we can make sure we fix it properly?

[kevinclancy]

Sure. Issue 689 is a just a copy-paste of this issue.

CertikProject#57 removes the __eq__ method from Literal and moves its code into the ArrayType class. I made this change because having an __eq__ method without a __hash__ method made Literal unhashable, and I don't think it's ideal to have one subclass of Expression use structural equality while all the others are using reference equality.

I discovered issue 689 while working on CertikProject#57.

[0xalpharush]

The .value can be different types here and need to both be str or int to be compared https://github.com/crytic/slither/blob/e3dcf1ecd3e9de60da046de471c5663ab637993a/slither/core/expressions/literal.py#L53