crytic / crytic/slither

solidity_signature is wrong for structs with repeated convertible field types (shared `seen` set)

Open
#3,064 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
6.4k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

### Describe the issue

`Function.solidity_signature` silently renders the **wrong** signature for a struct parameter when
two or more of the struct's fields have the same type *and* that type requires conversion
(user-defined value type, enum, or contract/interface). The first such field converts correctly; the
second and later ones emit the alias name instead of its underlying elementary type.

Because `Contract.get_function_from_signature` matches strictly, the affected functions become
**unresolvable by signature** — any correctly-derived ABI signature can never match. It fails
silently: no exception, no warning, just a signature that disagrees with the compiler.

### Minimal reproduction

```solidity
// Repro.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

type Amount is uint256;

struct Pair {
Amount a;
Amount b; // same user-defined value type as `a`
}

contract Repro {
function f(Pair calldata p) external pure returns (uint256) {
return Amount.unwrap(p.a) + Amount.unwrap(p.b);
}
}
```

```python
from slither import Slither

sl = Slither("Repro.sol")
fn = next(f for c in sl.contracts for f in c.functions_entry_points if f.name == "f")
print(fn.solidity_signature)
```

```
actual f((uint256,Amount))
expected f((uint256,uint256)) # solc --combined-json abi agrees with `expected`
```

The second `Amount` field is rendered as `Amount`; the first converts correctly to `uint256`.

### It is not specific to user-defined value types

The same collision occurs for any sibling field type that needs conversion:

| struct fields | actual | expected |
|---|---|---|
| `Amount a; Amount b;` (UDVT over `uint256`) | `fAlias((uint256,Amount))` | `fAlias((uint256,uint256))` |
| `Flag a; Flag b;` (enum) | `fEnum((uint8,Flag))` | `fEnum((uint8,uint8))` |
| `IThing a; IThing b;` (interface) | `fContract((address,IThing))` | `fContract((address,address))` |

Two `uint256` fields are unaffected, which is why this hides: returning the type unconverted is
harmless precisely when no conversion was needed.

### Root cause

`slither/utils/type.py` — `convert_type_for_solidity_signature_to_string` creates **one** `seen` set
and threads it through the entire traversal, including across sibling struct fields:

```python
def convert_type_for_solidity_signature_to_string(t: Type) -> str:
seen: set[Type] = set()
types = convert_type_for_solidity_signature(t, seen)
return _convert_type_for_solidity_signature_to_string(types, seen)

def convert_type_for_solidity_signature(t: Type, seen: set[Type]) -> Type | list[Type]:
if t in seen:
return t # <-- returns UNCONVERTED
seen.add(t)
...
if isinstance(underlying_type, Structure):
types = [
convert_type_for_solidity_signature(x.type, seen) # <-- siblings share `seen`
for x in underlying_type.elems_ordered
]
```

The guard is a **self-recursion** guard, and the comment above it says so — a struct that contains
itself must terminate. But `seen` accumulates every type visited anywhere in the traversal rather
than the current descent, so a repeat across *siblings* is misread as a recursive back-edge. The
membership test is `==`/`__hash__`, not identity, so two distinct-but-equal field types collide too.

### A note for whoever fixes it

The obvious fix — scope `seen` to the current descent (add on entry, drop on exit) — fixes all three
rows above but **reintroduces non-termination** on genuinely recursive structs. The shared mutable
`seen` is currently load-bearing across the *phase boundary*: `_convert_type_for_solidity_signature_to_string`
calls back into `convert_type_for_solidity_signature` when it unwraps an `ArrayType`, and relies on
entries left in `seen` by the first phase to stop `struct Node { uint256 v; Node[] kids; }` from
looping forever. I hit exactly this while testing.

Passing the ancestor set (a path) by value through **both** phases satisfies both properties. I
verified this locally against the three cases above plus a recursive struct, where it still yields
upstream's existing output `(uint256,Node[])` and terminates.

### Impact

Any consumer of `solidity_signature` for structs with repeated field types. Concretely, Uniswap V4's
`PoolKey` has two `Currency` fields (a UDVT over `address`) and is passed to all ten hook callbacks,
so every V4 hook contract's entry points are unresolvable by signature. In our use (matching
functions by ABI signature for analysis coverage) this silently excluded an entire contract from
analysis — the failure mode is "the function was never examined", with no error to notice.

### Frequency

Deterministic — 100% reproducible.

### Version

```
slither-analyzer 0.11.5
```

`slither/utils/type.py` on `master` is byte-identical to 0.11.5 in the relevant functions, so
`master` is affected as well.

Contributor guide

Open the contributing guide

Research direction

Start with slither/utils/type.py, especially convert_type_for_solidity_signature_to_string and convert_type_for_solidity_signature, then run the Repro.sol example through Function.solidity_signature. Verify that repeated convertible fields produce the compiler's ABI signature, while recursive structs still terminate with the existing output and Contract.get_function_from_signature can resolve the result.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, solidity
Domain
blockchain, devtools
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.