feat: Setup Slither

[gpylypchuk]

Resolves #3 issue.

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[gpylypchuk]

I'm ignoring the Slither warning because the actual contracts of OpenZeppelin are with pragma solidity ^0.8.20

[rndquu]

Pls add tests for all newly added require() statements

[gpylypchuk]

should I add the merged changes to this PR? (about #4)

[rndquu]

should I add the merged changes to this PR? (about #4)

Yes, you should merge the development branch because there are code conflicts which should be resolved

[gpylypchuk]

Done @rndquu !

[rndquu]

1. There is still this warning:

NftReward.safeMint(NftReward.MintRequest,bytes) (src/NftReward.sol#145-176) uses timestamp for comparisons
	Dangerous comparisons:
	- require(bool,string)(block.timestamp < _mintRequest.deadline,Signature expired) (src/NftReward.sol#155)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp

I think we can simply exclude the block-timestamp detector.

2. There is still this warning:

Pragma version^0.8.20 (src/NftReward.sol#2) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
solc-0.8.24 is not recommended for deployment
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity

What if we use solidity v0.8.18?

3. There is still this info warning:

Parameter NftReward.invalidateNonce(uint256)._nonceValue (src/NftReward.sol#223) is not in mixedCase
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions

Either exclude this detector either refactor the code.

4. Pls make sure the slither check workflow fails on any warning in the NftReward.sol contract

5. Pls update the slither worflow to run only when *.sol files are changed

[gpylypchuk]

1. There is still this warning:

NftReward.safeMint(NftReward.MintRequest,bytes) (src/NftReward.sol#145-176) uses timestamp for comparisons
	Dangerous comparisons:
	- require(bool,string)(block.timestamp < _mintRequest.deadline,Signature expired) (src/NftReward.sol#155)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp

I think we can simply exclude the block-timestamp detector.

2. There is still this warning:

Pragma version^0.8.20 (src/NftReward.sol#2) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
solc-0.8.24 is not recommended for deployment
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity

What if we use solidity v0.8.18?

3. There is still this info warning:

Parameter NftReward.invalidateNonce(uint256)._nonceValue (src/NftReward.sol#223) is not in mixedCase
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions

Either exclude this detector either refactor the code.

4. Pls make sure the slither check workflow fails on any warning in the NftReward.sol contract
5. Pls update the slither worflow to run only when *.sol files are changed

In 2, I tested before and there is still the warning because of the OZ libraries

[rndquu]

In 2, I tested before and there is still the warning because of the OZ libraries

But there is no warning for NftReward.sol in this case, right?

[gpylypchuk]

In 2, I tested before and there is still the warning because of the OZ libraries

But there is no warning for NftReward.sol in this case, right?

correct!

[rndquu]

In 2, I tested before and there is still the warning because of the OZ libraries

But there is no warning for NftReward.sol in this case, right?

correct!

Then it makes sense to update NftReward.sol to use solidity v0.8.18 since OZ contracts are not part of this project

[gpylypchuk]

Should we fail on medium or low? Also I think we should Ignore the OZ libraries 🤔

[rndquu]

Should we fail on medium or low? Also I think we should Ignore the OZ libraries 🤔

Yes, we use this strategy (fall on any slither warning or compiler warning) in our main repo https://github.com/ubiquity/ubiquity-dollar/

Not all of the slither detectors are included in our main repo because it makes sense to be reasonable (for example, block.timestamp related warnings can be excluded)

[gpylypchuk]

Should we fail on medium or low? Also I think we should Ignore the OZ libraries 🤔

Yes, we use this strategy (fall on any slither warning or compiler warning) in our main repo https://github.com/ubiquity/ubiquity-dollar/

Not all of the slither detectors are included in our main repo because it makes sense to be reasonable (for example, block.timestamp related warnings can be excluded)

yep, I added a detector ignore about OZ libraries & timestamp

[gpylypchuk]

@rndquu should I add this: "exclude_informational": true to ignore this solc-0.8.24 is not recommended for deployment Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity ?

[rndquu]

@rndquu should I add this: "exclude_informational": true to ignore this solc-0.8.24 is not recommended for deployment Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity ?

If we remove the exclude_informational config the slither workflow still passes, right?

[gpylypchuk]

@rndquu should I add this: "exclude_informational": true to ignore this solc-0.8.24 is not recommended for deployment Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity ?

If we remove the exclude_informational config the slither workflow still passes, right?

No, I think that is because is like a low vuln.
In this commit I removed the exclude_informational and the log show that failed because of that.

[rndquu]

@rndquu should I add this: "exclude_informational": true to ignore this solc-0.8.24 is not recommended for deployment Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity ?

If we remove the exclude_informational config the slither workflow still passes, right?

No, I think that is because is like a low vuln. In this commit I removed the exclude_informational and the log show that failed because of that.

I don't understand from the log output, what file causes the solc-0.8.24 is not recommended for deployment error?

[gpylypchuk]

exclude_informational

That is because slither only recommends deployment with the version 0.8.18 and the OZ contracts are in ^0.8.20 so there is no way to not recognize that error.

[gpylypchuk]

I just added a line in foundry.toml about solc_version = "0.8.20" so you can select the version you want to compile.

[rndquu]

I just added a line in foundry.toml about solc_version = "0.8.20" so you can select the version you want to compile.

But that doesn't solve the warning as far as I understand

Overall we shouldn't exclude all low level issues. This is getting out of hand so lets:

1. Remove exclude_informational: true
2. Add the solc-version detector to exceptions. The only valid case I can think of is the usage of the PUSH0 instruction which is mainly supported.

[gpylypchuk]

I just added a line in foundry.toml about solc_version = "0.8.20" so you can select the version you want to compile.

But that doesn't solve the warning as far as I understand

Overall we shouldn't exclude all low level issues. This is getting out of hand so lets:

1. Remove exclude_informational: true
2. Add the solc-version detector to exceptions. The only valid case I can think of is the usage of the PUSH0 instruction which is mainly supported.

done!

[gitcoindev]

Reviewed, read all comments and approving from my side, too!