Merit Circle

Oversimplifying, a _flash loan_ is a way to atomically borrow some tokens, performs some actions using them, and repays the initial loan at the end of the transaction. A flash loan _attack_ is a way to use flash loans to extract an unfair amount of value from a system. In this system, since rewards are a function of a user's deposits, a quick injection of funds may entitle users to a larger share of rewards. Immediately withdrawing these funds may mean that those rewards are taken from other participants and were not earned honestly.

The following function calls make a call to a function that returns a value, which however is not checked: - `LiquidityMiningManager.sol`, - `addPool()` ignores the return value of `approve()` (Line 62). - `distributeRewards()` ignores the return value of `transferFrom()` (Line 116). - `distributeRewards()` ignores the return value of `transfer()` (Line 129), `call()` (Line 122). - `TokenSaver.sol`: - `saveToken()` not checking return value of `IERC20(_token).transfer()`. - `BasePool.sol`: - `claimRewards()` not checking return value of `rewardToken.transfer(_receiver, nonEscrowedRewardAmount);`. - the `constructor` ignores the return value of `approve()` (Line 42).

In `BasePool._mint()`, `BasePool._burn()` and `TimeLockPool.withdraw()` the insecure primitive casting operations `int256()` and `uint256()` are used. For sufficiently large positive or negative values these casts may wrap around, without leading to a revert and therefore lead to unexpected behaviour. In function `cumulativeRewardsOf` of `AbstractRewards.sol`, a `UInt256` is cast to an `Int256` variable, which may not be safe (Line 71).

In `LiquidityMiningManager.removePool()` parameter `_poolId` is not checked to be within the bounds of `pools.length`, allowing the caller to write the contents of `pools[pools.length - 1]` to an arbitrary storage location, by carefully crafting `_poolId`, when executing L75 of `LiquidityMiningManager.sol`. Similarly, but in this case performable by anyone and not just an address having the `GOV_ROLE` role, in `TimeLockPool.withdraw()` `_depositId` is not checked to be within the bounds of `depositsOf[_msgSender()].length`, allowing the caller to write the contents of `depositsOf[_msgSender()][depositsOf[_msgSender()].length - 1]` to an arbitrary storage location, by carefully crafting `_depositId`, when executing L71 of `TimeLockPool.sol`. See https://github.com/Arachnid/uscc/tree/master/submissions-2017/doughoyte for reference.

In `View.fetchData()` the `multiplier` field of the Deposit structures in L80 and L107 are set with `multiplier: poolContract.getMultiplier(deposit.end - deposit.end)` and `multiplier: escrowPool.getMultiplier(deposit.end - deposit.end)`. As `deposit.end - deposit.end` is used it will always result in zero, always returning the same multiplier `1e18`.

In `View.fetchData()` and `LiquidityMiningManager.distributeRewards()` a for-loop is used to iterate over the pools returned by `liquidityMiningManager.getPools()`. Similarly, `TimeLockPool.depositsOf`, which is user-controllable through deposits and iterated over in `TimeLockPool.getTotalDeposit()`. For sufficiently large arrays, processing over these arrays could run out of gas.

There is a comment `ignore tx failing` on line 121. The effects of ignoring a failed transaction is not clear.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error. The following functions do not have a proper validation of input parameters: 1. `BasePool.constructor()` does not check that `_depositToken`is not the zero address, thus neither does the constructors of `TimeLockPool.sol` and `TimeLockNonTransferablePool.sol`. 1. `View.constructor()` does not check that parameters `_liquidityMiningManager` and `_escrowPool` are different from `address(0)`. 1. `LiquidityMiningManager.constructor()` does not check that parameters `_reward` and `_rewardSource` are different from `address(0)`. 1. `LiquidityMiningManager.addPool()` does not check that parameter `_poolContract` is different from `address(0)` or parameter `_weight` is different from zero. 1. `LiquidityMiningManager.adjustWeight()` does not check that parameter `_poolId` is smaller than `LiquidityMiningManager.pools.length` or that parameter `_newWeight` is non-zero. 1. `AbstractRewards.constructor` does not check that parameters `getSharesOf_` and `getTotalShares_` are different from `address(0)`. 1. `AbstractRewards._prepareCollect()` does not check that parameter `_account` is different from `address(0)`. 1. `AbstractRewards._correctPointsForTransfer()` does not check that parameters `_from` and `_to` are different from `address(0)` or `_shares` is non-zero. 1. `AbstractRewards._correctPoints()` does not check that parameter `_account` is different from `address(0)` or _shares is non-zero. 1. `TimeLockPool.constructor()` does not check that parameter `_maxLockDuration` is greater than `MIN_LOCK_DURATION` (10min). 1. `TimeLockPool.deposit()` does not check that parameters `_amount` and `_duration` are non-zero or that parameter `_receiver` is different from `address(0)`. 1. `TimeLockPool.withdraw()` does not check that parameter `_depositId` is within `depositsOf[_msgSender()].length`, or that parameter `_receiver` is different from `address(0)`.

Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.4.*`. The caret (`^`) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version _and above_, hence the term "unlocked".