GeneralIndexModule internal review and external audit revisions (#75)

* Make all revert reason strings < 32bytes

* Remove _normalizedTargetMethod getter

* Encapsulate _noTokensToSell#canSell logic as internal method

* Encapsulate _allTargetsMet#targetUnmet logic in internal method

* Validate exchange adapter when setting & store name as bytes32 hash

* Consolidate position state and timestamp updates

* Validate params for external view methods

* Make all external setter method names use prefix `set`

* Add RebalanceStarted event

* Remove _caller param from onlyAllowedTrader modifier

* Add getDefaultPositionRealUnit getter

* Encapsulate data aggregation logic in #startRebalance in own method

* Prohibit external positions for components in #initialize and #startRebalance

* Refactor #createTradeInfo methods 

* Make weth state variable immutable

* Delegate modifier logic to internal helpers in ModuleBase and GeneralIndexModule

* Delete trader permissions on module removal / add removeModule tests

* Add #validatePairsWithArray methods to AddressArrayUtils library and use in setter methods

* Add approximatelyEquals helper in PreciseUnitMath and use in #_targetUnmet

* Add AssetTargetsRaised event in RaiseAssetTargets with positionMultiplier arg

* Remove trader from permissionInfo.tradersHistory when they are de-authorized

* Add getAllowedTraders method and make tradersHistory contain unique elements

Author: cgewecke

contracts/lib/AddressArrayUtils.sol

@@ -23,6 +23,9 @@ pragma solidity 0.6.10;
  * @author Set Protocol
  *
  * Utility functions to handle Address Arrays
+ *
+ * CHANGELOG:
+ * - 4/21/21: Added validatePairsWithArray methods
  */
 library AddressArrayUtils {
 
@@ -74,7 +77,7 @@ library AddressArrayUtils {
 
     /**
      * @param A The input array to search
-     * @param a The address to remove     
+     * @param a The address to remove
      * @return Returns the array with the object removed.
      */
     function remove(address[] memory A, address a)
@@ -149,4 +152,74 @@ library AddressArrayUtils {
         }
         return newAddresses;
     }
-}
+
+    /**
+     * Validate that address and uint array lengths match. Validate address array is not empty
+     * and contains no duplicate elements.
+     *
+     * @param A         Array of addresses
+     * @param B         Array of uint
+     */
+    function validatePairsWithArray(address[] memory A, uint[] memory B) internal pure {
+        require(A.length == B.length, "Array length mismatch");
+        _validateLengthAndUniqueness(A);
+    }
+
+    /**
+     * Validate that address and bool array lengths match. Validate address array is not empty
+     * and contains no duplicate elements.
+     *
+     * @param A         Array of addresses
+     * @param B         Array of bool
+     */
+    function validatePairsWithArray(address[] memory A, bool[] memory B) internal pure {
+        require(A.length == B.length, "Array length mismatch");
+        _validateLengthAndUniqueness(A);
+    }
+
+    /**
+     * Validate that address and string array lengths match. Validate address array is not empty
+     * and contains no duplicate elements.
+     *
+     * @param A         Array of addresses
+     * @param B         Array of strings
+     */
+    function validatePairsWithArray(address[] memory A, string[] memory B) internal pure {
+        require(A.length == B.length, "Array length mismatch");
+        _validateLengthAndUniqueness(A);
+    }
+
+    /**
+     * Validate that address array lengths match, and calling address array are not empty
+     * and contain no duplicate elements.
+     *
+     * @param A         Array of addresses
+     * @param B         Array of addresses
+     */
+    function validatePairsWithArray(address[] memory A, address[] memory B) internal pure {
+        require(A.length == B.length, "Array length mismatch");
+        _validateLengthAndUniqueness(A);
+    }
+
+    /**
+     * Validate that address and bytes array lengths match. Validate address array is not empty
+     * and contains no duplicate elements.
+     *
+     * @param A         Array of addresses
+     * @param B         Array of bytes
+     */
+    function validatePairsWithArray(address[] memory A, bytes[] memory B) internal pure {
+        require(A.length == B.length, "Array length mismatch");
+        _validateLengthAndUniqueness(A);
+    }
+
+    /**
+     * Validate address array is not empty and contains no duplicate elements.
+     *
+     * @param A          Array of addresses
+     */
+    function _validateLengthAndUniqueness(address[] memory A) internal pure {
+        require(A.length > 0, "Array length must be > 0");
+        require(!hasDuplicate(A), "Cannot duplicate addresses");
+    }
+}

