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

Invalid handling of try-catch code #982

Closed
pdyraga opened this issue Nov 30, 2021 · 6 comments
Closed

Invalid handling of try-catch code #982

pdyraga opened this issue Nov 30, 2021 · 6 comments
Labels
bug Something isn't working High Priority

Comments

@pdyraga
Copy link

pdyraga commented Nov 30, 2021

$ slither --version
0.8.0

Please consider the following code:

        try
            self.dkgValidator.validate(result, self.seed, self.startBlock)
        returns (bool isValid, string memory errorMsg) {
            if (isValid) {
                revert("unjustified challenge");
            }

            emit DkgResultChallenged(
                self.submittedResultHash,
                msg.sender,
                errorMsg
            );
        } catch {
            // if the validation reverted we consider the DKG result as invalid
            emit DkgResultChallenged(
                self.submittedResultHash,
                msg.sender,
                "validation reverted"
            );
        }

Slither says:

INFO:Detectors:
DKG.challengeResult(DKG.Data,DKG.Result).isValid (contracts/libraries/DKG.sol#408) is a local variable never initialized
DKG.challengeResult(DKG.Data,DKG.Result).errorMsg (contracts/libraries/DKG.sol#408) is a local variable never initialized
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-local-variables
INFO:Detectors:
DKG.challengeResult(DKG.Data,DKG.Result) (contracts/libraries/DKG.sol#383-441) ignores return value by self.dkgValidator.validate(result,self.seed,self.startBlock) (contracts/libraries/DKG.sol#404-425)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return
INFO:Detectors:
Variable 'DKG.challengeResult(DKG.Data,DKG.Result).isValid (contracts/libraries/DKG.sol#408)' in DKG.challengeResult(DKG.Data,DKG.Result) (contracts/libraries/DKG.sol#383-441) potentially used before declaration: isValid (contracts/libraries/DKG.sol#409)
Variable 'DKG.challengeResult(DKG.Data,DKG.Result).errorMsg (contracts/libraries/DKG.sol#408)' in DKG.challengeResult(DKG.Data,DKG.Result) (contracts/libraries/DKG.sol#383-441) potentially used before declaration: DkgResultChallenged(self.submittedResultHash,msg.sender,errorMsg) (contracts/libraries/DKG.sol#413-417)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#pre-declaration-usage-of-local-variables

This is a pretty standard try-catch block from Solidity.

pdyraga added a commit to keep-network/keep-core that referenced this issue Nov 30, 2021
It looks that slither does not handle Solidity try-catch properly.
Issue reported: crytic/slither#982
pdyraga added a commit to keep-network/keep-core that referenced this issue Nov 30, 2021
It looks that slither does not handle Solidity try-catch properly.
Issue reported: crytic/slither#982
@frangio
Copy link

frangio commented Jun 8, 2022

Ran into this as well. The try expression triggers an "unused return value" warning, even though it is assigned to a variable and used. Another reproduction:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.2;

interface Foo {
    function foo() external returns (uint);
}

contract Test {
    function test(Foo a) external returns (uint y) {
        try a.foo() returns (uint x) {
            y = x;
        } catch {}
    }
}
C.test(IC).x (contracts/test.sol#10) is a local variable never initialized
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-local-variables

C.test(IC) (contracts/test.sol#9-13) ignores return value by a.foo() (contracts/test.sol#10-12)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return

@ryanc414
Copy link

Got this same issue from using the Openzeppelin ERC721 library contract. It seems odd that slither is reporting errors in my dependencies - is this intentional? Is there any way to configure it to only check the primary sources and not deps?

@pbuda
Copy link

pbuda commented Sep 15, 2022

Got this same issue from using the Openzeppelin ERC721 library contract. It seems odd that slither is reporting errors in my dependencies - is this intentional? Is there any way to configure it to only check the primary sources and not deps?

You can use the filter_paths parameter to filter out openzeppelin contracts. I use json config file, and it's as simple as adding "filter_paths": "openzeppelin" to it.

@ryanc414
Copy link

Got this same issue from using the Openzeppelin ERC721 library contract. It seems odd that slither is reporting errors in my dependencies - is this intentional? Is there any way to configure it to only check the primary sources and not deps?

You can use the filter_paths parameter to filter out openzeppelin contracts. I use json config file, and it's as simple as adding "filter_paths": "openzeppelin" to it.

Yep, I figured that out thanks. Seems like a strange design choice to include dependencies by default though.

@0xalpharush 0xalpharush added the bug Something isn't working label Oct 6, 2022
@thedavidmeister
Copy link

seems related or same as #511

@QEDK
Copy link

QEDK commented Apr 20, 2023

Bumping this, we're facing the same issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working High Priority
Projects
None yet
Development

No branches or pull requests

7 participants