perf!: replace error strings by custom errors + use enum for `OPERA…

…TION_TYPE` (#175)

* chore: improve docs comments and variable names

* refactor!: use enum for operation type

* build: add gas reporter configs

* refactor!: change revert reason errors with custom errors

* refactor: add custom error for ERC725Y + extract error to separate file

* chore: apply prettier

* chore: remove section comments

* refactor: add type casting in events

* docs: improve docs for operation type and contract creation

* perf: save gas on deployment by removing unused params in internal functions

* perf: remove modifier + inline constructor/initializer check for `newOwner != address(0)`

* test: add test for contract deployment code not provided

* docs: rename variables from `data` to `creationCode`

* docs: fix order constructor + runtime code

* docs: use term "creationCode" for consistency

* revert: use enum for operation type

docs/ERC-725.md

@@ -53,29 +53,37 @@ MUST revert when the execution fails.
 
 _Parameters:_
 
-- `operationType`: the operation to execute.
-- `to`: the smart contract or address to interact with. `to` will be unused if a contract is created (operation 1 and 2).
+- `operationType`: the operation type used to execute.
+- `to`: the smart contract or address to call. `to` will be unused if a contract is created (operation types 1 and 2).
 - `value`: the amount of native tokens to transfer (in Wei).
-- `data`: the call data, or the bytecode of the contract to deploy.
+- `data`: the call data, or the creation bytecode of the contract to deploy.
 
 #### data parameter
 
 - For operationType, `CALL`, `STATICCALL` and `DELEGATECALL` the data field can be random bytes or an abi-encoded function call.
 
-- For operationType, `CREATE` the data field is the bytecode of the contract to deploy appended with the constructor argument abi-encoded.
+- For operationType, `CREATE` the `data` field is the creation bytecode of the contract to deploy appended with the constructor argument(s) abi-encoded.
 
-- For operationType, `CREATE2` the data field is the bytecode of the contract to deploy appended with the constructor argument abi-encoded appended with a bytes32 salt. Check [EIP-1014: Skinny CREATE2](https://eips.ethereum.org/EIPS/eip-1014) for more information.
+- For operationType, `CREATE2` the `data` field is the creation bytecode of the contract to deploy appended with:
+    1. the constructor argument(s) abi-encoded
+    2. a bytes32 salt. 
+
+```
+data = <contract-creation-code> + <abi-encoded-constructor-arguments> + <bytes32-salt>
+```
+
+> Check [EIP-1014: Skinny CREATE2](https://eips.ethereum.org/EIPS/eip-1014) for more information.
   
 
-_Returns:_ `bytes` , the returned data of the called function, or the address of the contract created (operation 1 and 2).
+_Returns:_ `bytes` , the returned data of the called function, or the address of the contract deployed (operation types 1 and 2).
 
 The following `operationType` MUST exist:
 
 - `0` for `CALL`
 - `1` for `CREATE`
 - `2` for `CREATE2`
 - `3` for `STATICCALL`
-- `4` for `DELEGATECALL` - **NOTE** This is a potentially dangerous operation
+- `4` for `DELEGATECALL` - **NOTE** This is a potentially dangerous operation type
 
 Others may be added in the future.
 
@@ -88,15 +96,15 @@ Others may be added in the future.
 #### Executed
 
 ```solidity
-event Executed(uint256 indexed operation, address indexed to, uint256 indexed _value, bytes4 data);
+event Executed(uint256 indexed operationType, address indexed to, uint256 indexed _value, bytes4 data);
 ```
 
 MUST be triggered when `execute` creates a new call using the `operationType` `0`, `3`, `4`.
 
 #### ContractCreated
 
 ```solidity
-event ContractCreated(uint256 indexed operation, address indexed contractAddress, uint256 indexed value);
+event ContractCreated(uint256 indexed operationType, address indexed contractAddress, uint256 indexed value);
 ```
 
 MUST be triggered when `execute` creates a new contract using the `operationType` `1`, `2`.
@@ -214,7 +222,7 @@ Reference implementations can be found [here](../assets/eip-725)
 
 This contract allows generic executions, therefore special care need to be take care to prevent re-entrancy attacks and other forms for call chain attacks.
 
-When using the operator `4` for the `delegatecall` operation, its important to consider that called contracts can alter the state of the calling contract and also change owner variables at will. Additionally calls to `selfdestruct` are possible and other harmful state changing operations.
+When using the operation type `4` for `delegatecall`, it is important to consider that called contracts can alter the state of the calling contract and also change owner variables at will. Additionally calls to `selfdestruct` are possible and other harmful state changing operations.
 
 ### Solidity Interfaces
 
@@ -224,8 +232,8 @@ pragma solidity >=0.5.0 <0.7.0;
 
 // ERC165 identifier: `0x44c028fe`
 interface IERC725X  /* is ERC165, ERC173 */ {
-    event ContractCreated(uint256 indexed operation, address indexed contractAddress, uint256 indexed value);
-    event Executed(uint256 indexed operation, address indexed to, uint256 indexed  value, bytes4 data);
+    event ContractCreated(uint256 indexed operationType, address indexed contractAddress, uint256 indexed value);
+    event Executed(uint256 indexed operationType, address indexed to, uint256 indexed  value, bytes4 data);
 
     function execute(uint256 operationType, address to, uint256 value, bytes memory data) external payable returns(bytes memory);
 }

implementations/contracts/ERC725.sol

@@ -20,14 +20,13 @@ contract ERC725 is ERC725XCore, ERC725YCore {
      * @notice Sets the owner of the contract
      * @param newOwner the owner of the contract
      */
-    constructor(address newOwner) notZeroAddressAsOwner(newOwner) {
+    constructor(address newOwner) {
+        require(newOwner != address(0), "Ownable: new owner is the zero address");
         OwnableUnset._setOwner(newOwner);
     }
 
     // NOTE this implementation has not by default: receive() external payable {}
 
-    /* Overrides functions */
-
     /**
      * @inheritdoc ERC165
      */

implementations/contracts/ERC725InitAbstract.sol

@@ -21,15 +21,13 @@ abstract contract ERC725InitAbstract is Initializable, ERC725XCore, ERC725YCore
         internal
         virtual
         onlyInitializing
-        notZeroAddressAsOwner(newOwner)
     {
+        require(newOwner != address(0), "Ownable: new owner is the zero address");
         OwnableUnset._setOwner(newOwner);
     }
 
     // NOTE this implementation has not by default: receive() external payable {}
 
-    /* Overrides functions */
-
     /**
      * @inheritdoc ERC165
      */

implementations/contracts/ERC725X.sol

@@ -17,7 +17,8 @@ contract ERC725X is ERC725XCore {
      * @notice Sets the owner of the contract
      * @param newOwner the owner of the contract
      */
-    constructor(address newOwner) notZeroAddressAsOwner(newOwner) {
+    constructor(address newOwner) {
+        require(newOwner != address(0), "Ownable: new owner is the zero address");
         OwnableUnset._setOwner(newOwner);
     }
 }

implementations/contracts/ERC725XCore.sol

@@ -15,18 +15,19 @@ import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
 import {OwnableUnset} from "./custom/OwnableUnset.sol";
 
 // constants
-// prettier-ignore
 import {
-    _INTERFACEID_ERC725X,
-    OPERATION_CALL, 
-    OPERATION_DELEGATECALL, 
-    OPERATION_STATICCALL, 
-    OPERATION_CREATE, 
-    OPERATION_CREATE2
+    _INTERFACEID_ERC725X, 
+    OPERATION_0_CALL,
+    OPERATION_1_CREATE,
+    OPERATION_2_CREATE2,
+    OPERATION_3_STATICCALL,
+    OPERATION_4_DELEGATECALL
 } from "./constants.sol";
 
+import "./errors.sol";
+
 /**
- * @title Core implementation of ERC725 X executor
+ * @title Core implementation of ERC725X executor
  * @author Fabian Vogelsteller <fabian@lukso.network>
  * @dev Implementation of a contract module which provides the ability to call arbitrary functions at any other smart contract and itself,
  * including using `delegatecall`, `staticcall` as well creating contracts using `create` and `create2`
@@ -37,17 +38,17 @@ abstract contract ERC725XCore is OwnableUnset, ERC165, IERC725X {
      * @inheritdoc IERC725X
      */
     function execute(
-        uint256 operation,
+        uint256 operationType,
         address to,
         uint256 value,
         bytes memory data
     ) public payable virtual onlyOwner returns (bytes memory) {
-        require(address(this).balance >= value, "ERC725X: insufficient balance");
-        return _execute(operation, to, value, data);
+        if (address(this).balance < value) {
+            revert ERC725X_InsufficientBalance(address(this).balance, value);
+        }
+        return _execute(operationType, to, value, data);
     }
 
-    /* Overrides functions */
-
     /**
      * @inheritdoc ERC165
      */
@@ -61,25 +62,38 @@ abstract contract ERC725XCore is OwnableUnset, ERC165, IERC725X {
         return interfaceId == _INTERFACEID_ERC725X || super.supportsInterface(interfaceId);
     }
 
-    /* Internal functions */
-
+    /**
+     * @dev check the `operationType` provided and perform the associated low-level opcode.
+     * see `IERC725X.execute(...)`.
+     */
     function _execute(
-        uint256 operation,
+        uint256 operationType,
         address to,
         uint256 value,
         bytes memory data
     ) internal virtual returns (bytes memory) {
         // CALL
-        if (operation == OPERATION_CALL) return _executeCall(to, value, data);
+        if (operationType == OPERATION_0_CALL) {
+            return _executeCall(to, value, data);
+        }
 
         // Deploy with CREATE
-        if (operation == OPERATION_CREATE) return _deployCreate(to, value, data);
+        if (operationType == uint256(OPERATION_1_CREATE)) {
+            if (to != address(0)) revert ERC725X_CreateOperationsRequireEmptyRecipientAddress();
+            return _deployCreate(value, data);
+        }
 
         // Deploy with CREATE2
-        if (operation == OPERATION_CREATE2) return _deployCreate2(to, value, data);
+        if (operationType == uint256(OPERATION_2_CREATE2)) {
+            if (to != address(0)) revert ERC725X_CreateOperationsRequireEmptyRecipientAddress();
+            return _deployCreate2(value, data);
+        }
 
         // STATICCALL
-        if (operation == OPERATION_STATICCALL) return _executeStaticCall(to, value, data);
+        if (operationType == uint256(OPERATION_3_STATICCALL)) {
+            if (value != 0) revert ERC725X_MsgValueDisallowedInStaticCall();
+            return _executeStaticCall(to, data);
+        }
 
         // DELEGATECALL
         //
@@ -93,13 +107,16 @@ abstract contract ERC725XCore is OwnableUnset, ERC165, IERC725X {
         // - update the contract owner
         // - run selfdestruct in the context of this contract
         //
-        if (operation == OPERATION_DELEGATECALL) return _executeDelegateCall(to, value, data);
+        if (operationType == uint256(OPERATION_4_DELEGATECALL)) {
+            if (value != 0) revert ERC725X_MsgValueDisallowedInDelegateCall();
+            return _executeDelegateCall(to, data);
+        }
 
-        revert("ERC725X: Unknown operation type");
+        revert ERC725X_UnknownOperationType(operationType);
     }
 
     /**
-     * @dev perform call using operation 0
+     * @dev perform low-level call (operation type = 0)
      * @param to The address on which call is executed
      * @param value The value to be sent with the call
      * @param data The data to be sent with the call
@@ -110,108 +127,94 @@ abstract contract ERC725XCore is OwnableUnset, ERC165, IERC725X {
         uint256 value,
         bytes memory data
     ) internal virtual returns (bytes memory result) {
-        emit Executed(OPERATION_CALL, to, value, bytes4(data));
+        emit Executed(OPERATION_0_CALL, to, value, bytes4(data));
 
         // solhint-disable avoid-low-level-calls
         (bool success, bytes memory returnData) = to.call{value: value}(data);
         result = Address.verifyCallResult(success, returnData, "ERC725X: Unknown Error");
     }
 
     /**
-     * @dev perform staticcall using operation 3
+     * @dev perform low-level staticcall (operation type = 3)
      * @param to The address on which staticcall is executed
-     * @param value The value passed to the execute(...) function (MUST be 0)
      * @param data The data to be sent with the staticcall
      * @return result The data returned from the staticcall
      */
     function _executeStaticCall(
         address to,
-        uint256 value,
         bytes memory data
     ) internal virtual returns (bytes memory result) {
-        require(value == 0, "ERC725X: cannot transfer value with operation STATICCALL");
-
-        emit Executed(OPERATION_STATICCALL, to, value, bytes4(data));
+        emit Executed(OPERATION_3_STATICCALL, to, 0, bytes4(data));
 
         // solhint-disable avoid-low-level-calls
         (bool success, bytes memory returnData) = to.staticcall(data);
         result = Address.verifyCallResult(success, returnData, "ERC725X: Unknown Error");
     }
 
     /**
-     * @dev perform delegatecall using operation 4
+     * @dev perform low-level delegatecall (operation type = 4)
      * @param to The address on which delegatecall is executed
-     * @param value The value passed to the execute(...) function (MUST be 0)
      * @param data The data to be sent with the delegatecall
      * @return result The data returned from the delegatecall
      */
     function _executeDelegateCall(
         address to,
-        uint256 value,
         bytes memory data
     ) internal virtual returns (bytes memory result) {
-        require(value == 0, "ERC725X: cannot transfer value with operation DELEGATECALL");
-
-        emit Executed(OPERATION_DELEGATECALL, to, value, bytes4(data));
+        emit Executed(OPERATION_4_DELEGATECALL, to, 0, bytes4(data));
 
         // solhint-disable avoid-low-level-calls
         (bool success, bytes memory returnData) = to.delegatecall(data);
         result = Address.verifyCallResult(success, returnData, "ERC725X: Unknown Error");
     }
 
     /**
-     * @dev perform contract creation using operation 1
-     * @param to The recipient address passed to execute(...) (MUST be address(0) for CREATE)
+     * @dev deploy a contract using the CREATE opcode (operation type = 1)
      * @param value The value to be sent to the contract created
-     * @param data The contract bytecode to deploy
+     * @param creationCode The contract creation bytecode to deploy appended with the constructor argument(s)
      * @return newContract The address of the contract created as bytes
      */
     function _deployCreate(
-        address to,
         uint256 value,
-        bytes memory data
+        bytes memory creationCode
     ) internal virtual returns (bytes memory newContract) {
-        require(
-            to == address(0),
-            "ERC725X: CREATE operations require the receiver address to be empty"
-        );
-        require(data.length != 0, "ERC725X: No contract bytecode provided");
+        if (creationCode.length == 0) {
+            revert ERC725X_NoContractBytecodeProvided();
+        }
 
         address contractAddress;
         // solhint-disable no-inline-assembly
         assembly {
-            contractAddress := create(value, add(data, 0x20), mload(data))
+            contractAddress := create(value, add(creationCode, 0x20), mload(creationCode))
         }
 
-        require(contractAddress != address(0), "ERC725X: Could not deploy contract");
+        if (contractAddress == address(0)) {
+            revert ERC725X_ContractDeploymentFailed();
+        }
 
         newContract = abi.encodePacked(contractAddress);
-        emit ContractCreated(OPERATION_CREATE, contractAddress, value);
+        emit ContractCreated(OPERATION_1_CREATE, contractAddress, value);
     }
 
     /**
-     * @dev perform contract creation using operation 2
-     * @param to The recipient address passed to execute(...) (MUST be address(0) for CREATE2)
+     * @dev deploy a contract using the CREATE2 opcode (operation type = 2)
      * @param value The value to be sent to the contract created
-     * @param data The contract bytecode to deploy appended with a bytes32 salt
+     * @param creationCode The contract creation bytecode to deploy appended with the constructor argument(s) and a bytes32 salt
      * @return newContract The address of the contract created as bytes
      */
     function _deployCreate2(
-        address to,
         uint256 value,
-        bytes memory data
+        bytes memory creationCode
     ) internal virtual returns (bytes memory newContract) {
-        require(
-            to == address(0),
-            "ERC725X: CREATE operations require the receiver address to be empty"
-        );
-        require(data.length != 0, "ERC725X: No contract bytecode provided");
-
-        bytes32 salt = BytesLib.toBytes32(data, data.length - 32);
-        bytes memory bytecode = BytesLib.slice(data, 0, data.length - 32);
+        if (creationCode.length == 0) {
+            revert ERC725X_NoContractBytecodeProvided();
+        }
+
+        bytes32 salt = BytesLib.toBytes32(creationCode, creationCode.length - 32);
+        bytes memory bytecode = BytesLib.slice(creationCode, 0, creationCode.length - 32);
         address contractAddress = Create2.deploy(value, salt, bytecode);
 
         newContract = abi.encodePacked(contractAddress);
-        emit ContractCreated(OPERATION_CREATE2, contractAddress, value);
+        emit ContractCreated(OPERATION_2_CREATE2, contractAddress, value);
     }
 }

implementations/contracts/ERC725XInitAbstract.sol

@@ -18,8 +18,8 @@ abstract contract ERC725XInitAbstract is Initializable, ERC725XCore {
         internal
         virtual
         onlyInitializing
-        notZeroAddressAsOwner(newOwner)
     {
+        require(newOwner != address(0), "Ownable: new owner is the zero address");
         OwnableUnset._setOwner(newOwner);
     }
 }

implementations/contracts/ERC725Y.sol

@@ -18,7 +18,8 @@ contract ERC725Y is ERC725YCore {
      * @notice Sets the owner of the contract
      * @param newOwner the owner of the contract
      */
-    constructor(address newOwner) notZeroAddressAsOwner(newOwner) {
+    constructor(address newOwner) {
+        require(newOwner != address(0), "Ownable: new owner is the zero address");
         OwnableUnset._setOwner(newOwner);
     }
 }