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

Setup slither #3

Closed
rndquu opened this issue Dec 7, 2023 · 18 comments · Fixed by #7
Closed

Setup slither #3

rndquu opened this issue Dec 7, 2023 · 18 comments · Fixed by #7

Comments

@rndquu
Copy link
Member

rndquu commented Dec 7, 2023

What should be done:

  1. Fix all slither info warnings produced by slither .
  2. Setup a workflow that runs slither check on any PR and push to the development branch (make sure it fails if there are any warnings, example)
Copy link

ubiquibot bot commented Dec 29, 2023

! action has an uncaught error

Copy link

ubiquibot bot commented Dec 29, 2023

! action has an uncaught error

@gpylypchuk
Copy link
Contributor

What should be done:

  1. Fix all slither info warnings produced by slither .
  2. Setup a workflow that runs slither check on any PR and push to the development branch (make sure it fails if there are any warnings, example)

Hey should I use the --triage-mode? or simply can I comment //slither-disable-next-line [DETECTOR] in the OZ contracts to disable the warnings?

@rndquu
Copy link
Member Author

rndquu commented Feb 8, 2024

What should be done:

  1. Fix all slither info warnings produced by slither .
  2. Setup a workflow that runs slither check on any PR and push to the development branch (make sure it fails if there are any warnings, example)

Hey should I use the --triage-mode? or simply can I comment //slither-disable-next-line [DETECTOR] in the OZ contracts to disable the warnings?

This issue implies refactoring the code in a way that fixes warnings, we shouldn't disable detectors with //slither-disable-next-line [DETECTOR] unless there is a reason

@gpylypchuk
Copy link
Contributor

What should be done:

  1. Fix all slither info warnings produced by slither .
  2. Setup a workflow that runs slither check on any PR and push to the development branch (make sure it fails if there are any warnings, example)

Hey should I use the --triage-mode? or simply can I comment //slither-disable-next-line [DETECTOR] in the OZ contracts to disable the warnings?

This issue implies refactoring the code in a way that fixes warnings, we shouldn't disable detectors with //slither-disable-next-line [DETECTOR] unless there is a reason

But the thing is all warnings come from the openzeppelin libraries, is it correct refactoring audited code only to get all green? is not possible that they are false positives?

@rndquu
Copy link
Member Author

rndquu commented Feb 8, 2024

But the thing is all warnings come from the openzeppelin libraries

Could you show slither's output?

@gpylypchuk
Copy link
Contributor

Here there are the warnings.

Screenshot 2024-02-08 at 12 33 58 Screenshot 2024-02-08 at 12 34 56 Screenshot 2024-02-08 at 12 35 14

@rndquu
Copy link
Member Author

rndquu commented Feb 9, 2024

The slither . command does show some info warnings regarding the NftReward contract (I checked on the latest development branch, not on the current PR):

