QA Report #220
Labels
bug
Something isn't working
grade-a
low quality report
This report is of especially low quality
Q-46
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Non-Critical Issues List
0
address checkFunction writing
that does not comply with theSolidity Style Guide
immutable
Total 8 issues
Low Risk Issues List
safeTransferOwnership
instead oftransferOwnership
functionTotal 2 issues
Suggestions
Total 1 suggestion
[N-01] Insufficient coverage
Description:
The test coverage rate of the project is 30%. Testing all functions is best practice in terms of security criteria.
Due to its capacity, test coverage is expected to be 100%.
[N-02]
0 address
checkContext:
JBTiered721DelegateDeployer.sol#L51-L53
JBTiered721DelegateProjectDeployer.sol#L52-L53
JB721Delegate.sol#L212
JBTiered721Delegate.sol#L223-L224
Description:
Also check of the address to protect the code from 0x0 address problem just in case. This is best practice or instead of suggesting that they verify address != 0x0, you could add some good NatSpec comments explaining what is valid and what is invalid and what are the implications of accidentally using an invalid address.
Recommendation:
like this;
if (routerV2== address(0)) revert ADDRESS_ZERO();
[N-03] For modern and more readable code; update import usages
Context:
All contracts
Description:
Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct
polluted the source code
with an unnecessary object we were not using because we did not need it.This was breaking the rule of modularity and modular programming:
only import what you need
Specific imports with curly braces allow us to apply this rule better.Recommendation:
import {contract1 , contract2} from "filename.sol";
A good example from the ArtGobblers project;
[N-04]
Function writing
that does not comply with theSolidity Style Guide
Context:
JBTiered721Delegate.sol
JBTiered721DelegateStore.sol
JB721Delegate.sol
Description:
Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
https://docs.soliditylang.org/en/v0.8.17/style-guide.html
Functions should be grouped according to their visibility and ordered:
[N-05] State variables should be marked
immutable
Context:
JB721Delegate.sol#L48-LL62
Recommendation:
The
projectId
anddirectory
state variables should be changed toimmutable
, also adhering to the Natspec comments.[N-06] Omissions in Events
Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters
The events should include the new value and old value where possible:
Events with no old value;;
JBTiered721Delegate.sol#L374
JBTiered721Delegate.sol#L390
JBTiered721Delegate.sol#L406
JBTiered721Delegate.sol#L422
[N-07] Need Fuzzing test
Context:
22 results - 4 files
Project uncheckeds list:
Description:
In total 4 contracts, 22 unchecked are used, the functions used are critical. For this reason, there must be fuzzing tests in the tests of the project. Not seen in tests.
Recommendation:
Use should fuzzing test like Echidna.
As Alberto Cuesta Canada said:
Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.
https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05
[N-08] Use a more recent version of Solidity
Context:
All contracts
Description:
For security, it is best practice to use the latest Solidity version.
For the security fix list in the versions;
https://github.com/ethereum/solidity/blob/develop/Changelog.md
Recommendation:
Old version of Solidity is used
(^0.8.16)
, newer version can be used(0.8.17)
[L-01] Use
safeTransferOwnership
instead oftransferOwnership
functionContext:
JBTiered721Delegate.sol#L251
JBTiered721DelegateDeployer.sol#L100
Description:
transferOwnership
function is used to change OwnershipUse a 2 structure transferOwnership which is safer.
safeTransferOwnership
, use it is more secure due to 2-stage ownership transfer.Recommendation:
Use
Ownable2Step.sol
Ownable2Step.sol
[L-02] Use a more recent version of OpenZeppelin dependencies
Context:
All contracts
Description:
For security, it is best practice to use the latest OZ version.
package.json#L14
For the security fix list in the versions;
https://github.com/ethereum/solidity/blob/develop/Changelog.md
Recommendation:
Old version of OZ is used
(4.6.0)
, newer version can be used(4.7.3)
https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.7.3
[S-01] Add to blacklist function
Description:
Cryptocurrency mixing service, Tornado Cash, has been blacklisted in the OFAC.
A lot of blockchain companies, token projects, NFT Projects have
blacklisted
all Ethereum addresses owned by Tornado Cash listed in the US Treasury Department's sanction against the protocol.https://home.treasury.gov/policy-issues/financial-sanctions/recent-actions/20220808
Some of these Projects;
USDC
Aave
Uniswap
Balancer
Infura
Alchemy
Opensea
dYdX
Transactions from the project by an account funded by Tonadocash or banned by OFAC can lead to legal problems.Especially American citizens may want to get addresses to the blacklist legally, this is not an obligation
If you think that such legal prohibitions should be made directly by validators, this may not be possible:
https://www.paradigm.xyz/2022/09/base-layer-neutrality
The ban on Tornado Cash makes little sense, because in the end, no one can prevent people from using other mixer smart contracts, or forking the existing ones. It neither hinders cybercrime, nor privacy.
Here is the most beautiful and close to the project example; Manifold
Manifold Contract
https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code
Recommended Mitigation Steps add to Blacklist function and modifier.
The text was updated successfully, but these errors were encountered: