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

feat: add detector for unused custom errors #2565

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from

Conversation

gvladika
Copy link

@gvladika gvladika commented Sep 25, 2024

New detector looks for Solidity custom errors which are declared, but never used. This can indicate a mistake, ie. error is declared but check which would invoke the error was never put in place.

It can be executed like this:

slither .  --detect unused-custom-errors

Output will list identified unused errors and the respective file names. Ie.

The following unused error(s) should be removed:
        -UnusedLibError() (tests/e2e/detectors/test_data/unused-custom-errors/0.8.15/CustomErrorsLib.sol)

        -UnusedErrorLibConsumerErr() (tests/e2e/detectors/test_data/unused-custom-errors/0.8.15/ErrorLibConsumer.sol)

        -UnusedParentErrWithArg(uint256) (tests/e2e/detectors/test_data/unused-custom-errors/0.8.15/ContractToTestForCustomErrors.sol)

        -UnusedParentErr() (tests/e2e/detectors/test_data/unused-custom-errors/0.8.15/ContractToTestForCustomErrors.sol)

        -UnusedTopLevelErr() (tests/e2e/detectors/test_data/unused-custom-errors/0.8.15/ContractToTestForCustomErrors.sol)

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2024

CLA assistant check
All committers have signed the CLA.

# Find unused custom errors
for declared_error in declared_custom_errors:
if not any(
declared_error.name in custom_revert.name for custom_revert in custom_reverts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking the name could have false negative. For example try adding this in the contract test. It won't get detected.

contract C {
    error UsedParentErr(address x);
}

A better approach could be to have custom_errors with the actual CustomError instance instead of the name and then check here with declared_error == custom_revert. When you are adding the custom error you can get the instance with internal_call.function.custom_error

if isinstance(custom_error, CustomErrorTopLevel)
else custom_error.contract.file_scope
)
info += ["\n\t-", custom_error.full_name, " (", file_scope.filename.short, ")\n"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just use custom_error and in the output you will have automatically its filename. Also it would be better to have individual findings for each custom error instead of one with everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants