[Bug]: Library call function is not an instance of `Function`

[0xalpharush]

Describe the issue:

Library call function is not an instance of Function Related with this library call here https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L112

Code example to reproduce the issue:

git clone https://github.com/code-423n4/2023-01-timeswap
yarn
cd v2-token
slither .

Version:

0.9.2

Relevant log output:

File "/Users/alpharush/tob/slither/slither/core/cfg/node.py", line 920, in _find_read_write_call
    assert isinstance(ir.function, Function)
AssertionError

[0xGusMcCrae]

I tried to figure this out after noticing it in the c4 audit for timeswap.

As far as I can tell, the issue is that v2-option, v2-pool, and v2-token all have ParamLibrary contracts with check functions in their respective structs/Param.sol files. When they are used in the same directory, as they are in packages/v2-pool and packages/v2-token, slither mixes up the ParamLibraries.

The issue appears to be with ParamLibrary.check(param), which is only present in v2-pool and v2-token's ParamLibrary. v2-option's ParamLibrary.check functions all have a second parameter, blockTimestamp. The assertion error occurs while slither is processing ParamLibrary.check in both TimeswapV2Token.sol and TimeswapV2Pool.sol.

I added some print statements throughout slither to try to debug.

in slither/slithir/convert.py (around line 100):

def convert_expression(expression, node):
# handle standlone expression
# such as return true;
from slither.core.cfg.node import NodeType

print("beginning of convert_expression")
print(expression)
try:
    print(expression.called.expression.value.file_scope)
    print([f.name for f in expression.called.expression.value.functions])
except:
    None

Running slither . in packages/v2-token:

beginning of convert_expression
ParamLibrary.check(param)
../v2-option/src/structs/Param.sol
['check', 'check', 'check', 'check']
Traceback (most recent call last):
(error is thrown here)

This is occurring in TimeswapV2Token.mint(TimeswapV2TokenMintParam) (as you've stated above). It's using check(TimeswapV2TokenMintParam memory param) from v2-token's ParamLibrary.

Running slither . in packages/v2-pool gets the same result:

beginning of convert_expression
ParamLibrary.check(param)
../v2-option/src/structs/Param.sol
['check', 'check', 'check', 'check']
Traceback (most recent call last):
(error is thrown here)

This one was occurring in TimeswapV2Pool.collectProtocolFees(TimeswapV2PoolCollectParam). This time it's using check(TimeswapV2PoolCollectParam memory param) from v2-pool's ParamLibrary.

All of v2-option's ParamLibrary.check functions have an additional uint96 input blockTimestamp, so the expression ParamLibrary(check) can't belong to v2-option's ParamLibrary. But in both cases, the expression's node is giving the v2-option param.sol file as its file_scope and listing its parent contract functions as being 4 check functions, which is only valid for v2-option since v2-pool's ParamLibrary has 6 check functions and v2-token's ParamLibrary has 5 functions.

I haven't been able to pinpoint where the mismatch is originating, but it appears that there's some sort of incorrect assignment that matches the v2-pool and v2-token ParamLibrary.check functions to v2-option's ParamLibrary file and contract. I tried to backtrack to find the error but kind of hit a dead end. If someone more familiar with the codebase knows where this might be occurring, I could take another look and try to fix it.

I have no idea why this is causing it to throw an AssertionError on isinstance(ir.function, Function), but something is going wrong here.

[0xalpharush]

@0xGusMcCrae, thanks for taking the time tracking this down. Based off the duplicate function names, I suspect this may be fixed by #1625 but I need to double check

[smonicas]

To add something more the issue appears when parsing an expression. The find_variable returns the ParamLibrary with 4 functions always because it's found in the currect scope. https://github.com/crytic/slither/blob/0ec487460690482c72cacdea6705e2c51bb3981e/slither/solc_parsing/expressions/expression_parsing.py#L472 https://github.com/crytic/slither/blob/0ec487460690482c72cacdea6705e2c51bb3981e/slither/solc_parsing/expressions/find_variable.py#L405-L407

[0xalpharush]

@smonicas this is similar to the issue outlined in https://github.com/crytic/slither/issues/1809#issuecomment-1492114159 ref https://github.com/crytic/slither/issues/1809

[smonicas]

The problem is that contracts is a dict with key the contract name however when updating with the new accessible contracts will overwrite the older contract with the same name even if it's actually a different contract (e.g. in a different file). https://github.com/crytic/slither/blob/0ec487460690482c72cacdea6705e2c51bb3981e/slither/core/scope/scope.py#L32 https://github.com/crytic/slither/blob/0ec487460690482c72cacdea6705e2c51bb3981e/slither/core/scope/scope.py#L68-L70

[0xalpharush]

Any issue that can be fixed directly in Slither is fixed in https://github.com/crytic/slither/pull/2404 so this is blocked on https://github.com/foundry-rs/foundry/issues/7591