Biconomy Hyphen 2.0 contest - 0xNazgul's results

Lack of Zero Address Validation in initialize() function

Severity low

Description Although most of the functions throughout the codebase properly validate function inputs, there are some instances of functions that do not. Such as severely initialize() functions that do not check for zero address. They are missing in:

LiquidityFarming.sol initialize() doesn't check if _trustedForwarder, _pauser, lpToken, and _liquidityProviders are zero address. withdraw() doesn't check if _to is a zero address

LiquidityPool.sol withdrawErc20GasFee() doesn't check if tokenAddress is a zero address

LiquidityProviders.sol initialize() doesn't check if _trustedForwarder, _lpToken, _tokenManager and _pauser are zero address.

WhitelistPeriodManager.sol initialize() doesn't check if _trustedForwarder, _liquidityProviders, _lpToken, _tokenManager and _pauser are zero address.

Recommendation Consider implementing require statements where appropriate to validate all address inputs avoiding potential for erroneous values to result in unexpected behaviors or wasted gas.

Reentrancy In Modifiers

Severity low

Description The code inside a modifier is usually executed before the function body, so any state changes or external calls will violate the Checks-Effects-Interactions pattern.

LiquidityPool.sol Has a modifier tokenChecks() that contains an external call and is vulnerable to Reentrancy in the function depositErc20() since it has the modifier prior to nonReentrant.

LiquidityProviders.sol Has a modifier onlyValidLpToken() contains an external call and is vulnerable to Reentrancy in claimFee() since it has the modifier prior to nonReentrant.

Recommendation Use modifiers to replace duplicate condition checks in multiple functions, such as isOwner(), otherwise use require or revert inside the function.

Emission of The Same Event

Severity low

Description LiquidityPool.sol depositNative() and depositErc20() emit the same deposit event. This could cause confusion when reviewing the deposit logs for off-chain programs.

Recommendation Create a different event for both types of deposits.

initializations May Be Front-Run

Description Low Contracts using initialize patterns, instead of constructors, may be susceptible to front-running if not properly deployed. Many contracts use initialize pattern, instead of constructors, at deployment to initialize key contract variables. If factory patterns are not used to deploy and initialize such contracts atomically or if deployment scripts are not robust enough to prevent front-running of such initialization then it may lead to security concerns. While most of them use OpenZeppelin’s initializable to enforce single initializations, few of them reimplement this functionality instead of using the OpenZeppelin library.

Contracts that use initialize: LiquidityFarming.sol LiquidityPool.sol LiquidityProviders.sol WhitelistPeriodManager.sol LPToken.sol

Recommendation Use a factory pattern that will deploy and initialize atomically to prevent front-running of the initialization, or ensure the deployment scripts are robust in case of a front-running attack.

Public Functions That Can Be Set To External

Description Several functions across multiple contracts are marked public and can be marked external to save gas. They are as follows:

In LPToken.sol there are: setSvgHelper(), getAllNftIdsByUser(), tokenURI and supportsInterface().

In TokenManager.sol there are: getEquilibriumFee(), getMaxFee(), getTokensInfo(), getDepositConfig(), and getTransferConfig().

In SvgHelperBase.sol there are: setTokenDecimals(), getAttributes(), getDescription() and getTokenSvg().

In ExecutorManager.sol there are: getExecutorStatus() and getAllExecutors().

In LiquidityFarming.sol there are: setRewardPerSecond(), getNftIdsStaked() and getRewardRatePerSecond()

In LiquidityPool.sol there are: setTrustedForwarder(), getExecutorManager(), setLiquidityProviders() and setExecutorManager().

In LiquidityProviders.sol there are: getTotalReserveByToken(), getSuppliedLiquidityByToken(), getCurrentLiquidity(), increaseCurrentLiquidity(), decreaseCurrentLiquidity() and getFeeAccumulatedOnNft().

Recommendation Switch all of these functions from public to external to save gas on deployment.

Catching The Array Length Prior To Loop.

Description One can save gas by caching the array length (in stack) and using that set variable in the loop. They are as follows:

In ExecutorManager.sol there are: addExecutors(), removeExecutors().

In WhitelistPeriodManager.sol there is: setIsExcludedAddressStatus().

Recommendation For addExecutors(), removeExecutors() simply do something like so: uint length = executorArray.length; As for setIsExcludedAddressStatus() simply do something like so: uint length = _addresses.length;

Rearrangement of Order of Execution.

Description In LiquidityPool.sol By moving require(receiver != address(0), "Receiver address cannot be 0"); and require(amount != 0, "Amount cannot be 0"); to the start of the function. The function will revert before even reaching the more costly require.

Recommendation: Move require(receiver != address(0), "Receiver address cannot be 0"); and require(amount != 0, "Amount cannot be 0"); to the start of these functions depositErc20(), depositNative() and sendFundsToUser().