Custom errors and Gas optimizations (#1)

.gitignore

@@ -15,3 +15,5 @@ artifacts
 cache
 #manticore
 mcore_*
+#secrets
+.env

contracts/CMTAT_PROXY.sol

@@ -1,6 +1,6 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 import "./modules/CMTAT_BASE.sol";
 

contracts/CMTAT_STANDALONE.sol

@@ -1,6 +1,6 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 import "./modules/CMTAT_BASE.sol";
 

contracts/libraries/Errors.sol

@@ -0,0 +1,21 @@
+//SPDX-License-Identifier: MPL-2.0
+
+pragma solidity ^0.8.17;
+
+library Errors {
+    error InvalidTransfer(address from, address to, uint256 amount);
+    error SnapshotScheduledInThePast(uint256 time, uint256 timestamp);
+    error SnapshotTimestampBeforeLastSnapshot(uint256 time, uint256 lastSnapshotTimestamp);
+    error SnapshotTimestampAfterNextSnapshot(uint256 time, uint256 nextSnapshotTimestamp);
+    error SnapshotTimestampBeforePreviousSnapshot(uint256 time, uint256 previousSnapshotTimestamp);
+    error SnapshotAlreadyExists();
+    error SnapshotAlreadyDone();
+    error SnapshotNotScheduled();
+    error SnapshotNotFound();
+    error SnapshotNeverScheduled();
+    error AddressZeroNotAllowed();
+    error DirectCallToImplementation();
+    error WrongAllowance(uint256 allowance, uint256 currentAllowance);
+    error SameValue();
+}
+

contracts/mocks/MinimalForwarderMock.sol

@@ -1,11 +1,12 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
-import "../../openzeppelin-contracts-upgradeable/contracts/metatx/MinimalForwarderUpgradeable.sol";
+import "../../openzeppelin-contracts-upgradeable/contracts/metatx/ERC2771ForwarderUpgradeable.sol";
 
-contract MinimalForwarderMock is MinimalForwarderUpgradeable {
-    function initialize() public initializer {
-        __MinimalForwarder_init();
+contract MinimalForwarderMock is ERC2771ForwarderUpgradeable {
+    function initialize(string memory name) public initializer {
+        __EIP712_init_unchained(name, "1");
+        __ERC2771Forwarder_init_unchained("");
     }
 }

contracts/mocks/RuleEngine/CodeList.sol

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 abstract contract CodeList {
     // Used by RuleMock.sol

contracts/mocks/RuleEngine/RuleEngineMock.sol

@@ -1,6 +1,6 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 import "./interfaces/IRule.sol";
 import "./interfaces/IRuleEngine.sol";
@@ -43,7 +43,7 @@ contract RuleEngineMock is IRuleEngine {
         uint256 _amount
     ) public view override returns (uint8) {
         uint256 ruleArrayLength = _rules.length;
-        for (uint256 i = 0; i < ruleArrayLength; ++i) {
+        for (uint256 i; i < ruleArrayLength;) {
             uint8 restriction = _rules[i].detectTransferRestriction(
                 _from,
                 _to,
@@ -52,6 +52,7 @@ contract RuleEngineMock is IRuleEngine {
             if (restriction != uint8(REJECTED_CODE_BASE.TRANSFER_OK)) {
                 return restriction;
             }
+            unchecked { ++i; }
         }
         return uint8(REJECTED_CODE_BASE.TRANSFER_OK);
     }
@@ -72,11 +73,12 @@ contract RuleEngineMock is IRuleEngine {
         uint8 _restrictionCode
     ) public view override returns (string memory) {
         uint256 ruleArrayLength = _rules.length;
-        for (uint256 i = 0; i < ruleArrayLength; ++i) {
+        for (uint256 i; i < ruleArrayLength;) {
             if (_rules[i].canReturnTransferRestrictionCode(_restrictionCode)) {
                 return
                     _rules[i].messageForTransferRestriction(_restrictionCode);
             }
+            unchecked { ++i; }
         }
         return "Unknown restriction code";
     }

contracts/mocks/RuleEngine/RuleMock.sol

@@ -1,6 +1,6 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 import "./interfaces/IRule.sol";
 import "./CodeList.sol";

contracts/modules/CMTAT_BASE.sol

@@ -1,6 +1,6 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 // required OZ imports here
 import "../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol";
@@ -24,6 +24,8 @@ import "./wrapper/optional/DebtModule/CreditEventsModule.sol";
 import "./security/AuthorizationModule.sol";
 import "../interfaces/IEIP1404/IEIP1404Wrapper.sol";
 
+import "../libraries/Errors.sol";
+
 abstract contract CMTAT_BASE is
     Initializable,
     ContextUpgradeable,
@@ -174,20 +176,20 @@ abstract contract CMTAT_BASE is
     * e.g. override(SnapshotModuleInternal, ERC20Upgradeable)
     * - remove the keyword view
     */
-    function _beforeTokenTransfer(
+    function _update(
         address from,
         address to,
         uint256 amount
-    ) internal view override(ERC20Upgradeable) {
-        require(
-            ValidationModule.validateTransfer(from, to, amount),
-            "CMTAT: transfer rejected by validation module"
-        );
+    ) internal override(ERC20Upgradeable) {
+        if(!ValidationModule.validateTransfer(from, to, amount)) {
+            revert Errors.InvalidTransfer(from, to, amount);
+        }
+        ERC20Upgradeable._update(from, to, amount);
         // We call the SnapshotModule only if the transfer is valid
         /*
         SnapshotModule:
         Add this call in case you add the SnapshotModule
-        SnapshotModuleInternal._beforeTokenTransfer(from, to, amount);
+        SnapshotModuleInternal._update(from, to, amount);
         */
     }
 

contracts/modules/internal/EnforcementModuleInternal.sol

@@ -1,6 +1,6 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 import "../../../openzeppelin-contracts-upgradeable/contracts/utils/ContextUpgradeable.sol";
 import "../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol";
@@ -64,7 +64,7 @@ abstract contract EnforcementModuleInternal is
      */
     function _freeze(
         address account,
-        string memory reason
+        string calldata reason
     ) internal virtual returns (bool) {
         if (_frozen[account]) return false;
         _frozen[account] = true;
@@ -79,7 +79,7 @@ abstract contract EnforcementModuleInternal is
      */
     function _unfreeze(
         address account,
-        string memory reason
+        string calldata reason
     ) internal virtual returns (bool) {
         if (!_frozen[account]) return false;
         _frozen[account] = false;

contracts/modules/internal/SnapshotModuleInternal.sol

@@ -1,12 +1,14 @@
 //SPDX-License-Identifier: MPL-2.0
 
-pragma solidity ^0.8.17;
+pragma solidity ^0.8.20;
 
 import "../../../openzeppelin-contracts-upgradeable/contracts/utils/ContextUpgradeable.sol";
 import "../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol";
 import "../../../openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol";
 import "../../../openzeppelin-contracts-upgradeable/contracts/utils/ArraysUpgradeable.sol";
 
+import "../../libraries/Errors.sol";
+
 /**
  * @dev Snapshot module.
  *
@@ -63,8 +65,8 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
      * @dev Initializes the contract
      */
     function __Snapshot_init(
-        string memory name_,
-        string memory symbol_
+        string calldata name_,
+        string calldata symbol_
     ) internal onlyInitializing {
         __Context_init_unchained();
         __ERC20_init(name_, symbol_);
@@ -82,14 +84,13 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
     */
     function _scheduleSnapshot(uint256 time) internal {
         // Check the time firstly to avoid an useless read of storage
-        require(time > block.timestamp, "Snapshot scheduled in the past");
+        if(time <= block.timestamp) revert Errors.SnapshotScheduledInThePast(time, block.timestamp);
 
         if (_scheduledSnapshots.length > 0) {
             // We check the last snapshot on the list
-            require(
-                time > _scheduledSnapshots[_scheduledSnapshots.length - 1],
-                "time has to be greater than the last snapshot time"
-            );
+            if(time <= _scheduledSnapshots[_scheduledSnapshots.length - 1]) {
+                revert Errors.SnapshotTimestampBeforeLastSnapshot(time, _scheduledSnapshots[_scheduledSnapshots.length - 1]);
+            }
         }
         _scheduledSnapshots.push(time);
         emit SnapshotSchedule(0, time);
@@ -99,18 +100,18 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
     @dev schedule a snapshot at the specified time
     */
     function _scheduleSnapshotNotOptimized(uint256 time) internal {
-        require(time > block.timestamp, "Snapshot scheduled in the past");
+        if(time <= block.timestamp) revert Errors.SnapshotScheduledInThePast(time, block.timestamp);
         (bool isFound, uint256 index) = _findScheduledSnapshotIndex(time);
         // Perfect match
-        require(!isFound, "Snapshot already exists");
+        if(isFound) revert Errors.SnapshotAlreadyExists();
         // if no upper bound match found, we push the snapshot at the end of the list
         if (index == _scheduledSnapshots.length) {
             _scheduledSnapshots.push(time);
         } else {
             _scheduledSnapshots.push(
                 _scheduledSnapshots[_scheduledSnapshots.length - 1]
             );
-            for (uint256 i = _scheduledSnapshots.length - 2; i > index; ) {
+            for (uint256 i = _scheduledSnapshots.length - 2; i > index;) {
                 _scheduledSnapshots[i] = _scheduledSnapshots[i - 1];
                 unchecked {
                     --i;
@@ -126,25 +127,19 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
     */
     function _rescheduleSnapshot(uint256 oldTime, uint256 newTime) internal {
         // Check the time firstly to avoid an useless read of storage
-        require(oldTime > block.timestamp, "Snapshot already done");
-        require(newTime > block.timestamp, "Snapshot scheduled in the past");
-        require(_scheduledSnapshots.length > 0, "no scheduled snapshot");
+        if(oldTime <= block.timestamp) revert Errors.SnapshotAlreadyDone();
+        if(newTime <= block.timestamp) revert Errors.SnapshotScheduledInThePast(newTime, block.timestamp);
+        if(_scheduledSnapshots.length == 0) revert Errors.SnapshotNotScheduled();
 
         (bool foundOld, uint256 index) = _findScheduledSnapshotIndex(oldTime);
-        require(foundOld, "Snapshot not found");
+        if(!foundOld) revert Errors.SnapshotNotFound();
 
         if (index + 1 < _scheduledSnapshots.length) {
-            require(
-                newTime < _scheduledSnapshots[index + 1],
-                "time has to be less than the next snapshot"
-            );
+            if(newTime >= _scheduledSnapshots[index + 1]) revert Errors.SnapshotTimestampAfterNextSnapshot(newTime, _scheduledSnapshots[index + 1]);
         }
 
         if (index > 0) {
-            require(
-                newTime > _scheduledSnapshots[index - 1],
-                "time has to be greater than the previous snapshot"
-            );
+            if(newTime <= _scheduledSnapshots[index - 1]) revert Errors.SnapshotTimestampBeforePreviousSnapshot(newTime, _scheduledSnapshots[index - 1]);
         }
 
         _scheduledSnapshots[index] = newTime;
@@ -157,13 +152,10 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
     */
     function _unscheduleLastSnapshot(uint256 time) internal {
         // Check the time firstly to avoid an useless read of storage
-        require(time > block.timestamp, "Snapshot already done");
-        require(_scheduledSnapshots.length > 0, "No snapshot scheduled");
+        if(time <= block.timestamp) revert Errors.SnapshotAlreadyDone();
+        if(_scheduledSnapshots.length == 0) revert Errors.SnapshotNotScheduled();
         // All snapshot time are unique, so we do not check the indice
-        require(
-            time == _scheduledSnapshots[_scheduledSnapshots.length - 1],
-            "Only the last snapshot can be unscheduled"
-        );
+        if(time != _scheduledSnapshots[_scheduledSnapshots.length - 1]) revert Errors.SnapshotNeverScheduled();
         _scheduledSnapshots.pop();
         emit SnapshotUnschedule(time);
     }
@@ -175,10 +167,10 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
     - Reduce the array size by deleting the last snapshot
     */
     function _unscheduleSnapshotNotOptimized(uint256 time) internal {
-        require(time > block.timestamp, "Snapshot already done");
+        if(time <= block.timestamp) revert Errors.SnapshotAlreadyDone();
         (bool isFound, uint256 index) = _findScheduledSnapshotIndex(time);
-        require(isFound, "Snapshot not found");
-        for (uint256 i = index; i + 1 < _scheduledSnapshots.length; ) {
+        if(!isFound) revert Errors.SnapshotNotFound();
+        for (uint256 i = index; i + 1 < _scheduledSnapshots.length;) {
             _scheduledSnapshots[i] = _scheduledSnapshots[i + 1];
             unchecked {
                 ++i;
@@ -210,10 +202,11 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
                         indexLowerBound -
                         1;
                     nextScheduledSnapshot = new uint256[](arraySize);
-                    for (uint256 i = 0; i < nextScheduledSnapshot.length; ++i) {
+                    for (uint256 i; i < arraySize;) {
                         nextScheduledSnapshot[i] = _scheduledSnapshots[
                             indexLowerBound + 1 + i
                         ];
+                        unchecked { ++i; }
                     }
                 }
             }
@@ -262,12 +255,12 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
     @dev Update balance and/or total supply snapshots before the values are modified. This is implemented
     in the _beforeTokenTransfer hook, which is executed for _mint, _burn, and _transfer operations.
     */
-    function _beforeTokenTransfer(
+    function _update(
         address from,
         address to,
         uint256 amount
     ) internal virtual override {
-        super._beforeTokenTransfer(from, to, amount);
+        super._update(from, to, amount);
 
         _setCurrentSnapshot();
         if (from != address(0)) {
@@ -389,20 +382,21 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
         uint256 time
     ) private view returns (bool, uint256) {
         uint256 indexFound = _scheduledSnapshots.findUpperBound(time);
+        uint256 _scheduledSnapshotsLength = _scheduledSnapshots.length;
         // Exact match
         if (
-            indexFound != _scheduledSnapshots.length &&
+            indexFound != _scheduledSnapshotsLength &&
             _scheduledSnapshots[indexFound] == time
         ) {
             return (true, indexFound);
         }
         // Upper bound match
-        else if (indexFound != _scheduledSnapshots.length) {
+        else if (indexFound != _scheduledSnapshotsLength) {
             return (false, indexFound);
         }
         // no match
         else {
-            return (false, _scheduledSnapshots.length);
+            return (false, _scheduledSnapshotsLength);
         }
     }
 
@@ -423,16 +417,17 @@ abstract contract SnapshotModuleInternal is ERC20Upgradeable {
         ) {
             return (0, currentArraySize);
         }
-        uint256 mostRecent = 0;
+        uint256 mostRecent;
         index = currentArraySize;
-        for (uint256 i = _currentSnapshotIndex; i < currentArraySize; ++i) {
+        for (uint256 i = _currentSnapshotIndex; i < currentArraySize;) {
             if (_scheduledSnapshots[i] <= block.timestamp) {
                 mostRecent = _scheduledSnapshots[i];
                 index = i;
             } else {
                 // All snapshot are planned in the futur
                 break;
             }
+            unchecked { ++i; }
         }
         return (mostRecent, index);
     }