Tokemak

`_executeControllerCommand` is defined as a public function. This function carries a very dangerous logic, i.e., executing arbitrary transactions without any access control.

The listed parameters are not validated properly: 1. `contracts/wallet/ZeroExTradeWallet.sol:32~33`: check newRouter and newManager against zero addresses 1. `contracts/wallet/ZeroExTradeWallet.sol:46~47`: check `signer` against zero 1. `contracts/controllers/SushiswapController.sol:25~26`: check router snd factory against zero 0. `contracts\manager\Manager.sol::_executeControllerCommand`, should check if `registeredControllers[transfer.controllerId]` is not zero. 0. `contracts\controllers\ZeroExController.sol::constructor`, should check if the input addresses are non-zeros. 0. `contracts\controllers\UniswapController.sol::constructor`, should check if the input addresses are non-zeros. 0. `contracts\pools\Pool.sol::initialize`, should check if the input addresses are non-zeros. 0. `contracts\pools\EthPool.sol::initialize`, should check if the input addresses are non-zeros.

Division takes place before multipliciation: ``` uint256 effectiveSecondsStaked = (secondsStaked.div(schedule.interval)).mul(schedule.interval); ``` This potentially leads to precision loss.

There is no possiblity to alter properties of a given schedule. It might be needed to set certain props, namely `isActive` and `isPublic`. Not being able to unset `isActive` seems to be a bigger issue. To stop deposits, the only possibilities would be **i)** to pause the contract (which would affects all schedules) or **ii)** remove the schedule of interest.

The `withdraw` function in the stated contracts, unlike the deposit function, is not pausable. The same holds true for the `slash` function in the staking contract.

The provided liquidity is not eligible for SUSHI rewards.

By calling the functions listed below, the allowance is set to a very large number. This could be used by external contracts that are not under admin team’s control to drain the contract, whenever the target becomes malicious, or being controlled by malicious users. 0. `contracts\controllers\SushiswapController.sol::_approve`. 0. `contracts\controllers\ZeroExController.sol::_approve`. 0. `contracts\controllers\BalancerController.sol::_approve`. 0. `contracts\controllers\UniswapController.sol::_approve`. 0. `contracts\controllers\ZeroExTradeWallet.sol::_approve`.

Both `deposit` and `withdraw` functions accept external addresses and trust them with calling transfer (safeTransfer) on them. Calling token methods on arbitrary addresses is a bad security practice. At the moment, only the manager can call these methods. Moreover, deposit does not check duplicates before adding tokens to the token list.

All the deposited tokens are approved with the maximum value to be pulled out by the `zeroExRouter` address.