search

ubiquity / nft-rewards

Set of contracts that allow minter to sign off-chain mint requests for target users who can later mint NFTs using the minter's signature
0 stars 4 forks source link

Setup `slither` #3

Closed rndquu closed 8 months ago

rndquu commented 11 months ago

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)
ubiquibot[bot] commented 10 months ago 
! action has an uncaught error
ubiquibot[bot] commented 10 months ago 
! action has an uncaught error
gpylypchuk commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

But the thing is all warnings come from the openzeppelin libraries

Could you show slither's output?

gpylypchuk commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

/start

ubiquibot[bot] commented 8 months ago

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:
ubiquibot[bot] commented 8 months ago 
! action has an uncaught error
gpylypchuk commented 8 months ago

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

rndquu commented 8 months ago

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

Hard to say, @pavlovcik might know better

ubiquibot[bot] commented 8 months ago 
+ Evaluating results. Please wait...
0x4007 commented 8 months ago

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