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

add immutable-states detector #1455

Merged
merged 6 commits into from
Jan 9, 2023
Merged

add immutable-states detector #1455

merged 6 commits into from
Jan 9, 2023

Conversation

0xalpharush
Copy link
Contributor

@0xalpharush 0xalpharush added the enhancement New feature or request label Nov 28, 2022
Copy link
Member

@montyly montyly left a comment

Choose a reason for hiding this comment

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

We are missing the candidates that are written in the constructor directly, ex:

contract A{
	uint a;
	constructor(uint b) {
		a = b;
	}
}

Maybe we should split this detector in two? As adding the logic for the constructor will increase the complexity, it would make sense to differentiate variables can that be determined at compile time (constant), and variables that can be determined at deployment (immutable) into two distinct detectors.

We can reuse the functions (_constant_initial_expression / _is_valid_type / ..) and mostly change the filtering logic

variables = []
functions = []
for c in self.compilation_unit.contracts:
if is_openzeppelin(c):
Copy link
Member

Choose a reason for hiding this comment

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

Why removing openzepellin?

Maybe we should iterate over compilation_unit.contracts_derived here, so that we only consider the most derived contracts, and we avoid FP on potential constant variable that are written in a derived contract

@0xalpharush 0xalpharush requested a review from smonicas as a code owner January 6, 2023 16:11
@0xalpharush 0xalpharush changed the title update constable-states to consider constructor vars which can be immutable add immutable-states to consider detector Jan 6, 2023
@0xalpharush 0xalpharush changed the title add immutable-states to consider detector add immutable-states detector Jan 6, 2023
@0xalpharush 0xalpharush requested a review from montyly January 9, 2023 13:39
@montyly montyly merged commit 45c5ed9 into dev Jan 9, 2023
@montyly montyly deleted the detect/add-immutable-opti branch January 9, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants