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

Slither static code analysis #20

Open
CjHare opened this issue Sep 14, 2021 · 1 comment
Open

Slither static code analysis #20

CjHare opened this issue Sep 14, 2021 · 1 comment
Labels
enhancement New feature or request

Comments

@CjHare
Copy link
Contributor

CjHare commented Sep 14, 2021

Result of running the Slither analysis against Solidity 0.7.6.

42 result(s) found

ethsec@8f68a22109dd:/home/bitdao$ slither .
'npx hardhat compile --force' running
Compiling 1 file with 0.7.6
Generating typings for: 8 artifacts in dir: typechain for target: ethers-v5
Successfully generated 9 typings!
Compilation finished successfully

contracts/BitDAO.sol:512:5: Warning: Visibility for constructor is ignored. If you want the contract to be non-deployable, making it "abstract" is sufficient.
    constructor (string memory name_, string memory symbol_) public {
    ^ (Relevant source part starts here and spans across multiple lines).



BitDAO._writeCheckpoint(address,uint256,uint256,uint256) (contracts/BitDAO.sol#1042-1058) uses a dangerous strict equality:
	- nCheckpoints > 0 && checkpoints[delegatee][nCheckpoints - 1].fromBlock == blockNumber (contracts/BitDAO.sol#1050)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities

BitDAO.constructor(address)._admin (contracts/BitDAO.sol#831) lacks a zero-check on :
		- admin = _admin (contracts/BitDAO.sol#832)
BitDAO.setPendingAdmin(address).newPendingAdmin (contracts/BitDAO.sol#836) lacks a zero-check on :
		- pendingAdmin = newPendingAdmin (contracts/BitDAO.sol#841)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-zero-address-validation

BitDAO.delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32) (contracts/BitDAO.sol#945-962) uses timestamp for comparisons
	Dangerous comparisons:
	- require(bool,string)(block.timestamp <= expiry,BitDAO::delegateBySig: signature expired) (contracts/BitDAO.sol#960)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp

BitDAO.getChainId() (contracts/BitDAO.sol#1065-1071) uses assembly
	- INLINE ASM (contracts/BitDAO.sol#1067-1069)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage

BitDAO._beforeTokenTransfer(address,address,uint256) (contracts/BitDAO.sol#883-902) is never used and should be removed
BitDAO._lastSnapshotId(uint256[]) (contracts/BitDAO.sol#933-939) is never used and should be removed
BitDAO._transfer(address,address,uint256) (contracts/BitDAO.sol#1011-1018) is never used and should be removed
BitDAO._updateAccountSnapshot(address) (contracts/BitDAO.sol#917-919) is never used and should be removed
BitDAO._updateSnapshot(BitDAO.Snapshots,uint256) (contracts/BitDAO.sol#925-931) is never used and should be removed
BitDAO._updateTotalSupplySnapshot() (contracts/BitDAO.sol#921-923) is never used and should be removed
Context._msgData() (contracts/BitDAO.sol#370-373) is never used and should be removed
Counters.decrement(Counters.Counter) (contracts/BitDAO.sol#344-346) is never used and should be removed
ERC20._burn(address,uint256) (contracts/BitDAO.sol#706-714) is never used and should be removed
ERC20._setupDecimals(uint8) (contracts/BitDAO.sol#744-746) is never used and should be removed
Math.max(uint256,uint256) (contracts/BitDAO.sol#235-237) is never used and should be removed
Math.min(uint256,uint256) (contracts/BitDAO.sol#242-244) is never used and should be removed
SafeMath.div(uint256,uint256) (contracts/BitDAO.sol#141-144) is never used and should be removed
SafeMath.div(uint256,uint256,string) (contracts/BitDAO.sol#196-199) is never used and should be removed
SafeMath.mod(uint256,uint256) (contracts/BitDAO.sol#158-161) is never used and should be removed
SafeMath.mod(uint256,uint256,string) (contracts/BitDAO.sol#216-219) is never used and should be removed
SafeMath.mul(uint256,uint256) (contracts/BitDAO.sol#122-127) is never used and should be removed
SafeMath.tryAdd(uint256,uint256) (contracts/BitDAO.sol#30-34) is never used and should be removed
SafeMath.tryDiv(uint256,uint256) (contracts/BitDAO.sol#66-69) is never used and should be removed
SafeMath.tryMod(uint256,uint256) (contracts/BitDAO.sol#76-79) is never used and should be removed
SafeMath.tryMul(uint256,uint256) (contracts/BitDAO.sol#51-59) is never used and should be removed
SafeMath.trySub(uint256,uint256) (contracts/BitDAO.sol#41-44) is never used and should be removed
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dead-code

Pragma version>=0.6.5<0.8.0 (contracts/BitDAO.sol#768) is too complex
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity

Variable BitDAO.MAX_SUPPLY (contracts/BitDAO.sol#780) is not in mixedCase
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions

Redundant expression "this (contracts/BitDAO.sol#371)" inContext (contracts/BitDAO.sol#365-374)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#redundant-statements

BitDAO.MAX_SUPPLY (contracts/BitDAO.sol#780) should be constant
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#state-variables-that-could-be-declared-constant

symbol() should be declared external:
	- ERC20.symbol() (contracts/BitDAO.sol#529-531)
decimals() should be declared external:
	- ERC20.decimals() (contracts/BitDAO.sol#546-548)
transfer(address,uint256) should be declared external:
	- ERC20.transfer(address,uint256) (contracts/BitDAO.sol#572-575)
allowance(address,address) should be declared external:
	- ERC20.allowance(address,address) (contracts/BitDAO.sol#580-582)
approve(address,uint256) should be declared external:
	- ERC20.approve(address,uint256) (contracts/BitDAO.sol#591-594)
transferFrom(address,address,uint256) should be declared external:
	- ERC20.transferFrom(address,address,uint256) (contracts/BitDAO.sol#609-613)
increaseAllowance(address,uint256) should be declared external:
	- ERC20.increaseAllowance(address,uint256) (contracts/BitDAO.sol#627-630)
decreaseAllowance(address,uint256) should be declared external:
	- ERC20.decreaseAllowance(address,uint256) (contracts/BitDAO.sol#646-649)
balanceOfAt(address,uint256) should be declared external:
	- BitDAO.balanceOfAt(address,uint256) (contracts/BitDAO.sol#871-875)
totalSupplyAt(uint256) should be declared external:
	- BitDAO.totalSupplyAt(uint256) (contracts/BitDAO.sol#877-881)
getPriorVotes(address,uint256) should be declared external:
	- BitDAO.getPriorVotes(address,uint256) (contracts/BitDAO.sol#969-999)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external
. analyzed (8 contracts with 75 detectors), 42 result(s) found
@CjHare CjHare added the enhancement New feature or request label Sep 14, 2021
@CjHare
Copy link
Contributor Author

CjHare commented Sep 14, 2021

Better organisation of the findings in their categories, mainly low & optimisation issues.

ethsec@8f68a22109dd:/home/bitdao$ slither contracts/BitDAO.sol --print human-summary
Compilation warnings/errors on contracts/BitDAO.sol:
Warning: Visibility for constructor is ignored. If you want the contract to be non-deployable, making it "abstract" is sufficient.
   --> contracts/BitDAO.sol:512:5:
    |
512 |     constructor (string memory name_, string memory symbol_) public {
    |     ^ (Relevant source part starts here and spans across multiple lines).



Compiled with solc
Number of lines: 1072 (+ 0 in dependencies, + 0 in tests)
Number of assembly lines: 0
Number of contracts: 8 (+ 0 in dependencies, + 0 tests)

Number of optimization issues: 12
Number of informational issues: 26
Number of low issues: 3
Number of medium issues: 1
Number of high issues: 0

ERCs: ERC20

+----------+-------------+-------+--------------------+--------------+-----------+
|   Name   | # functions |  ERCS |     ERC20 info     | Complex code |  Features |
+----------+-------------+-------+--------------------+--------------+-----------+
| SafeMath |      13     |       |                    |      No      |           |
|   Math   |      3      |       |                    |      No      |           |
|  Arrays  |      1      |       |                    |     Yes      |           |
| Counters |      3      |       |                    |      No      |           |
|  BitDAO  |      50     | ERC20 |     No Minting     |     Yes      | Ecrecover |
|          |             |       | Approve Race Cond. |              |  Assembly |
|          |             |       |                    |              |           |
+----------+-------------+-------+--------------------+--------------+-----------+
contracts/BitDAO.sol analyzed (8 contracts)

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

No branches or pull requests

1 participant