Saddle security record

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.

The function `completeToSynth` is callable by any user after the `maxSecsLeftInWaitingPeriod`. Although the `synth` wil be sent to the correct `nftOwner` regardless of `msg.sender`, this could have impact on external contracts that rely upon calling `completeToSynth`. For example, suppose some contract has a function of the form: ``` function getSynthAndTransfer(...) { Bridge.completeToSynth(id); synth.transfer(external_addr); } ``` If another arbitrary user invokes `completeToSynth` before the contract invokes it, the funds could be locked forever (assuming the contract did not have sufficient "retrieval" functions).

To save some gas on settlement and storage in general (in case of backup or migration), a cloned `SynthSwapper` that has already been used for settled swaps, can be destroyed. You can also remove the swaps from the pending list.

These pending swap states are set but never checked anywhere; this must be due to implicit assumptions on the state of swaps, but opens the door for duplicated calls to, for example, `completeToToken()`. A second duplicated call to `completeToToken()` might be possible with `swapAmount = 0` even if it was completed before.