Open code423n4 opened 2 years ago
A couple invalid points on this report
L5: OZ create2 reverts on failed deployment already https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Create2.sol#L42
L3 checking for contract existence would increase gas costs. Tokens are only white listed if they actually exist.
QA (LOW & NON-CRITICAL)
[L-01] Use of floating pragma
Floating Pragma used in
wfCashERC4626.sol
. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) 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. Reference[L-02] Different and outdated compiler versions
The scoped contracts have different solidity compiler ranges (0.6.10 - 0.8.11) referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior. Current Solidity version available is 0.8.14
[L-03] Missing checks for contract existence
OpenZeppelin's
safeTransfer
andsafeTransferFrom
functions usefunctionCall
of Address.sol which is a low level call. However, it's stated that the target must be a contract and code existence should be checked prior using. Reference It's advised to check code existence prior calling safeTransfer or safeTransferFrom as below; In wfCashLogic.sol,[L-04] Missing of zero address checks for the immutables
No
address(0)
or Zero value check at the constructors of: In WrappedfCashFactory.sol;In wfCashBase.sol;
In wfCashERC4626.sol;
In wfCashLogic.sol;
In NotionalTradeModule.sol;
[L-05] Missing address comparison after Create2 function
WrappedfCashFactory uses Solidity's Create2 function to deploy the wrappers. In case of a deployment failure it will revert as zero address and will throw an error. However, a good practice is to compare the
_computedWrapper
address with the deployedwrapper
same as in Solidity's official page here[L-06] Unbounded loop on array can lead to DoS
Programming patterns such as looping over arrays of unknown size may lead to DoS when the gas cost of execution exceeds the block gas limit. Reference Instances in NotionalTradeModule.sol;
[L-07] External Calls inside a loop
Calls to external contracts inside a loop are dangerous (especially if the loop index can be user-controlled) because it could lead to DoS if one of the calls reverts or execution runs out of gas. Avoid calls within loops, check that loop index cannot be user-controlled or is bounded.Reference
Instances in NotionalTradeModule.sol;
[L-08] wfCashERC4626 is not as per EIP4626 standard
EIP4626 requires
deposit
andwithdraw
functions having event with indexed parameters forcaller
,owner
andreceiver
. However, this pattern is not followed. Reference[N-01] Some
require
statements don't throw error messageSome require statements in the scoped contracts don't throw error. In case of any error pops up, it will not be possible to know the error source. The list is as below;
[N-02] Scoped contracts are missing proper NatSpec comments
The scoped contracts are missing proper NatSpec comments such as @notice @dev @param on many places.It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI) Reference