/Reach - Diff 1

`ReachDistributionV2` is deployed by `ReachFactory`, making the `ReachFactory` contract the owner. However, the `ReachDistributionV2` contract is intended to be owned by the user who is creating the Reach server. Furthermore, the `ReachFactory` is not configured to call the guarded `onlyOwner` functions in `ReachDistributionV2`.

The `ReachFactory` implements the `topUp()` and `reserveEthAllocations()`functions, which will transfer user funds to the contract. However, the expected behavior is not defined in the documentation, and there is no way of withdrawing the funds from this contract, thereby locking them forever.

The `onlySigned` modifier validates the `_signature` and `_data` has been correctly signed by the `msg.sender`. However, the signatures are not marked as used, allowing them to be replayed at a later date. Additionally, the chain ID is not included either, allowing signatures to be used across chains. This vulnerability allows users to call `topUp()` and `reserveEthAllocation()` with the same signature, allowing them to reset their balances to a higher previous balance which might lead to a loss of funds.

The project does not implement tests for the new functionalities related to the `ReachDistribution` and `ReachFactory` contracts. Additionally, the test for the `Reach` token contract is very limited, only testing its deployment. Tests may express requirements and are necessary to validate the software's intent.

The function `Reach.rescueETH20Tokens()` suggests it rescues `ETH` but it rescues `ERC20` tokens that are accidentally sent to the contract.

The owner of the contracts inheriting from OpenZeppelin's Ownable can call `transferOwnership()` to transfer the ownership to a new address. If an uncontrollable address is accidentally provided as the new owner address then the contract will no longer have an active owner, and functions with the `onlyOwner` modifier can no longer be executed. While the implementation of `ReachDistributionV2.transferOwnership()` is a good practice (time lock), the risk of transferring it to the wrong address is still possible.

"Dead" code refers to code that is never executed and hence makes no impact on the final result of running a program. Dead code raises a concern, since either the code is unnecessary or the necessary code's results were ignored. Here are some snippets of dead code found in the codebase: 1. `addLiquidity()` is a private function not called in the contract. 2. In `swapAndLiquify()` there is commented-out code. 3. `claimEnabled` is not used which would make its setter function `setClaimEnabled()` also unnecessary. 4. `devWallet` is not used which would make its setter function `setDevWallet()` also unnecessary. 5. `airdropTokens()` can be removed as it is not used and acknowledged by the Reach team. 6. `EnumerableSet` import can be removed from `ReachDistributionV2.sol`. 7. Taxes in `Reach` have two fields: `treasury` and `liquidity`. However, in the protocol, only the total fee (sum of both) is used.

In `ReachDistributionV2`, `setEthDistribution()` and `setReachDistribution()` are public functions protected by the `onlyOwner` modifier. These allow the owner to change the Merkle roots for both `ETH` and `REACH` allocations, breaking the flow of distribution version creation that should be modified only by `createDistribution()`.

Although `Reach.rescueETH20Tokens()` is intended to just rescue ERC-20 tokens sent to the contract by error, it does not validate the return value of the transaction. Most of the ERC-20 implementations return `true` if the transfer was successful, following the ERC-20 standard. However, some tokens do not follow this recommendation (e.g. USDT), causing DoS and locked tokens in some smart contracts.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error. The following is a non-exhaustive list of functions that should validate their inputs: 1. `ReachFactory.constructor()`: Validate that the `reachToken` is a non-zero address. Since there is no setter for the token address, an incorrect provided value would require a redeployment of the factory. 2. `ReachFactory.setPrices()`: Validate that `_credits` and `_minEthAllocation` are non-zero. 3. `Reach.setSwapTokensAtAmount()`: Validate that the input `amount` lies within a given range with lower and upper values.