contracts/lib/PreciseUnitMath.sol

@@ -32,6 +32,7 @@ import { SignedSafeMath } from "@openzeppelin/contracts/math/SignedSafeMath.sol"
  *
  * CHANGELOG:
  * - 9/21/20: Added safePower function
+ * - 4/21/21: Added approximatelyEquals function
  */
 library PreciseUnitMath {
     using SafeMath for uint256;
@@ -149,16 +150,16 @@ library PreciseUnitMath {
     }
 
     /**
-     * @dev Multiplies value a by value b where rounding is towards the lesser number. 
-     * (positive values are rounded towards zero and negative values are rounded away from 0). 
+     * @dev Multiplies value a by value b where rounding is towards the lesser number.
+     * (positive values are rounded towards zero and negative values are rounded away from 0).
      */
     function conservativePreciseMul(int256 a, int256 b) internal pure returns (int256) {
         return divDown(a.mul(b), PRECISE_UNIT_INT);
     }
 
     /**
-     * @dev Divides value a by value b where rounding is towards the lesser number. 
-     * (positive values are rounded towards zero and negative values are rounded away from 0). 
+     * @dev Divides value a by value b where rounding is towards the lesser number.
+     * (positive values are rounded towards zero and negative values are rounded away from 0).
      */
     function conservativePreciseDiv(int256 a, int256 b) internal pure returns (int256) {
         return divDown(a.mul(PRECISE_UNIT_INT), b);
@@ -187,4 +188,11 @@ library PreciseUnitMath {
 
         return result;
     }
-}
+
+    /**
+     * @dev Returns true if a =~ b within range, false otherwise.
+     */
+    function approximatelyEquals(uint256 a, uint256 b, uint256 range) internal pure returns (bool) {
+        return a <= b.add(range) && a >= b.sub(range);
+    }
+}

contracts/mocks/AddressArrayUtilsMock.sol

@@ -51,11 +51,31 @@ contract AddressArrayUtilsMock {
         return A.pop(index);
     }
 
+    function testValidatePairsWithArrayUint(address[] memory A, uint[] memory a) external view {
+        A.validatePairsWithArray(a);
+    }
+
+    function testValidatePairsWithArrayBool(address[] memory A, bool[] memory a) external view {
+        A.validatePairsWithArray(a);
+    }
+
+    function testValidatePairsWithArrayString(address[] memory A, string[] memory a) external view {
+        A.validatePairsWithArray(a);
+    }
+
+    function testValidatePairsWithArrayAddress(address[] memory A, address[] memory a) external view {
+        A.validatePairsWithArray(a);
+    }
+
+    function testValidatePairsWithArrayBytes(address[] memory A, bytes[] memory a) external view {
+        A.validatePairsWithArray(a);
+    }
+
     function setStorageArray(address[] memory A) external {
         storageArray = A;
     }
 
     function getStorageArray() external view returns(address[] memory) {
         return storageArray;
     }
-}
+}

contracts/mocks/PreciseUnitMathMock.sol

@@ -81,4 +81,8 @@ contract PreciseUnitMathMock {
     function safePower(uint256 a, uint256 b) external pure returns(uint256) {
         return a.safePower(b);
     }
-}
+
+    function approximatelyEquals(uint256 a, uint256 b, uint256 range) external pure returns (bool) {
+        return a.approximatelyEquals(b, range);
+    }
+}

contracts/protocol/lib/ModuleBase.sol

@@ -38,6 +38,10 @@ import { SignedSafeMath } from "@openzeppelin/contracts/math/SignedSafeMath.sol"
  * @author Set Protocol
  *
  * Abstract class that houses common Module-related state and functions.
