-
Notifications
You must be signed in to change notification settings - Fork 983
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
base: dev
Are you sure you want to change the base?
Must depend on Analysis #2502
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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: | ||||||||||||||
return [] | ||||||||||||||
return [list(must_dependencies)[0]] | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def compute_must_dependencies(v: SUPPORTED_TYPES) -> Set[Variable]: | ||||||||||||||
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"]: | ||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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: | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
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 | ||||||||||||||
################################################################################### | ||||||||||||||
################################################################################### | ||||||||||||||
|
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 | ||
} | ||
} |
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 | ||
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" |
There was a problem hiding this comment.
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.Committable suggestion
Tools
Ruff