Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Must depend on Analysis #2502

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions slither/analyses/data_dependency/data_dependency.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,74 @@
return context.context[KEY_SSA]


def get_must_depends_on(variable: SUPPORTED_TYPES) -> List:
"""
Return must dependency of a variable if exist otherwise return None.

:param variable: target variable whose must dependency needs to be computed
:return: Variable | None
"""
must_dependencies = compute_must_dependencies(variable)
if len(must_dependencies) > 1 or len(must_dependencies) == 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(must_dependencies) > 1 or len(must_dependencies) == 0:
if len(must_dependencies) != 1:

return []
return [list(must_dependencies)[0]]

0xalpharush marked this conversation as resolved.
Show resolved Hide resolved

def compute_must_dependencies(v: SUPPORTED_TYPES) -> Set[Variable]:

Check warning on line 309 in slither/analyses/data_dependency/data_dependency.py

View workflow job for this annotation

GitHub Actions / Lint Code Base

R0912: Too many branches (16/12) (too-many-branches)
if isinstance(v, (SolidityVariableComposed, Constant)) or (
v.function.visibility in ["public", "external"] and v in v.function.parameters
):
return set([v])

function_dependencies = {}
function_dependencies["context"] = {}
lvalues = []

for node in v.function.nodes:
for ir in node.irs_ssa:
if isinstance(ir, OperationWithLValue) and ir.lvalue:
if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage:
continue
if isinstance(ir.lvalue, ReferenceVariable):
lvalue = ir.lvalue.points_to
if lvalue:
lvalues.append((lvalue, v.function, ir))
lvalues.append((ir.lvalue, v.function, ir))

for lvalue_details in lvalues:
lvalue = lvalue_details[0]
ir = lvalue_details[2]

if not lvalue in function_dependencies["context"]:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix membership test.

Convert the membership test to not in for better readability.

- if not lvalue in function_dependencies["context"]:
+ if lvalue not in function_dependencies["context"]:
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if not lvalue in function_dependencies["context"]:
if lvalue not in function_dependencies["context"]:
Tools
Ruff

328-328: Test for membership should be not in

Convert to not in

(E713)

function_dependencies["context"][lvalue] = set()
read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]]

if isinstance(ir, Index):
read = [ir.variable_left]
elif isinstance(ir, InternalCall) and ir.function:
read = ir.function.return_values_ssa
else:
read = ir.read
for variable in read:
# if not isinstance(variable, Constant):
function_dependencies["context"][lvalue].add(variable)
function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"])

must_dependencies = set()
data_dependencies = (
list(function_dependencies["context"][v])
if function_dependencies["context"] is not None
else []
)
Comment on lines +350 to +354
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
data_dependencies = (
list(function_dependencies["context"][v])
if function_dependencies["context"] is not None
else []
)
data_dependencies = list(function_dependencies["context"].get(v, set()))

This could probably be a defaultdict with an empty set as the default to avoid this

for i, data_dependency in enumerate(data_dependencies):
result = compute_must_dependencies(data_dependency)
if i > 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this condition as it seems it will always take the intersection. Could you clarify why this is done?

must_dependencies = must_dependencies.intersection(result)
else:
must_dependencies = must_dependencies.union(result)
return must_dependencies
0xalpharush marked this conversation as resolved.
Show resolved Hide resolved


# endregion
###################################################################################
###################################################################################
Expand Down
30 changes: 30 additions & 0 deletions tests/unit/core/test_data/must_depend_on.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
pragma solidity ^0.8.19;

interface IERC20 {
function transferFrom(address from, address to, uint amount) external returns (bool);
}

/**
* @title MissingReturnBug
* @author IllIllI
*/

// test case of the missing return bug described here:
// https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
contract Unsafe {
IERC20 erc20;
function good2(address to, uint256 am) public {
address from_msgsender = msg.sender;
int_transferFrom(from_msgsender, to, am); // from is constant
}

// This is not detected
function bad2(address from, address to, uint256 am) public {
address from_msgsender = msg.sender;
int_transferFrom(from_msgsender, to, amount); // from is not a constant
}

function int_transferFrom(address from, address to, uint256 amount) internal {
erc20.transferFrom(from, to, amount); // not a constant = not a constant U constant
}
}
30 changes: 30 additions & 0 deletions tests/unit/core/test_must_depend_on.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from pathlib import Path
from slither import Slither
from slither.analyses.data_dependency.data_dependency import get_must_depends_on
from slither.core.variables.variable import Variable
from slither.core.declarations import SolidityVariable, SolidityVariableComposed
from typing import Union

Check warning on line 6 in tests/unit/core/test_must_depend_on.py

View workflow job for this annotation

GitHub Actions / Lint Code Base

C0411: standard import "from typing import Union" should be placed before "from slither import Slither" (wrong-import-order)
from slither.slithir.variables import (
Constant,
)
0xalpharush marked this conversation as resolved.
Show resolved Hide resolved

TEST_DATA_DIR = Path(__file__).resolve().parent / "test_data"
SUPPORTED_TYPES = Union[Variable, SolidityVariable, Constant]


def test_must_depend_on_returns(solc_binary_path):
solc_path = solc_binary_path("0.8.19")
file = Path(TEST_DATA_DIR, "must_depend_on.sol").as_posix()
slither_obj = Slither(file, solc=solc_path)

for contract in slither_obj.contracts:
for function in contract.functions:
if contract == "Unsafe" and function == "int_transferFrom":
result = get_must_depends_on(function.parameters[0])
break
assert isinstance(result, list)
assert result[0] == SolidityVariableComposed("msg.sender"), "Output should be msg.sender"

result = get_must_depends_on(slither_obj.contracts[1].functions[2].parameters[1])
assert isinstance(result, list)
assert len(result) == 0, "Output should be empty"
Loading