INFO:Detectors:
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) has bitwise-xor operator ^ instead of the exponentiation operator **: 
	 - inverse = (3 * denominator) ^ 2 (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#184)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-exponentiation
INFO:Detectors:
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- denominator = denominator / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#169)
	- inverse = (3 * denominator) ^ 2 (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#184)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- denominator = denominator / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#169)
	- inverse *= 2 - denominator * inverse (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#188)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- denominator = denominator / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#169)
	- inverse *= 2 - denominator * inverse (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#189)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- denominator = denominator / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#169)
	- inverse *= 2 - denominator * inverse (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#190)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- denominator = denominator / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#169)
	- inverse *= 2 - denominator * inverse (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#191)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- denominator = denominator / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#169)
	- inverse *= 2 - denominator * inverse (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#192)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- denominator = denominator / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#169)
	- inverse *= 2 - denominator * inverse (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#193)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
	- prod0 = prod0 / twos (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#172)
	- result = prod0 * inverse (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#199)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply
INFO:Detectors:
NftReward.constructor(string,string,address,address)._minter (src/NftReward.sol#62) lacks a zero-check on :
		- minter = _minter (src/NftReward.sol#68)
NftReward.setMinter(address)._newMinter (src/NftReward.sol#185) lacks a zero-check on :
		- minter = _newMinter (src/NftReward.sol#186)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-zero-address-validation
INFO:Detectors:
NftReward.safeMint(NftReward.MintRequest,bytes) (src/NftReward.sol#129-160) uses timestamp for comparisons
	Dangerous comparisons:
	- require(bool,string)(block.timestamp < _mintRequest.deadline,Signature expired) (src/NftReward.sol#139)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp
INFO:Detectors:
ERC721._checkOnERC721Received(address,address,uint256,bytes) (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#465-482) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#476-478)
ShortStrings.toString(ShortString) (lib/openzeppelin-contracts/contracts/utils/ShortStrings.sol#63-73) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/ShortStrings.sol#68-71)
StorageSlot.getAddressSlot(bytes32) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#59-64) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#61-63)
StorageSlot.getBooleanSlot(bytes32) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#69-74) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#71-73)
StorageSlot.getBytes32Slot(bytes32) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#79-84) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#81-83)
StorageSlot.getUint256Slot(bytes32) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#89-94) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#91-93)
StorageSlot.getStringSlot(bytes32) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#99-104) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#101-103)
StorageSlot.getStringSlot(string) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#109-114) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#111-113)
StorageSlot.getBytesSlot(bytes32) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#119-124) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#121-123)
StorageSlot.getBytesSlot(bytes) (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#129-134) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#131-133)
Strings.toString(uint256) (lib/openzeppelin-contracts/contracts/utils/Strings.sol#24-44) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/Strings.sol#30-32)
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/Strings.sol#36-38)
ECDSA.tryRecover(bytes32,bytes) (lib/openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol#56-73) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol#64-68)
MessageHashUtils.toEthSignedMessageHash(bytes32) (lib/openzeppelin-contracts/contracts/utils/cryptography/MessageHashUtils.sol#30-37) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/cryptography/MessageHashUtils.sol#32-36)
MessageHashUtils.toTypedDataHash(bytes32,bytes32) (lib/openzeppelin-contracts/contracts/utils/cryptography/MessageHashUtils.sol#76-85) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/cryptography/MessageHashUtils.sol#78-84)
Math.mulDiv(uint256,uint256,uint256) (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#123-202) uses assembly
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#130-133)
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#154-161)
	- INLINE ASM (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#167-176)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage
INFO:Detectors:
Different versions of Solidity are used:
	- Version used: ['^0.8.13', '^0.8.20']
	- ^0.8.13 (src/NftReward.sol#2)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/access/Ownable.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/interfaces/IERC5267.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/interfaces/draft-IERC6093.sol#3)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/IERC721.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/IERC721Receiver.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/extensions/IERC721Metadata.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/Context.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/Pausable.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/ShortStrings.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#5)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/Strings.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/cryptography/EIP712.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/cryptography/MessageHashUtils.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/introspection/IERC165.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#4)
	- ^0.8.20 (lib/openzeppelin-contracts/contracts/utils/math/SignedMath.sol#4)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used
INFO:Detectors:
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/access/Ownable.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/interfaces/IERC5267.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/interfaces/draft-IERC6093.sol#3) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/IERC721.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/IERC721Receiver.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/token/ERC721/extensions/IERC721Metadata.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/Context.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/Pausable.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/ShortStrings.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/StorageSlot.sol#5) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/Strings.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/cryptography/EIP712.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/cryptography/MessageHashUtils.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/introspection/IERC165.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/math/Math.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.20 (lib/openzeppelin-contracts/contracts/utils/math/SignedMath.sol#4) necessitates a version too recent to be trusted. Consider deploying with 0.8.18.
Pragma version^0.8.13 (src/NftReward.sol#2) allows old versions
solc-0.8.23 is not recommended for deployment
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity
INFO:Detectors:
Function EIP712._EIP712Name() (lib/openzeppelin-contracts/contracts/utils/cryptography/EIP712.sol#146-148) is not in mixedCase
Function EIP712._EIP712Version() (lib/openzeppelin-contracts/contracts/utils/cryptography/EIP712.sol#157-159) is not in mixedCase
Parameter NftReward.getMintRequestDigest(NftReward.MintRequest)._mintRequest (src/NftReward.sol#80) is not in mixedCase
Parameter NftReward.recover(NftReward.MintRequest,bytes)._mintRequest (src/NftReward.sol#112) is not in mixedCase
Parameter NftReward.recover(NftReward.MintRequest,bytes)._signature (src/NftReward.sol#113) is not in mixedCase
Parameter NftReward.safeMint(NftReward.MintRequest,bytes)._mintRequest (src/NftReward.sol#130) is not in mixedCase
Parameter NftReward.safeMint(NftReward.MintRequest,bytes)._signature (src/NftReward.sol#131) is not in mixedCase
Parameter NftReward.setBaseUri(string)._newBaseUri (src/NftReward.sol#177) is not in mixedCase
Parameter NftReward.setMinter(address)._newMinter (src/NftReward.sol#185) is not in mixedCase
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions
INFO:Detectors:
ShortStrings.slitherConstructorConstantVariables() (lib/openzeppelin-contracts/contracts/utils/ShortStrings.sol#40-123) uses literals with too many digits:
	- FALLBACK_SENTINEL = 0x00000000000000000000000000000000000000000000000000000000000000FF (lib/openzeppelin-contracts/contracts/utils/ShortStrings.sol#42)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#too-many-digits
INFO:Slither:. analyzed (22 contracts with 93 detectors), 59 result(s) found

@gpylypchuk
Copy link
Contributor

yep, but should we look at NftReward detections only, correct? I do not find useful refactoring audited code. Also the warnings at NftReward are only Low e.g. Blocktimestamp but here is used for a deadline not for randomness.

@gpylypchuk
Copy link
Contributor

gpylypchuk commented Feb 9, 2024

Well, I've been reviewing the code and I only found the cases of:

  1. Is safe adding requires of zero addresses where there are addresses as parameters.
require(initialOwner != address(0), "Initial owner cannot be zero address");
  1. Renaming variables like _initialOwner to initialOwner as mixed Case.

  2. Use the versions as the same used in libraries pragma solidity ^0.8.20;

Then the other warnings come from OpenZeppelin libraries, but I don't find it necessary modifying them.

@rndquu
Copy link
Member Author

rndquu commented Feb 9, 2024

but should we look at NftReward detections only, correct?

Yes

Also the warnings at NftReward are only Low

This issue implied fixing low severity warnings (where it makes sense). Mixed case and block.timestamp detectors can be omitted while zero address checks and using the same solidity version related warnings could be fixed.

@gpylypchuk
Copy link
Contributor

/start

Copy link

ubiquibot bot commented Feb 15, 2024

Warning! This task was created over 70 days ago. Please confirm that this issue specification is accurate before starting.
DeadlineThu, Feb 15, 10:09 PM UTC
Registered Wallet 0x3ac293A770B62F7fECCe918dCC361a594b7f68eA
Tips:
  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

Copy link

ubiquibot bot commented Feb 21, 2024

! action has an uncaught error

@gpylypchuk
Copy link
Contributor

The bot did not respond! This happened because I opened two PR at the same time right?

@rndquu
Copy link
Member Author

rndquu commented Feb 22, 2024

The bot did not respond! This happened because I opened two PR at the same time right?

Hard to say, @pavlovcik might know better

@0x4007 0x4007 reopened this Feb 22, 2024
@0x4007 0x4007 closed this as completed Feb 22, 2024
Copy link

ubiquibot bot commented Feb 22, 2024

+ Evaluating results. Please wait...

@0x4007
Copy link
Member

0x4007 commented Feb 22, 2024

I've never seen this scenario before. But yesterday there was some strange behavior I think on GitHub's side with the pull request links. One of our contributors was unable to automatically link their pull request to the issue via Resolves closing keyword, which has nothing to do with our bot.

+ Evaluating results. Please wait...

There's another issue I've never seen so I'll handle the payout manually.

https://gnosisscan.io/tx/0xb245990569cebe0757c42d8d2ac29d284fc44484df0feea6e521b2b8533fedd5

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

Successfully merging a pull request may close this issue.

3 participants