/Reach - Diff 2

The off-chain system generating the Merkle trees and distributions is out of the scope of this audit, but as stated by the Reach team, non-claimed rewards will be included in the next distribution. An exploitable behavior has been identified where a user can intentionally delay claiming rewards until observing a new `createDistribution()` transaction by the system owner in the mempool. Subsequently, the user can claim their un-claimed rewards before the execution of `createDistribution()` and then claim them again after the distribution creation process.

In `ReachAffiliateDistribution.swapEth()`, part of the `_ethAmount` is transferred to the main distribution contract. Specifically, `uint256 commissionAmount = (_ethAmount * commission) / 100;` is transferred to the `mainDistribution` contract. However, in the lines that follow, it swaps `_ethAmount` of ETH for `REACH` tokens, without accounting for the `commissionAmount` that was already transferred out, which can lead to serious accounting issues because the contract might not have enough ETH for the swap. ``` router.swapExactETHForTokensSupportingFeeOnTransferTokens{ value: _ethAmount }(_outputAmount, path, address(this), block.timestamp + 5); ``` Furthermore, if the extra ETH amount that is being swapped is part of the user’s distribution, this could block some users from claiming their rewards.

Dead code should be avoided since it might consume more resources during development, testing, and compilation without contributing to the software's functionality. It makes the codebase harder to understand and may mislead developers, leading to potential bugs or wasted time. The reader might also believe that the code is incomplete. In the case of solidity, it might also increase the bytecode size increasing deployment costs. The variable `transferThreshold`, the import `EnumerableSet`, and the data structure `Config` are not used. Additionally, both distribution contracts have the variable `totalEthAllocated` whose goal and functionality are unclear.

It is unclear why `_ethAmount` is not used in the verification in `createDistribution`. It is likely that `if (address(this).balance < totalEthAllocated)` should be replaced with `if (address(this).balance < _ethAmount)` to ensure that the distributed amount is less than the available balance in the contract. This can potentially lead to an accounting issue. ``` function createDistribution( bytes32 _merkleRoot, uint256 _ethAmount, uint256 _reachAmount ) public onlyOwner { ... if (address(this).balance < totalEthAllocated) revert UnsufficientEthBalance(); ... } ```

As discussed with the Reach team, the goal `totalEthAllocated` logic in `ReachAffiliateDistribution` is to avoid allocating funds in distributions that will be used to buy back Reach tokens using `swapEth()`. However, as shown in several issues above, the variable introduced various issues. We concluded that this logic is not designed properly to achieve that goal, and should be revised.

Code duplication is detrimental as it leads to the maintenance of the same issues in different locations, and is therefore, error-prone. It makes code harder to understand and increases development time. In the case of solidity, it might also lead to larger bytecode which might hinder the deployment of the system. The contracts `ReachAffliateDistribution.sol` and `ReachMainDistribution.sol` share a lot of the same code. It is advisable to utilize inheritance to facilitate the reuse of code, which will make the codebase easier to maintain in the future.

In `swapEth()`, there is this line `totalEthAllocated += address(this).balance - _ethAmount;`. Note that if `_ethAmount` is not equal to `address(this).balance`, the value `totalEthAllocated` will be double counting the contract's balance when `swapEth()` is called multiple times. For example, suppose the contract has a balance of `10 ETH` and `totalEthAllocated = 0`. The owner calls `swapEth()` with `_ethAmount = 1`. This updates `totalEthAllocated` to be `10 - 1 = 9`. The owner again calls `swapEth()` with `_ethAmount = 1`, this updates `totalEthAllocated` to be `9 + (9 -1) = 17`, which is double counting the `8 ETH` balance in the contract across the two `swapETH()` calls.

In the documentation, it mentioned that each mission credit costs `100 REACH` tokens (see [here](https://docs.getreach.xyz/lang/overview/mission-types)). However, in `ReachMainDistribution.sol`, we have `uint256 public creditPrice = 50 ether;`, meaning that each credit costs `50 REACH` tokens.

Based on the context of how the commission is used in `ReachAffliateDistribution.sol`, `commission` represents a percentage, which is denominated by `100`. However, in `setCommission()`, we can set the commission to be over `100`, which can lead to unexpected behavior, such as potentially reverting on this line ` payable(mainDistribution).transfer(commissionAmount);` in `swapEth()`.

If the owner renounces their ownership, all ownable contracts will be left without an owner. Consequently, any function guarded by the `onlyOwner` modifier will no longer be able to be executed.