search

hashgraph / hedera-smart-contracts

Contains Hedera Smart Contract Service supporting files
Apache License 2.0
37 stars 51 forks source link

Adds framework for the documentation of analyzed tools #726

Closed nabialek-arianelabs closed 1 month ago

nabialek-arianelabs commented 2 months ago

Adds framework for research documentation of EVM analytic tools:

acuarica commented 2 months ago

Hi @nabialek-arianelabs, thanks for sending this. Is there any particular reason to include specific analysis files, maian-analysis and slither-analysis, in this PR? Does it make sense to include these in their respective PRs? This way the other PRs would be self-contained.

acuarica commented 2 months ago

Is there a reason to include the generated files? To see the actual report result, might be better to include the analysis in CI and published the results from there.

acuarica commented 2 months ago

Hi @arianejasuwienas, we have discussed with the team the main points of these PRs. From its contents, it is not entirely clear what the purpose of these PRs is. However, from your comments it seems the main goal is to evaluate the suitability of these tools in a Hedera project. If that's the case, could you rewrite the docs to make that explicit?

There is no need to include full report and logs output, at most, some sample report output might be embedded in the docs. All the docs may be placed inside a folder tools/analysis (leaving room for other kind of tools in the future). Instructions and config files, e.g., for docker, on how to run these tools might be included as well.

Without the report and logs output the PRs might be just a few files. If that's the case, it might be easier to merge them into a single PR (like the original one posted in the relay https://github.com/hashgraph/hedera-json-rpc-relay/pull/2439).

The references for Etherscan API keys don't apply for Hedera, those should be removed.

Happy to discuss this further is something is not clear.

team in cc @ebadiere @AlfredoG87 @quiet-node

arianejasuwienas commented 2 months ago

@acuarica

Test reports are a critical component of our research. Removing them entirely from the contexts in which we draw conclusions from performed tests would be challenging.

Perhaps, in such cases, we could maintain these reports in a separate repository and reference them in this specific PR.

Omitting Etherscan API information entirely means we lose essential details: Slither's ability to access resources directly from the blockchain is not compatible with Hashscan. My proposal is to condense this information but still retain it in the report.

acuarica commented 2 months ago

Hey @arianejasuwienas,

Test reports are a critical component of our research. Removing them entirely from the contexts in which we draw conclusions from performed tests would be challenging.

I might be missing something here, but why it would be challenging? If the focus is on the Smart Contracts themselves, I agree, but this is not the case. Some example report might be worth including, but not all of them.

Perhaps, in such cases, we could maintain these reports in a separate repository and reference them in this specific PR.

Sure, that might be a potential solution. However, having these reports in a separate repo begs the question of whether to move the whole evaluation into a separate repo.

Omitting Etherscan API information entirely means we lose essential details: Slither's ability to access resources directly from the blockchain is not compatible with Hashscan. My proposal is to condense this information but still retain it in the report.

One clarification first: Etherscan is both a block explorer and a verification service. Hashscan is only a block explorer. The verification service that we use is based on Sourcify and is provided by a different service.[*]

Here what's important is that slither-read-storage does not support Hedera networks. slither-read-storage seems to only support Etherscan-like services. I don't think the problem comes from eth_getStorageAt, I think the problem comes from not having the source code given the error ERROR:SlitherSolcParsing:crytic-compile returned an empty AST. If you are trying to analyze a contract from etherscan or similar make sure it has source code available.. From the slither-read-storage docs https://github.com/crytic/slither/blob/master/slither/tools/read_storage/README.md

Limitations

  • Requires source code.

[*] Hashscan could in principle provide a Etherscan-like API given that it has the block information and can act as a facade for the verification service. This might be worth exploring in the future.

arianejasuwienas commented 1 month ago

Replaced with: https://github.com/hashgraph/hedera-smart-contracts/pull/740