Saddle Token

The constant `DURATION = 2 * (52 weeks)` is not documented. Since the provided requirement states that "transfer is blocked until 52 weeks pass...", it is not clear if this is defined correctly.

Given that the constructor is using a loop to distribute the initial amount of the token inside `Vesting()` contracts, there is the concern that given an enough long list of recipients the constructor will run out of gas. Also, there is no other way to include further recipients afterward if the list of recipients list has to be shortened.

Since [Solidity 0.8.x does not perform safe casting by default](https://github.com/ethereum/solidity/issues/10195), there is a potential integer overflow on L96 in `RetroactiveVesting.sol` where a summation of 2 `uint256` variables is cast to `uint120`.

On `rescueTokens()` there is a check to avoid transferring 0 tokens, but there is a check missing to transfer 0 ETH.

1. `Vesting.initialize` should ensure that `_token` is non-zero and `_durationInSeconds` is non-zero (the latter is to avoid a possible divide-by-zero in `vestedAmount`). 2. `RetroactiveVesting.constructor` should ensure that all three address arguments are non-zero. 3. `SDL.constructor` should check that `_pausePeriod > 0` and also `_pausePeriod <= 52 weeks`. If the value were set to zero, it would allow immediate transfers since `_pause` would not be invoked. The constructor should also check that `_governance`, `_vestingContractTarget`, and each subfield of `_recipients` is non-zero (except `cliffPeriod`). 1. `RetroactiveVesting.verifyAndClaimReward()` does not check if `totalAmount` is greater than zero and if it fits into `uint120`. Due to the cast on L69, this could lead to an integer overflow in case `totalAmount` is greater than the largest integer in `uint120`. 3. `Vesting.changeBeneficiary()` does not check if `newBeneficiary != beneficiary`. 5. `SimpleGovernance.changeGovernance()` does not check that `newGovernance != governance`.

According to *natspec* the addresses in `allowedTransferee` are allowed to transfer tokens even if the contract is paused. Is understood that these addresses can be `from` addresses when transferring. But in the logic at `_beforeTokenTransfer()` if the `to` address is in allowed then the transfer is allowed (`allowedTransferee[from] || allowedTransferee[to]`). This logic seems unclear and could allow users to partner with an allowed accomplice account to drain funds out of the contract, even during a paused contract scenario.

The `GenericERC20WithGovernance` contract extends `SimpleGovernance` and its description in the code comments indicates: "This contract simulates a generic ERC20 token that is mintable and burnable." First, it is important to note that there is no publicly accessible burn function. Therefore, there is a deviation from the specification at this point. Second, and more importantly, the `owner` and not the `governance` address is the one which is only allowed to call the `mint()` function. This is confusing as one would expect the decision to mint tokens would be given to the `governance` address, not to the `owner`.

The function `changeBeneficiary` allows governance to set a new beneficiary address for any `Vesting` contract. Suppose the `Vesting` contract owns 100 total tokens and they are 50% vested. If the old beneficiary address has recently been claimed, they would have received up to 50 tokens and the new beneficiary would only receive around 50 as well (after full vesting). However, if the previous beneficiary has not claimed any tokens yet, that address will not be entitled to any tokens after the change, and therefore the new address will be allotted all 100 tokens after the full vesting period. It is not clear which case is desirable.

The contract mentions that vesting is "optionally revocable by the owner" and later mentions that `currentBalance` can be 0 in case of vesting being revoked earlier." However, it does not appear that any revocation logic exists.

The `Vesting.vestedAmount()` function returns the minimum between `currentBalance` and `unreleased` on L129, and the comment right above on L128 indicates: "`currentBalance` can be 0 in case of vesting being revoked earlier." However, in the case when `currentBalance == 0`, the subtraction on L126 would underflow and the function would throw. This would also seemingly block the beneficiary from calling `release()`. However, since the intended vested amount would be anyway `0` it would effectively not make a difference to the beneficiary's balance. Nevertheless, the intended error message: "No tokens to release" would not be displayed to the beneficiary.