Saddle Finance

The `SwapUtils.getD` function contains an incorrect `add(1)` for the denominator of `dP` inside the inner `for`-loop, on L241. This will lead to an incorrect value of `D` being returned every time `SwapUtils.getD` is called.

Integer overflow/underflow occur when an integer hits its bit-size limit. Every integer has a set range; when that range is passed, the value loops back around. A clock is a good analogy: at 11:59, the minute hand goes to 0, not 60, because 59 is the largest possible minute. Integer overflow and underflow may cause many unexpected kinds of behavior and was the core reason for the `batchOverflow` attack. <br> The subtraction inside `SwapUtils.withdrawAdminFees` does not use `SafeMath`, which could cause an underflow if `token.balanceOf(address(this)) < self.balances[i]`. The result of the underflow would then be assigned to the `uint256 balance` variable, which would be positive and therefore always pass the check on the subsequent line: `if (balance > 0)`. <br> A similar underflow issue occurs on L504 inside `SwapUtils.getAdminBalance`.

The following functions are missing checks for user input: 1. **[Fixed]** The `Swap.getToken` function does not check if the value of the `index` parameter is lower than the length of the `pooledTokens` array. 2. **[Fixed]** The `Swap.getTokenBalance` function does not check if the value of the `index` parameter is lower than the length of the `pooledTokens` array. 3. **[Fixed]** The `Swap.calculateSwap` function does not check if value of the 2 `tokenIndex{From | To}` parameters are lower than the length of the `pooledTokens` array. It also does not check if the value of the `dx` input parameter is lower than the amount of tokens that the user actually has. 4. **[Fixed]** The `Swap.calculateRemoveLiquidity` function does not check if value of the `amount` parameter is larger than the total supply of the `swapStorage.lpToken`. 5. **[Fixed]** The `Swap.calculateRemoveLiquidityOneToken` function does not check if the value of the `tokenIndex` parameter is lower than the length of the `pooledTokens` array. It also doesn't check if the value of the `tokenAmount` is lower than the actual amount available for that token. 6. **[Fixed]** The `Swap.getAdminBalance` function does not check if the value of the `index` parameter is lower than the length of the `pooledTokens` array. 7. **[Fixed]** The `Swap.swap` function does not check if value of the 2 `tokenIndex{From | To}` parameters are lower than the length of the `pooledTokens` array. It also does not check if the value of the `dx` input parameter is lower than the amount of tokens that the user actually has. 8. **[Fixed]** There should be checks for `poolAddress != address(0x0)` in the functions `Allowlist.setPoolAccountLimit`, and `Allowlist.setPoolCap`. 9. **[Fixed]** The `Swap.constructor` does not check if the length of the `_pooledTokens.length > 1`. This is an implicit assumption, which should be made explicit. 10. **[Fixed]** The `Swap.constructor` does not check if all the addresses provided in `_pooledTokens` are different. This would allow pools with multiple tokens, where the tokens have the same address. The consequences of an exploit of the aforementioned items vary. However, some of the items mentioned above could significantly impact the reputation of the project and should be therefore addressed.

There is no verification when the `stopRampA()` function is called to check if: 1. the current ramp has already ended due to the `self.futureATime` being in the past. 2. the `stopRampA()` was already called before to stop the current ramp. This allows calling this function multiple times (consecutively) with the effect that each time the `self.initialATime` is set to the current block timestamp, which prevents a new ramp for `MIN_RAMP_TIME` seconds. This could be problematic if the `stopRampA` function is called multiple times by mistake.

The [checks-effects-interactions pattern](https://solidity.readthedocs.io/en/develop/security-considerations.html?highlight=check%20effects#use-the-checks-effects-interactions-pattern) is respected by most methods with one notable exception being the `SwapUtils.addLiquidity` method.

The return value of the following call on L94: `allowlist.getAllowedAmount(address(this), address(0)); // crude check of the allowlist`, is ignored inside the `Swap.constructor`. It seems that this function is called to check if the `allowlist` address is indeed the address of an `Allowlist` contract instance. However, there are 2 problems with this approach: 1. The address can point to any address even `address(0)`, an EOA or a contract that does not have the `getAllowedAmount` and that call will still not cause a revert and will only return `0`. 2. If the address indeed points to the `Allowlist` contract, then the parameters passed to this call: `allowlist.getAllowedAmount(address(this), address(0));` should correctly return `0`. Therefore, wrapping the call to `allowlist.getAllowedAmount` in a `require` statement that checks if the return value of that function is equal to `0` will not confirm if the address indeed points to the `Allowlist` contract.

The `SwapUtils.rampA()` function uses primitive addition (+) and multiplication (*) operators on L921, L922 and L925. The latter could cause an overflow if the value of the `futureA_` input parameter is too large.

The 2 functions `SwapUtils.calculateRemoveLiquidity` and `SwapUtils.calculateRebalanceAmounts` have the same implementation logic. However, they have different names and the former is `external` and the latter is `internal`.

The [reference implementation](https://github.com/curvefi/curve-contract/blob/master/contracts/pool-templates/base/SwapTemplateBase.vy#L577) of the `removeLiquidityImbalance` function contains an additional assertion that check if the `tokenAmount` is different from zero, before adding one to it. This is missing on L759 in Saddle.

The error message of the first `require` statement inside `rampA()` says that: "Ramp already ongoing". However, this `require` statement only checks if the `MIN_RAMP_TIME` has passed since the last call to `rampA()`, when the value of `self.initialATime` was set. The time when the previous ramp ends is actually given by `self.futureATime`, which is required to be greater or equal to `self.initialATime + MIN_RAMP_TIME` on L922.