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.

Sign up for GitHub

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

Feature/extract manipulated state variables #639

Closed
wants to merge 5 commits into from
Closed

Feature/extract manipulated state variables #639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
71adcf3
done
TilakMaddy Jul 31, 2024
43824a2
change name
TilakMaddy Jul 31, 2024
28f6329
cli/reportgen
TilakMaddy Jul 31, 2024
f86b440
added support struct.prop = value
TilakMaddy Jul 31, 2024
2932410
cli/reportgen
TilakMaddy Jul 31, 2024
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
183 changes: 183 additions & 0 deletions aderyn_core/src/context/browser/extractor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashSet;

use crate::{
ast::*,
visitor::ast_visitor::{ASTConstVisitor, Node},
Expand Down Expand Up @@ -125,3 +127,184 @@ impl ASTConstVisitor for ExtractReferencedDeclarations {
Ok(true)
}
}

// Extract Reference Declaration IDs
#[derive(Default)]
pub struct ExtractManipulatedStateVariablesIDs {
pub deleted: HashSet<NodeID>,
pub assigned: HashSet<NodeID>,
pub pushed: HashSet<NodeID>,
pub popped: HashSet<NodeID>,
}

impl ExtractManipulatedStateVariablesIDs {
pub fn get_all_node_ids(&self) -> Vec<NodeID> {
let mut all_nodes = [
self.deleted.clone().into_iter().collect::<Vec<_>>(),
self.assigned.clone().into_iter().collect::<Vec<_>>(),
self.pushed.clone().into_iter().collect::<Vec<_>>(),
self.popped.clone().into_iter().collect::<Vec<_>>(),
]
.concat();
// Some state variables can undergo more than 1 of the above operation.
// Hence, we should deduplicate it
all_nodes.dedup();
all_nodes
}

pub fn from<T: Node + ?Sized>(node: &T) -> Self {
let mut extractor: ExtractManipulatedStateVariablesIDs = Self::default();
node.accept(&mut extractor).unwrap_or_default();
extractor
}
}

impl ASTConstVisitor for ExtractManipulatedStateVariablesIDs {
fn visit_unary_operation(&mut self, node: &UnaryOperation) -> Result<bool> {
// Catch delete operations
if node.operator == "delete" {
if let Some(id) = find_referenced_declaration_for_identifier_or_indexed_identifier(
node.sub_expression.as_ref(),
) {
self.deleted.insert(id);
}
}
Ok(true)
}

fn visit_member_access(&mut self, member: &MemberAccess) -> Result<bool> {
if let Some(id) = find_referenced_declaration_for_identifier_or_indexed_identifier(
member.expression.as_ref(),
) {
if member.member_name == "push" {
self.pushed.insert(id);
} else if member.member_name == "pop" {
self.popped.insert(id);
}
}
Ok(true)
}

fn visit_assignment(&mut self, assignment: &Assignment) -> Result<bool> {
if let Some(id) = find_referenced_declaration_for_identifier_or_indexed_identifier(
assignment.left_hand_side.as_ref(),
) {
self.assigned.insert(id);
}
Ok(true)
}
}

fn find_referenced_declaration_for_identifier_or_indexed_identifier(
expr: &Expression,
) -> Option<NodeID> {
match expr {
Expression::Identifier(Identifier {
referenced_declaration: Some(id),
..
}) => {
return Some(*id);
}
Expression::IndexAccess(IndexAccess {
base_expression, ..
}) => {
return find_referenced_declaration_for_identifier_or_indexed_identifier(
base_expression.as_ref(),
);
}
Expression::MemberAccess(MemberAccess { expression, .. }) => {
return find_referenced_declaration_for_identifier_or_indexed_identifier(
expression.as_ref(),
);
}
_ => (),
};
None
}

