Illuvium security record

The contract size can be optimized by turning `modifiers` into `functions`, as shown in the example below: ``` modifier checkStuff() {} function doSomething() checkStuff {} ``` The `function doSomething()` can rewritten without modifiers as: ``` function checkStuff() private {} function doSomething() { checkStuff(); } ```

In `setEndTime`, there is no sanity checks for the new value. It could be before the current time, or interact with existing pools in unexpected ways.

The function `unstake` in `FlashPool.sol` converts a `uint256` to a `unit128` without checking that the cast is safe. Similar issues - with as small a bit width as 64 - exist elsewhere in the contract. These issues are found in `CorePool.sol` as well.

If the ETH balance of `IlluviumVault` is zero (possibly due to a previous call to either `swapEthForIlv` or `sendIlvRewards`), the function will revert due to the check `balance > 0` on L159. However, since `swapEthForIlv` is public, a legitimate call to `sendIlvRewards` could be griefed by any user if they front-run with a call to `swapEthForIlv`.

According to the inline documentation, step 2 should invoke `setPool`, and step 3 sets balances (through potentially multiple calls to `setBalances`). However, the function `setBalances` requires on L164 that `address(pool) == address(0)`, so step 3 cannot occur after step 2. The steps on L17-20 appear correct, but L126 and L149 do not align with this summary.

In the `IlluviumCorePool.stakeAsPool` function on L155 code comment says that the `_useSILV` should be `false`, however it's not in the case on L165 where the 2nd parameter passed to the `_processRewards` function, which represents the value of `_useSILV` is hardcoded to `true`.

Assumptions about intermediate values during function processing should be explicitly checked, especially if these values depend on outputs returned by external contract calls. For example, we assume that the value of `stakeWeight` on L420 inside of `IlluviumPoolBase._stake()` should be greater than zero. Otherwise, it doesn't make sense to create a deposit with `stakeWeight == 0`. <br><br> Note that this is one example of what we assume to be an implicit assumption, however, all implicit assumptions should be checked in a similar way.

The following instances of missing or insufficient input validation have been encountered: 1. The `_pool` parameter of the of the `TokenLocking.setPool()` function is not checked to conform to the `ILockedPool` interface and could be any address. 2. The `_rewardsAmount` parameter of the `IlluviumLockedPool.receiveVaultRewards()` function is not checked to be greater than zero. The same applies to the function with the same name in other contracts. 3. The `_from` and `_to` parameters of `IlluviumLockedPool.changeLockedHolder()` are not checked to be different. This could lead to deleting a holder. 4. The `_vault` parameter of `IlluviumCorePool.setVault()` is not checked to be different from `address(0)`. 5. The `_weight` parameter of `IlluviumPoolBase.setWeight()` is not checked to be greater than zero. Note that this is not an exhaustive list. User inputs should always be validated.

The `TokenLocking.setBalances()` function may be called multiple times in order to "allow setting balance to zero in case of accidental addition of the holder". However, due to missing checks/assertions there exists a possibility for human error which could lead to unlocking a total balance larger than intended.

In `IlluviumCorePool.sol` on L30, the comment states that `poolTokenReserve` is the "Total value of ILV tokens available in the pool". However, while functions such as `IlluviumCorePool.receiveVaultRewards` only increase `poolTokenReserve` when `poolToken == ilv`, this is not the case for functions such as `IlluviumCorePool._stake`. For example, if the `poolToken` is `ILV/ETH Pair`, the `poolTokenReserve` is still increased on L205, even though pair tokens are staked (not ILV). This makes the computation in `IlluviumVault.sendIlvRewards` of `ilvInPairPool` unclear: ``` uint256 ilvInPairPool = (pairPoolReserve.mul(ilv.balanceOf(address(ilvEthPair))).div(ilvEthPair.totalSupply())).add( pairPoolIlvBalance ); ``` In particular, the expression `(pairPoolReserve.mul(ilv.balanceOf(address(ilvEthPair))).div(ilvEthPair.totalSupply()))` seems to suggest that `pairPoolReserve` should store the amount of `ILV/ETH Pair` tokens in the `ILV/ETH Pair` pool, *NOT* the ILV balance itself. With the current setup, it appears that the `ilvInPairPool` computation above will double-count some tokens, since `pairPoolReserve` is increased for both `ILV` and `ILV/ETH Pair` deposits. This will inflate the weight associated with the `ILV/ETH Pair` pool.