QA Report

[code423n4]

Title: Require with empty message
Severity: Low Risk

The following requires are with empty messages.
This is very important to add a message for any require. Such that the user has enough
information to know the reason of failure:

    Solidity file: Pausable.sol, In line 60 with Empty Require message.

Title: Does not validate the input fee parameter
Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    TokenManager.changeFee (_maxFee)
    TokenManager.changeFee (_equilibriumFee)
    TokenManager.addSupportedToken (equilibriumFee)
    TokenManager.addSupportedToken (maxFee)

Title: Init frontrun
Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself.
(details credit to: code-423n4/2021-09-sushimiso-findings#64)
The vulnerable initialization functions in the codebase are:

    LiquidityFarming.sol, initialize, 78
    LiquidityProviders.sol, initialize, 78
    WhitelistPeriodManager.sol, initialize, 60
    LPToken.sol, initialize, 36
    Pausable.sol, __Pausable_init, 26
    LiquidityPool.sol, initialize, 87

Title: Named return issue
Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing.
Furthermore, removing either the actual return or the named return will save gas.

    LiquidityProviders.sol, _msgSender
    LiquidityPool.sol, _msgSender
    LiquidityFarming.sol, _msgSender
    TokenManager.sol, _msgSender

Title: Not verified owner
Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    
    LPToken.sol.getAllNftIdsByUser _owner

Title: Missing non reentrancy modifier
Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer.
Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    LiquidityPool.sol, permitAndDepositErc20 is missing a reentrancy modifier
    LiquidityProviders.sol, receive is missing a reentrancy modifier
    LiquidityFarming.sol, updatePool is missing a reentrancy modifier
    LiquidityProviders.sol, decreaseCurrentLiquidity is missing a reentrancy modifier

Title: In the following public update functions no value is returned
Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0.
We assumed that after the update you return the latest new value.
(similar issue here: code-423n4/2021-10-badgerdao-findings#85).

    LPToken.sol, updateTokenMetadata
    TokenManager.sol, updateTokenCap

Title: Not verified input
Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    LiquidityProviders.sol._transferFromLiquidityPool _tokenAddress
    LiquidityProviders.sol._isSupportedToken _token
    LiquidityPool.sol.checkHashStatus tokenAddress
    Pausable.sol.__Pausable_init pauser

Title: Two Steps Verification before Transferring Ownership
Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked.
It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership.
A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    LPToken.sol
    LiquidityPool.sol
    WhitelistPeriodManager.sol
    LiquidityProviders.sol

[ankurdubey521]

Duplicate of #9

[pauliax]

Same as #9