LiquiFi

The function accepts a parameter called `_totalSupply` but does not utilize it properly. It neither checks whether `_cadenceSeconds * _cadenceAmount` is less than the `_totalSupply`, nor whether there are rounding issues that will lead to lost tokens. In case the company owner made a mistake with the inputs, the tokens might vest a lot faster than intended or require manual intervention of the contract owner to retrieve the locked tokens.

The contract does not have any functionality to allow a company to stop vesting a former employee. There are two kinds of employees, `activeEmployees` and `inactiveEmployees`. The contract keeps track of them but never utilizes this information in regard to enabling/disabling of vesting.

Functions `Escrow.deposit()` and `Escrow.withdraw()` are using the _unsafe_ ERC20 interface functions of OpenZeppelin (`transferFrom()` and `transfer()`). Since [some ERC20 tokens do not return a value for these functions](https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca) these calls might revert.

The `createPlan` function in the modules contracts does not check whether a plan with the provided ID already exists, allowing the owner to overwrite any existing plan.

According to the comments, the function `EmployeeRegistry.isOwner()` should return `true` only if the provided address parameter is the owner of of the provided company id parameter and `false` otherwise. However, as the `exists` return value of `EnumerableMap.tryGet(companyOwners, _companyId)` is not checked, providing `address(0)` to any company id would lead to returning `owner == _employee` equal to `true` and therefore violating the comments assumption.

There are two issues: 1. The function `transferOwner` does not check if `_newOwner` is an employee of the `_companyId`. The `_newOwner` could be an inactive employee, or even not an employee of that company. This would violate the assumption that a company owner should be an active employee of that company. 1. The function `transferOwner` does not check if `_newOwner` is not equal to `0x0`. This could lead to a disastrous result: A company has no owner to modify its data.

Although `Escrow.sol` inherits from `ReentrancyGuardUpgradeable`, it does not make use of the `nonReentrant` modifier. Further, functions `deposit()` and `withdraw()` do not comply with the ["Checks-Effects-Interactions" pattern](https://docs.soliditylang.org/en/v0.8.11/security-considerations.html#use-the-checks-effects-interactions-pattern), by performing state variable changes **after** external function calls and not before (`escrowBalance[][]` being changed after calling `transferFrom()`, `balanceOf()` and `transfer()`). As these lower level functions are only callable by `owner` or `vestingControllerAddress` the potential impact is limited.

The functions `createVestPlan` and `vest` are declared `payable`, allowing ether to be sent along the function calls. Since there is no function to retrieve the ether, it will lead to locked funds.

When smart contracts are deployed or their associated functions are invoked, the execution of these operations always consumes a certain quantity of gas, according to the amount of computation required to accomplish them. Modifying an unknown-size array that grows in size over time can result in a Denial-of-Service attack. Simply by having an excessively huge array, users can exceed the gas limit, therefore preventing the transaction from ever succeeding. For the loop on `L242`, the code does not restrict the number of vest plans that an employee in a company can have. This function might run out of gas and cannot be recovered from that state when the employee has too many vest plans. This could happen in case the company owner wants to deny an employee from accessing their vested tokens by adding many low-value vest plans.

The function `EmployeeRegistry.addEmployee()` does not check the return value of the call `EnumerableSet.add(companyIdToEmployees[_companyId].activeEmployees, _employee);`, which would indicate whether the added employee was already in the set. Therefore, calling the function multiple times would lead to duplicate company id entries in `employeeToCompanyIds` after executing `employeeToCompanyIds[_employee].push(_companyId);`.