Merge pull request #1 from Blockeys/bug/postAuditFixes

Post audit fixes

contracts/Core.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 pragma experimental ABIEncoderV2;
 
 import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
@@ -12,7 +12,7 @@ import "./Interface/IDerivativeLogic.sol";
 
 import "./Errors/CoreErrors.sol";
 
-import "./Lib/usingRegistry.sol";
+import "./Lib/UsingRegistry.sol";
 import "./Lib/LibDerivative.sol";
 import "./Lib/LibCommission.sol";
 
@@ -23,7 +23,7 @@ import "./SyntheticAggregator.sol";
 import "./TokenSpender.sol";
 
 /// @title Opium.Core contract creates positions, holds and distributes margin at the maturity
-contract Core is LibDerivative, LibCommission, usingRegistry, CoreErrors, ReentrancyGuard {
+contract Core is LibDerivative, LibCommission, UsingRegistry, CoreErrors, ReentrancyGuard {
     using SafeMath for uint256;
     using LibPosition for bytes32;
     using SafeERC20 for IERC20;
@@ -51,8 +51,8 @@ contract Core is LibDerivative, LibCommission, usingRegistry, CoreErrors, Reentr
     // Hashes of cancelled tickers
     mapping (bytes32 => bool) public cancelled;
 
-    /// @notice Calls Core.Lib.usingRegistry constructor
-    constructor(address _registry) public usingRegistry(_registry) {}
+    /// @notice Calls Core.Lib.UsingRegistry constructor
+    constructor(address _registry) public UsingRegistry(_registry) {}
 
     // PUBLIC FUNCTIONS
 
@@ -61,7 +61,7 @@ contract Core is LibDerivative, LibCommission, usingRegistry, CoreErrors, Reentr
     function withdrawFee(address _tokenAddress) public nonReentrant {
         uint256 balance = feesVaults[msg.sender][_tokenAddress];
         feesVaults[msg.sender][_tokenAddress] = 0;
-        IERC20(_tokenAddress).transfer(msg.sender, balance);
+        IERC20(_tokenAddress).safeTransfer(msg.sender, balance);
     }
 
     /// @notice Creates derivative contracts (positions)
@@ -516,9 +516,12 @@ contract Core is LibDerivative, LibCommission, usingRegistry, CoreErrors, Reentr
         // authorFee = fee - opiumFee
         uint256 authorFee = fee.sub(opiumFee);
 
+        // Get opium address
+        address opiumAddress = registry.getOpiumAddress();
+
         // Update feeVault for Opium team
         // feesVault[opium][token] += opiumFee
-        feesVaults[registry.getOpiumAddress()][_derivative.token] = feesVaults[registry.getOpiumAddress()][_derivative.token].add(opiumFee);
+        feesVaults[opiumAddress][_derivative.token] = feesVaults[opiumAddress][_derivative.token].add(opiumFee);
 
         // Update feeVault for `syntheticId` author
         // feeVault[author][token] += authorFee

contracts/Errors/CoreErrors.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 contract CoreErrors {
     string constant internal ERROR_CORE_NOT_POOL = "CORE:NOT_POOL";

contracts/Errors/MatchingErrors.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 contract MatchingErrors {
     string constant internal ERROR_MATCH_CANCELLATION_NOT_ALLOWED = "MATCH:CANCELLATION_NOT_ALLOWED";

contracts/Errors/OracleAggregatorErrors.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 contract OracleAggregatorErrors {
     string constant internal ERROR_ORACLE_AGGREGATOR_NOT_ENOUGH_ETHER = "ORACLE_AGGREGATOR:NOT_ENOUGH_ETHER";

contracts/Errors/RegistryErrors.sol

@@ -1,8 +1,10 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 contract RegistryErrors {
     string constant internal ERROR_REGISTRY_ONLY_INITIALIZER = "REGISTRY:ONLY_INITIALIZER";
     string constant internal ERROR_REGISTRY_ONLY_OPIUM_ADDRESS_ALLOWED = "REGISTRY:ONLY_OPIUM_ADDRESS_ALLOWED";
+
+    string constant internal ERROR_REGISTRY_CANT_BE_ZERO_ADDRESS = "REGISTRY:CANT_BE_ZERO_ADDRESS";
 
     string constant internal ERROR_REGISTRY_ALREADY_SET = "REGISTRY:ALREADY_SET";
 }

contracts/Errors/SyntheticAggregatorErrors.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 contract SyntheticAggregatorErrors {
     string constant internal ERROR_SYNTHETIC_AGGREGATOR_DERIVATIVE_HASH_NOT_MATCH = "SYNTHETIC_AGGREGATOR:DERIVATIVE_HASH_NOT_MATCH";

contracts/Errors/usingRegistryErrors.sol → contracts/Errors/UsingRegistryErrors.sol

@@ -1,5 +1,5 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
-contract usingRegistryErrors {
+contract UsingRegistryErrors {
     string constant internal ERROR_USING_REGISTRY_ONLY_CORE_ALLOWED = "USING_REGISTRY:ONLY_CORE_ALLOWED";
 }

contracts/Helpers/ExecutableByThirdParty.sol

@@ -1,9 +1,9 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 /// @title Opium.Helpers.ExecutableByThirdParty contract helps to syntheticId development and responsible for getting and setting thirdparty execution settings
 contract ExecutableByThirdParty {
     // Mapping holds whether position owner allows thirdparty execution
-    mapping (address => bool) thirdpartyExecutionAllowance;
+    mapping (address => bool) internal thirdpartyExecutionAllowance;
 
     /// @notice Getter for thirdparty execution allowance
     /// @param derivativeOwner Address of position holder that's going to be executed

contracts/Helpers/HasCommission.sol

@@ -1,11 +1,11 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 /// @title Opium.Helpers.HasCommission contract helps to syntheticId development and responsible for commission and author address
 contract HasCommission {
     // Address of syntheticId author
-    address public author;
+    address internal author;
     // Commission is in Opium.Lib.LibCommission.COMMISSION_BASE base
-    uint256 public commission = 25; // 0.25% of profit
+    uint256 constant internal AUTHOR_COMMISSION = 25; // 0.25% of profit
 
     /// @notice Sets `msg.sender` as syntheticId author
     constructor() public {
@@ -21,6 +21,6 @@ contract HasCommission {
     /// @notice Getter for syntheticId author commission
     /// @return uint26 syntheticId author commission
     function getAuthorCommission() public view returns (uint256) {
-        return commission;
+        return AUTHOR_COMMISSION;
     }
 }

contracts/Interface/IDerivativeLogic.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 pragma experimental ABIEncoderV2;
 
 import "../Lib/LibDerivative.sol";

contracts/Interface/IOracleId.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 /// @title Opium.Interface.IOracleId contract is an interface that every oracleId should implement
 interface IOracleId {

contracts/Lib/LibCommission.sol

@@ -1,13 +1,13 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 /// @title Opium.Lib.LibCommission contract defines constants for Opium commissions
 contract LibCommission {
     // Represents 100% base for commissions calculation
-    uint256 constant COMMISSION_BASE = 10000;
+    uint256 constant public COMMISSION_BASE = 10000;
 
     // Represents 100% base for Opium commission
-    uint256 constant OPIUM_COMMISSION_BASE = 10;
+    uint256 constant public OPIUM_COMMISSION_BASE = 10;
 
     // Represents which part of `syntheticId` author commissions goes to opium
-    uint256 constant OPIUM_COMMISSION_PART = 1;
+    uint256 constant public OPIUM_COMMISSION_PART = 1;
 }

contracts/Lib/LibDerivative.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 pragma experimental ABIEncoderV2;
 
 /// @title Opium.Lib.LibDerivative contract should be inherited by contracts that use Derivative structure and calculate derivativeHash

contracts/Lib/LibEIP712.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 /// @title Opium.Lib.LibEIP712 contract implements the domain of EIP712 for meta transactions
 contract LibEIP712 {

contracts/Lib/usingRegistry.sol → contracts/Lib/UsingRegistry.sol

@@ -1,11 +1,11 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 import "../Registry.sol";
 
-import "../Errors/usingRegistryErrors.sol";
+import "../Errors/UsingRegistryErrors.sol";
 
-/// @title Opium.Lib.usingRegistry contract should be inherited by contracts, that are going to use Opium.Registry
-contract usingRegistry is usingRegistryErrors {
+/// @title Opium.Lib.UsingRegistry contract should be inherited by contracts, that are going to use Opium.Registry
+contract UsingRegistry is UsingRegistryErrors {
     // Emitted when registry instance is set
     event RegistrySet(address registry);
 
@@ -23,4 +23,10 @@ contract usingRegistry is usingRegistryErrors {
         registry = Registry(_registry);
         emit RegistrySet(_registry);
     }
+
+    /// @notice Getter for registry variable
+    /// @return address Address of registry set in current contract
+    function getRegistry() external view returns (address) {
+        return address(registry);
+    }
 }

contracts/Lib/Whitelisted.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 /// @title Opium.Lib.Whitelisted contract implements whitelist with modifier to restrict access to only whitelisted addresses
 contract Whitelisted {
@@ -11,7 +11,8 @@ contract Whitelisted {
         bool allowed = false;
 
         // Going through whitelisted addresses array
-        for (uint256 i = 0; i < whitelist.length; i++) {
+        uint256 whitelistLength = whitelist.length;
+        for (uint256 i = 0; i < whitelistLength; i++) {
             // If `msg.sender` is met within whitelisted addresses, raise the flag and exit the loop
             if (whitelist[i] == msg.sender) {
                 allowed = true;

contracts/Lib/WhitelistedWithGovernance.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 import "./Whitelisted.sol";
 
@@ -13,16 +13,14 @@ contract WhitelistedWithGovernance is Whitelisted {
     event Committed(address[] whitelist);
 
     // Proposal life timelock interval
-    uint256 public TIME_LOCK_INTERVAL;
+    uint256 public timeLockInterval;
 
     // Governor address
     address public governor;
 
-    // Contract initialization flag
-    bool public initialized = false;
-
     // Timestamp of last proposal
-    uint256 public proposalTime = 0;
+    uint256 public proposalTime;
+
     // Proposed whitelist
     address[] public proposedWhitelist;
 
@@ -36,7 +34,7 @@ contract WhitelistedWithGovernance is Whitelisted {
     /// @param _timeLockInterval uint256 Initial value for timelock interval
     /// @param _governor address Initial value for governor
     constructor(uint256 _timeLockInterval, address _governor) public {
-        TIME_LOCK_INTERVAL = _timeLockInterval;
+        timeLockInterval = _timeLockInterval;
         governor = _governor;
         emit GovernorSet(governor);
     }
@@ -46,17 +44,17 @@ contract WhitelistedWithGovernance is Whitelisted {
         // Restrict empty proposals
         require(_whitelist.length != 0, "Can't be empty");
 
+        // Consider empty whitelist as not initialized, as proposing of empty whitelists is not allowed
         // If whitelist has never been initialized, we set whitelist right away without proposal
-        if (!initialized) {
-            initialized = true;
+        if (whitelist.length == 0) {
             whitelist = _whitelist;
-            emit Committed(whitelist);
+            emit Committed(_whitelist);
 
         // Otherwise save current time as timestamp of proposal, save proposed whitelist and emit event
         } else {
             proposalTime = now;
             proposedWhitelist = _whitelist;
-            emit Proposed(proposedWhitelist);
+            emit Proposed(_whitelist);
         }
     }
 
@@ -66,7 +64,7 @@ contract WhitelistedWithGovernance is Whitelisted {
         require(proposalTime != 0, "Didn't proposed yet");
 
         // Check if timelock interval was passed
-        require((proposalTime + TIME_LOCK_INTERVAL) < now, "Can't commit yet");
+        require((proposalTime + timeLockInterval) < now, "Can't commit yet");
 
         // Set new whitelist and emit event
         whitelist = proposedWhitelist;
@@ -79,6 +77,7 @@ contract WhitelistedWithGovernance is Whitelisted {
     /// @notice This function allows governor to transfer governance to a new governor and emits event
     /// @param _governor address Address of new governor
     function setGovernor(address _governor) public onlyGovernor {
+        require(_governor != address(0), "Can't set zero address");
         governor = _governor;
         emit GovernorSet(governor);
     }

contracts/Lib/WhitelistedWithGovernanceAndChangableTimelock.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 
 import "./WhitelistedWithGovernance.sol";
 
@@ -10,30 +10,30 @@ contract WhitelistedWithGovernanceAndChangableTimelock is WhitelistedWithGoverna
     event Committed(uint256 timelock);
 
     // Timestamp of last timelock proposal
-    uint256 timelockProposalTime = 0;
+    uint256 public timeLockProposalTime;
     // Proposed timelock
-    uint256 proposedTimelock = 0;
+    uint256 public proposedTimeLock;
 
     /// @notice Calling this function governor could propose new timelock
     /// @param _timelock uint256 New timelock value
     function proposeTimelock(uint256 _timelock) public onlyGovernor {
-        timelockProposalTime = now;
-        proposedTimelock = _timelock;
+        timeLockProposalTime = now;
+        proposedTimeLock = _timelock;
         emit Proposed(_timelock);
     }
 
     /// @notice Calling this function governor could commit previously proposed new timelock if timelock interval of proposal was passed
     function commitTimelock() public onlyGovernor {
         // Check if proposal was made
-        require(timelockProposalTime != 0, "Didn't proposed yet");
+        require(timeLockProposalTime != 0, "Didn't proposed yet");
         // Check if timelock interval was passed
-        require((timelockProposalTime + TIME_LOCK_INTERVAL) < now, "Can't commit yet");
+        require((timeLockProposalTime + timeLockInterval) < now, "Can't commit yet");
 
         // Set new timelock and emit event
-        TIME_LOCK_INTERVAL = proposedTimelock;
-        emit Committed(proposedTimelock);
+        timeLockInterval = proposedTimeLock;
+        emit Committed(proposedTimeLock);
 
         // Reset timelock time lock
-        timelockProposalTime = 0;
+        timeLockProposalTime = 0;
     }
 }

contracts/Matching/Match/LibOrder.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 pragma experimental ABIEncoderV2;
 
 import "../../Lib/LibEIP712.sol";
@@ -103,7 +103,7 @@ contract LibOrder is LibEIP712 {
     /// @notice Hashes the order
     /// @param _order Order Order to hash
     /// @return hash bytes32 Order hash
-    function hashOrder(Order memory _order) internal pure returns (bytes32 hash) {
+    function hashOrder(Order memory _order) public pure returns (bytes32 hash) {
         hash = keccak256(
             abi.encodePacked(
                 abi.encodePacked(

contracts/Matching/Match/Match.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.4;
+pragma solidity 0.5.16;
 pragma experimental ABIEncoderV2;
 
 import "./MatchCreate.sol";
@@ -9,5 +9,5 @@ contract Match is MatchCreate, MatchSwap {
 
     /// @notice Calls constructors of super-contracts
     /// @param _registry address Address of Opium.registry
-    constructor (address _registry) public usingRegistry(_registry) {}
+    constructor (address _registry) public UsingRegistry(_registry) {}
 }