#[cfg(test)]
mod written_state_variables_tests {
use crate::detect::test_utils::load_solidity_source_unit;

use super::ExtractManipulatedStateVariablesIDs;

#[test]
fn has_variable_declarations() {
let context =
load_solidity_source_unit("../tests/contract-playground/src/StateVariablesWritten.sol");

assert!(!context.variable_declarations().is_empty());
}

#[test]
fn can_capture_deletes() {
let context =
load_solidity_source_unit("../tests/contract-playground/src/StateVariablesWritten.sol");

let mut total_state_variables_deleted = 0;

for contract in context.contract_definitions() {
let state_variables_info = ExtractManipulatedStateVariablesIDs::from(contract);
println!("{} - {}", contract.name, state_variables_info.deleted.len());
println!("{:?}", state_variables_info.deleted);
total_state_variables_deleted += state_variables_info.deleted.len();
}

assert_eq!(total_state_variables_deleted, 5);
}

#[test]
fn can_capture_pushes() {
let context =
load_solidity_source_unit("../tests/contract-playground/src/StateVariablesWritten.sol");

let mut total_state_variables_pushed_to = 0;

for contract in context.contract_definitions() {
let state_variables_info = ExtractManipulatedStateVariablesIDs::from(contract);
println!("{} - {}", contract.name, state_variables_info.pushed.len());
println!("{:?}", state_variables_info.pushed);
total_state_variables_pushed_to += state_variables_info.pushed.len();
}

assert_eq!(total_state_variables_pushed_to, 2);
}

#[test]
fn can_capture_pops() {
let context =
load_solidity_source_unit("../tests/contract-playground/src/StateVariablesWritten.sol");

let mut total_state_variables_popped = 0;

for contract in context.contract_definitions() {
let state_variables_info = ExtractManipulatedStateVariablesIDs::from(contract);
println!("{} - {}", contract.name, state_variables_info.popped.len());
println!("{:?}", state_variables_info.popped);
total_state_variables_popped += state_variables_info.popped.len();
}

assert_eq!(total_state_variables_popped, 1);
}

#[test]
fn can_capture_assignments() {
let context =
load_solidity_source_unit("../tests/contract-playground/src/StateVariablesWritten.sol");

let mut total_state_variables_assigned = 0;

for contract in context.contract_definitions() {
let state_variables_info = ExtractManipulatedStateVariablesIDs::from(contract);
println!(
"{} - {}",
contract.name,
state_variables_info.assigned.len()
);
Comment on lines +298 to +303
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use this pattern :)

println!("{:?}", state_variables_info.assigned);
total_state_variables_assigned += state_variables_info.assigned.len();
}

assert_eq!(total_state_variables_assigned, 11);
}
}
38 changes: 36 additions & 2 deletions reports/report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"files_summary": {
"total_source_units": 73,
"total_sloc": 1996
"total_source_units": 74,
"total_sloc": 2094
},
"files_details": {
"files_details": [
Expand Down Expand Up @@ -161,6 +161,10 @@
"file_path": "src/StateVariables.sol",
"n_sloc": 58
},
{
"file_path": "src/StateVariablesWritten.sol",
"n_sloc": 98
},
{
"file_path": "src/StorageConditionals.sol",
"n_sloc": 59
Expand Down Expand Up @@ -2061,6 +2065,12 @@
"src": "32:23",
"src_char": "32:23"
},
{
"contract_path": "src/StateVariablesWritten.sol",
"line_no": 2,
"src": "32:23",
"src_char": "32:23"
},
{
"contract_path": "src/UncheckedSend.sol",
"line_no": 2,
Expand Down Expand Up @@ -2128,6 +2138,12 @@
"src": "2121:14",
"src_char": "2121:14"
},
{
"contract_path": "src/StateVariablesWritten.sol",
"line_no": 59,
"src": "1733:22",
"src_char": "1733:22"
},
{
"contract_path": "src/ZeroAddressCheck.sol",
"line_no": 43,
Expand Down Expand Up @@ -2213,6 +2229,18 @@
"src": "2539:25",
"src_char": "2539:25"
},
{
"contract_path": "src/StateVariablesWritten.sol",
"line_no": 33,
"src": "958:8",
"src_char": "958:8"
},
{
"contract_path": "src/StateVariablesWritten.sol",
"line_no": 48,
"src": "1414:12",
"src_char": "1414:12"
},
{
"contract_path": "src/UninitializedStateVariable.sol",
"line_no": 17,
Expand Down Expand Up @@ -2807,6 +2835,12 @@
"src": "32:23",
"src_char": "32:23"
},
{
"contract_path": "src/StateVariablesWritten.sol",
"line_no": 2,
"src": "32:23",
"src_char": "32:23"
},
{
"contract_path": "src/StorageConditionals.sol",
"line_no": 2,
Expand Down
45 changes: 38 additions & 7 deletions reports/report.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati

| Key | Value |
| --- | --- |
| .sol Files | 73 |
| Total nSLOC | 1996 |
| .sol Files | 74 |
| Total nSLOC | 2094 |


## Files Details
Expand Down Expand Up @@ -121,6 +121,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
| src/SendEtherNoChecks.sol | 58 |
| src/StateShadowing.sol | 17 |
| src/StateVariables.sol | 58 |
| src/StateVariablesWritten.sol | 98 |
| src/StorageConditionals.sol | 59 |
| src/StorageParameters.sol | 16 |
| src/T11sTranferer.sol | 8 |
Expand Down Expand Up @@ -155,7 +156,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
| src/reused_contract_name/ContractB.sol | 7 |
| src/uniswap/UniswapV2Swapper.sol | 50 |
| src/uniswap/UniswapV3Swapper.sol | 150 |
| **Total** | **1996** |
| **Total** | **2094** |


## Issue Summary
Expand Down Expand Up @@ -2008,7 +2009,7 @@ ERC20 functions may not behave as expected. For example: return values are not a

Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;`

<details><summary>19 Found Instances</summary>
<details><summary>20 Found Instances</summary>


- Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2)
Expand Down Expand Up @@ -2077,6 +2078,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid
pragma solidity ^0.4.0;
```

- Found in src/StateVariablesWritten.sol [Line: 2](../tests/contract-playground/src/StateVariablesWritten.sol#L2)

```solidity
pragma solidity ^0.8.0;
```

- Found in src/UncheckedSend.sol [Line: 2](../tests/contract-playground/src/UncheckedSend.sol#L2)

```solidity
Expand Down Expand Up @@ -2133,7 +2140,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid

Check for `address(0)` when assigning values to address state variables.

<details><summary>6 Found Instances</summary>
<details><summary>7 Found Instances</summary>


- Found in src/ArbitraryTransferFrom.sol [Line: 12](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L12)
Expand All @@ -2148,6 +2155,12 @@ Check for `address(0)` when assigning values to address state variables.
addr = newAddr;
```

- Found in src/StateVariablesWritten.sol [Line: 59](../tests/contract-playground/src/StateVariablesWritten.sol#L59)

```solidity
simpleAddress = _value;
```

- Found in src/ZeroAddressCheck.sol [Line: 43](../tests/contract-playground/src/ZeroAddressCheck.sol#L43)

```solidity
Expand Down Expand Up @@ -2180,7 +2193,7 @@ Check for `address(0)` when assigning values to address state variables.

Instead of marking a function as `public`, consider marking it as `external` if it is not used internally.

<details><summary>23 Found Instances</summary>
<details><summary>25 Found Instances</summary>


- Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28)
Expand Down Expand Up @@ -2237,6 +2250,18 @@ Instead of marking a function as `public`, consider marking it as `external` if
function setNonEmptyAlteredNumbers(
```

- Found in src/StateVariablesWritten.sol [Line: 33](../tests/contract-playground/src/StateVariablesWritten.sol#L33)

```solidity
function addValue(address _user, uint256 _id, int64 _value) public {
```

- Found in src/StateVariablesWritten.sol [Line: 48](../tests/contract-playground/src/StateVariablesWritten.sol#L48)

```solidity
function deleteValues(address _user, uint256 _id) public {
```

- Found in src/UninitializedStateVariable.sol [Line: 17](../tests/contract-playground/src/UninitializedStateVariable.sol#L17)

```solidity
Expand Down Expand Up @@ -2774,7 +2799,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail.

<details><summary>28 Found Instances</summary>
<details><summary>29 Found Instances</summary>


- Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2)
Expand Down Expand Up @@ -2843,6 +2868,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai
pragma solidity 0.8.20;
```

- Found in src/StateVariablesWritten.sol [Line: 2](../tests/contract-playground/src/StateVariablesWritten.sol#L2)

```solidity
pragma solidity ^0.8.0;
```

- Found in src/StorageConditionals.sol [Line: 2](../tests/contract-playground/src/StorageConditionals.sol#L2)

```solidity
Expand Down
Loading
Loading