Open code423n4 opened 1 year ago
Good QA report, I can see why it was selected for the audit report.
I'll leave my usual note on versions that I've left in other issues:
Since it's been noted in several QA reports, I wanted to comment on the use of assert()
as well. Newer OZ versions have removed it, but the code that uses assert()
in our case is similar to: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.2/contracts/proxy/TransparentUpgradeableProxy.sol#L34
Its purpose is to validate at deployment time that the admin/implementation slot has been computed correctly (or, alternatively, that the EVM is computing hashes as expected 😅) and I believe is a legitimate use of the expression.
L01 - should be informational only
I do not think this type of opinion should be presented as fact by wardens.
For this reason, every project in the Ethereum network must have a blacklist function, this is a good method to avoid legal problems in the future, apart from the current need.
Noting that for the audit report, since the judge downgraded L-01 to informational, we have renumbered the issues slightly, with L-01 changed to N-17. I am also omitting L-03 from the report as the risks and recommendations are outlined in issue #149 .
As such, the Low risk issues are renumbered for the report as follows:
Non-Critical Issues List
return parameters
in NatSpec comments0
address checkRequire
revert cause should be knownrequire
instead ofassert
Function writing
that does not comply with theSolidity Style Guide
Total 16 issues
Low Risk Issues List
inherited state variables
Total 5 issues
[N-01] Testing all functions is best practice
Description: The test coverage rate of 91%. Testing all functions is best practice in terms of security criteria.
[N-02] Include
return parameters
in NatSpec commentsContext: IRewardsManager.sol IStaking.sol IGraphToken.sol GraphTokenGateway.sol#L54 IGraphProxy.sol IEpochManager.sol IController.sol
Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
If Return parameters are declared, you must prefix them with "/// @return".
Some code analysis programs do analysis by reading NatSpec details, if they can't see the "@return" tag, they do incomplete analysis.
Recommendation: Include return parameters in NatSpec comments
Recommendation Code Style:
[N-03]
0 address
checkContext: BridgeEscrow.sol#L21 L2GraphToken.sol#L81 GraphProxyAdmin.sol#L69 GraphProxyStorage.sol#L80 GraphProxy.sol#L116 GraphTokenUpgradeable.sol#L133
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 (_treasury == address(0)) revert ADDRESS_ZERO();
[N-04] Use a more recent version of Solidity
Context: All contracts
Recommendation: Old version of Solidity is used
0.7.6
, newer version can be used0.8.17
[N-05] Require revert cause should be known
*Context:** GraphProxy.sol#L133 GraphProxyAdmin.sol#L34 GraphProxyAdmin.sol#L47 GraphProxyAdmin.sol#L59
Description: Vulnerability related to description is not written to require, it must be written so that the user knows why the process is reverted.
Recommendation: Current Code:
Recommendation Code:
[N-06] Use
require
instead ofassert
Context: GraphProxy.sol#L47-L54
Description: Assert should not be used except for tests,
require
should be usedPrior to Solidity 0.8.0, pressing a confirm consumes the remainder of the process's available gas instead of returning it, as request()/revert() did.
Assertion() should be avoided even after solidity version 0.8.0, because its documentation states "The Assert function generates an error of type Panic(uint256). Code that works properly should never Panic, even on invalid external input. If this happens, you need to fix it in your contract. there's a mistake".
[N-07] For modern and more readable code; update import usages
Context: ICuration.sol, IGraphCurationToken.sol, BridgeEscrow.sol#L5-L7, L1GraphTokenGateway.sol#L6-L10, Managed.sol#L5-L12, L2GraphTokenGateway.sol#L6-L13, GraphTokenUpgradeable.sol#L5-L10, L2GraphToken.sol#L5-L8, IStaking.sol#L6, IGraphToken.sol#L5, GraphProxy.sol#L5-L7, GraphProxyAdmin.sol#L5-L8, GraphUpgradeable.sol#L5
Description: Our 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";
[N-08]
Function writing
that does not comply with theSolidity Style Guide
Context: GraphUpgradeable.sol, GraphProxyAdmin.sol, GraphProxy.sol, Managed.sol, L2GraphTokenGateway.sol, L1GraphTokenGateway.sol, GraphTokenGateway.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-09] Implement some type of version counter that will be incremented automatically for contract upgrades
GraphProxyAdmin.sol#L77 GraphProxy.sol#L115
Description: As part of the upgradeability of Proxies , the contract can be upgraded multiple times, where it is a systematic approach to record the version of each upgrade.
I suggest implementing some kind of version counter that will be incremented automatically when you upgrade the contract.
Recommendation:
[N-10] NatSpec is Missing
NatSpec is missing for the following function:
Managed.sol#L43 Managed.sol#L48 Managed.sol#L52 Managed.sol#L56
[N-11] 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;; L1GraphTokenGateway.sol#L114, L1GraphTokenGateway.sol#L124, L1GraphTokenGateway.sol#L134, L1GraphTokenGateway.sol#L134, L1GraphTokenGateway.sol#L134, L1GraphTokenGateway.sol#L134, L1GraphTokenGateway.sol#L134, L2GraphTokenGateway.sol#L100, L2GraphTokenGateway.sol#L110, L2GraphTokenGateway.sol#L120, Managed.sol#L106
[N-12] Constant values such as a call to keccak256(), should used to immutable rather than constant
There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts.
While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand.
Constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
There are 5 instances of this issue:
[N-13 ] Floating pragma
Description: Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103
Floating Pragma List: pragma ^0.7.6. (all contracts)
Recommendation: Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.
[N-14 ] Inconsistent Solidity Versions
Description: Different Solidity compiler versions are used throughout Src repositories. The following contracts mix versions:
Recommendation: Versions must be consistent with each other.
[N-15 ] For extended "using-for" usage, use the latest pragma version
Description: https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/
Recommendation: Use solidity pragma version min. 0.8.13
[N-16 ] For functions, follow Solidity standard naming conventions
Context: L2GraphTokenGateway.sol#L286 L1GraphTokenGateway.sol#L290
Description: The above codes don't follow Solidity's standard naming convention,
internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)
public and external functions : only mixedCase
constant variable : UPPER_CASE_WITH_UNDERSCORES (separated by uppercase and underscore)
[L-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 In addition, these platforms even ban accounts that have received ETH on their account with Tornadocash.Some of these Projects; USDC (https://www.circle.com/en/usdc) Flashbots (https://www.paradigm.xyz/portfolio/flashbots ) Aave (https://aave.com/) Uniswap Balancer Infura Alchemy Opensea dYdX
Details on the subject; https://twitter.com/bantg/status/1556712790894706688?s=20&t=HUTDTeLikUr6Dv9JdMF7AA
https://cryptopotato.com/defi-protocol-aave-bans-justin-sun-after-he-randomly-received-0-1-eth-from-tornado-cash/
For this reason, every project in the Ethereum network must have a blacklist function, this is a good method to avoid legal problems in the future, apart from the current need.
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.
[L-02] Using Vulnerable Version of Openzeppelin
Context: package.json
Description: The package.json configuration file says that the project is using 3.4.1 – 3.4.2 of OpenZeppelin which has a vulnerability in initializers that call external contracts, There is 1 instance of this issue
Recommendation: Use patched versions
Proof Of Concept: https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-9c22-pwxw-p6hx
[L-03] A protected initializer function that can be called at most once must be defined
Context: BridgeEscrow.sol#L20 L1GraphTokenGateway.sol#L99 L2GraphTokenGateway.sol#L87 L2GraphToken.sol#L48
Description: A protected initializer function that can be called at most once must be defined. Admin should be prevented from calling the restartize function, even if it is incorrect
Recommendation: OpenZeppelin recommends that the initializer modifier be applied to constructors https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
[L-04] Avoid shadowing
inherited state variables
Context: GraphProxy.sol#L140
Description: In
GraphProxy.sol#L140
, there is a local variable named_pendingImplementation
, but there is a function named_pendingImplementation()
in the inheritedGraphProxyStorage
with the same name. This use causes compilers to issue warnings, negatively affecting checking and code readability.Recommendation: Avoid using variables with the same name, including inherited in the same contract, if used, it must be specified in the NatSpec comments.
[L-05] NatSpec comments should be increased in contracts
Context: Governed.sol, Managed.sol, GraphTokenGateway.sol, IGraphCurationToken.sol, IGraphProxy.sol, IEpochManager.sol, IController.sol, IGraphToken.sol, IRewardsManager.sol, IStakingData.sol, ICuration.sol, IStaking.sol
Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
Recommendation: NatSpec comments should be increased in Contracts