+ *
+ * CHANGELOG:
+ * - 4/21/21: Delegated modifier logic to internal helpers to reduce contract size
+ *
  */
 abstract contract ModuleBase is IModule {
     using AddressArrayUtils for address[];
@@ -57,35 +61,26 @@ abstract contract ModuleBase is IModule {
 
     /* ============ Modifiers ============ */
 
-    modifier onlyManagerAndValidSet(ISetToken _setToken) { 
-        require(isSetManager(_setToken, msg.sender), "Must be the SetToken manager");
-        require(isSetValidAndInitialized(_setToken), "Must be a valid and initialized SetToken");
+    modifier onlyManagerAndValidSet(ISetToken _setToken) {
+        _validateOnlyManagerAndValidSet(_setToken);
         _;
     }
 
     modifier onlySetManager(ISetToken _setToken, address _caller) {
-        require(isSetManager(_setToken, _caller), "Must be the SetToken manager");
+        _validateOnlySetManager(_setToken, _caller);
         _;
     }
 
     modifier onlyValidAndInitializedSet(ISetToken _setToken) {
-        require(isSetValidAndInitialized(_setToken), "Must be a valid and initialized SetToken");
+        _validateOnlyValidAndInitializedSet(_setToken);
         _;
     }
 
     /**
      * Throws if the sender is not a SetToken's module or module not enabled
      */
     modifier onlyModule(ISetToken _setToken) {
-        require(
-            _setToken.moduleStates(msg.sender) == ISetToken.ModuleState.INITIALIZED,
-            "Only the module can call"
-        );
-
-        require(
-            controller.isModule(msg.sender),
-            "Module must be enabled on controller"
-        );
+        _validateOnlyModule(_setToken);
         _;
     }
 
@@ -94,8 +89,7 @@ abstract contract ModuleBase is IModule {
      * and that the SetToken is valid
      */
     modifier onlyValidAndPendingSet(ISetToken _setToken) {
-        require(controller.isSet(address(_setToken)), "Must be controller-enabled SetToken");
-        require(isSetPendingInitialization(_setToken), "Must be pending initialization");        
+        _validateOnlyValidAndPendingSet(_setToken);
         _;
     }
 
@@ -191,4 +185,53 @@ abstract contract ModuleBase is IModule {
     function getNameHash(string memory _name) internal pure returns(bytes32) {
         return keccak256(bytes(_name));
     }
+
+    /* ============== Modifier Helpers ===============
+     * Internal functions used to reduce bytecode size
+     */
+
+    /**
+     * Caller must SetToken manager and SetToken must be valid and initialized
+     */
+    function _validateOnlyManagerAndValidSet(ISetToken _setToken) internal view {
+       require(isSetManager(_setToken, msg.sender), "Must be the SetToken manager");
+       require(isSetValidAndInitialized(_setToken), "Must be a valid and initialized SetToken");
+    }
+
+    /**
+     * Caller must SetToken manager
+     */
+    function _validateOnlySetManager(ISetToken _setToken, address _caller) internal view {
+        require(isSetManager(_setToken, _caller), "Must be the SetToken manager");
+    }
+
+    /**
+     * SetToken must be valid and initialized
+     */
+    function _validateOnlyValidAndInitializedSet(ISetToken _setToken) internal view {
+        require(isSetValidAndInitialized(_setToken), "Must be a valid and initialized SetToken");
+    }
+
+    /**
+     * Caller must be initialized module and module must be enabled on the controller
+     */
+    function _validateOnlyModule(ISetToken _setToken) internal view {
+        require(
+            _setToken.moduleStates(msg.sender) == ISetToken.ModuleState.INITIALIZED,
+            "Only the module can call"
+        );
+
+        require(
+            controller.isModule(msg.sender),
+            "Module must be enabled on controller"
+        );
+    }
+
+    /**
+     * SetToken must be in a pending state and module must be in pending state
+     */
+    function _validateOnlyValidAndPendingSet(ISetToken _setToken) internal view {
+        require(controller.isSet(address(_setToken)), "Must be controller-enabled SetToken");
+        require(isSetPendingInitialization(_setToken), "Must be pending initialization");
+    }
 }