Cyfrin / aderyn

Aderyn 🦜 Rust-based Solidity AST analyzer.
GNU Affero General Public License v3.0
274 stars 34 forks source link

Bug: Index out of bound panic in `get_source_code_of_node` #369

Closed jf-li00 closed 1 week ago

jf-li00 commented 2 weeks ago

Describe the bug When I run the example mentioned in #301 , the program panicked due to an index out of bound error.

thread '<unnamed>' panicked at aderyn_core/src/context/workspace_context.rs:1254:48:
byte index 9636 is out of bounds of `// SPDX-License-Identifier: MIT

in release mode it is another contract but the same problem happens:

thread '<unnamed>' panicked at aderyn_core/src/context/workspace_context.rs:1254:48:
byte index 8801 is out of bounds of `// SPDX-License-Identifier: MIT

To Reproduce Steps to reproduce the behavior:

  1. run on this repo: https://github.com/smartcontractkit/ccip/tree/ccip-develop/contracts
  2. See error
jf-li00 commented 2 weeks ago

I spent some time debugging and found that before the painc happens, in the function get_source_code_of_node in aderyn_core/src/context/workspace_context.rs,
the node_id variable is 159001 . How ever, the id field of node variable and source_unit variable are both 159002. Since the node variable is declared as let node = self.nodes.get(&node_id)?;, I think node.id == node_id should hold

jf-li00 commented 2 weeks ago

I spent some time debugging and found that before the painc happens, in the function get_source_code_of_node in aderyn_core/src/context/workspace_context.rs, the node_id variable is 159001 . How ever, the id field of node variable and source_unit variable are both 159002. Since the node variable is declared as let node = self.nodes.get(&node_id)?;, I think node.id == node_id should hold

forget about that, please #370

TilakMaddy commented 2 weeks ago

Thanks for bringing this up!

I'll send a fix

TilakMaddy commented 2 weeks ago

@jf-li00 Can you check it once after the PR gets in and let us know?

jf-li00 commented 2 weeks ago

@TilakMaddy I've checked it, this time it won't panic like what I mentioned before :)

jf-li00 commented 2 weeks ago

But we still can't find the right content of the source due to the duplication of AST node ids in WorkspaceContext introduced bt solc

TilakMaddy commented 2 weeks ago

Yes you are right, my PR only fixes the pipeline from panicking. In reality, the node id replacement problem still exists. But we plan on fixing that in our new internal compilation framework that we are working on.

In that, we have the ability to break down different contexts while ensuring there is no repeating node IDs. Also the best part is we no longer rely on foundry toolchain installation, hardhat, etc.

The framework will download the relevant solc binaries and create multiple WorkspaceContexts for each project. (And as far as writing the detector goes, nothing changes. Only the internals from our end change)

I'll keep you posted : )

alexroan commented 2 weeks ago

What is the root cause of the NodeID replacement? My understanding is that it is due to stale/old output files from previous compilations. If you run forge clean on the target repo, then run aderyn again, do you still get a panic @jf-li00 ?

TilakMaddy commented 2 weeks ago

By node replacement, I meant self.nodes hashmap is abused by replacing the value for the same key node.id when in reality we expect a new node to be added! :P

TilakMaddy commented 1 week ago

Fixed here https://github.com/Cyfrin/aderyn/pull/371