Refactor Schemes: increased coverage, replaced SafeMath and added custom errors

contracts/dao/schemes/AvatarScheme.sol

@@ -13,6 +13,27 @@ import "./Scheme.sol";
 contract AvatarScheme is Scheme {
     using Address for address;
 
+    /// @notice Emitted when the proposal is already being executed
+    error AvatarScheme__ProposalExecutionAlreadyRunning();
+
+    /// @notice Emitted when the proposal wasn't submitted
+    error AvatarScheme__ProposalMustBeSubmitted();
+
+    /// @notice Emitted when the call to setETHPermissionUsed fails
+    error AvatarScheme__SetEthPermissionUsedFailed();
+
+    /// @notice Emitted when the avatarCall failed. Returns the revert error
+    error AvatarScheme__AvatarCallFailed(string reason);
+
+    /// @notice Emitted when exceeded the maximum rep supply % change
+    error AvatarScheme__MaxRepPercentageChangePassed();
+
+    /// @notice Emitted when ERC20 limits passed
+    error AvatarScheme__ERC20LimitsPassed();
+
+    /// @notice Emitted if the number of totalOptions is not 2
+    error AvatarScheme__TotalOptionsMustBeTwo();
+
     /**
      * @dev Propose calls to be executed, the calls have to be allowed by the permission registry
      * @param _to - The addresses to call
@@ -31,7 +52,10 @@ contract AvatarScheme is Scheme {
         string calldata _title,
         string calldata _descriptionHash
     ) public override returns (bytes32 proposalId) {
-        require(_totalOptions == 2, "AvatarScheme: The total amount of options should be 2");
+        if (_totalOptions != 2) {
+            revert AvatarScheme__TotalOptionsMustBeTwo();
+        }
+
         return super.proposeCalls(_to, _callData, _value, _totalOptions, _title, _descriptionHash);
     }
 
@@ -48,11 +72,15 @@ contract AvatarScheme is Scheme {
         returns (bool)
     {
         // We use isExecutingProposal variable to avoid re-entrancy in proposal execution
-        require(!executingProposal, "AvatarScheme: proposal execution already running");
+        if (executingProposal) {
+            revert AvatarScheme__ProposalExecutionAlreadyRunning();
+        }
         executingProposal = true;
 
         Proposal storage proposal = proposals[_proposalId];
-        require(proposal.state == ProposalState.Submitted, "AvatarScheme: must be a submitted proposal");
+        if (proposal.state != ProposalState.Submitted) {
+            revert AvatarScheme__ProposalMustBeSubmitted();
+        }
 
         if ((proposal.submittedTime + maxSecondsForExecution) < block.timestamp) {
             // If the amount of time passed since submission plus max proposal time is lower than block timestamp
@@ -93,7 +121,7 @@ contract AvatarScheme is Scheme {
                     (callDataFuncSignature == bytes4(keccak256("mintReputation(uint256,address)")) ||
                         callDataFuncSignature == bytes4(keccak256("burnReputation(uint256,address)")))
                 ) {
-                    (callsSucessResult, ) = address(controller).call(proposal.callData[callIndex]);
+                    (callsSucessResult, returnData) = address(controller).call(proposal.callData[callIndex]);
                 } else {
                     // The permission registry keeps track of all value transferred and checks call permission
                     (callsSucessResult, returnData) = controller.avatarCall(
@@ -108,25 +136,34 @@ contract AvatarScheme is Scheme {
                         avatar,
                         0
                     );
-                    require(callsSucessResult, "AvatarScheme: setETHPermissionUsed failed");
-
+                    if (!callsSucessResult) {
+                        revert AvatarScheme__SetEthPermissionUsedFailed();
+                    }
                     (callsSucessResult, returnData) = controller.avatarCall(
                         proposal.to[callIndex],
                         proposal.callData[callIndex],
                         avatar,
                         proposal.value[callIndex]
                     );
                 }
-                require(callsSucessResult, string(returnData));
+
+                if (!callsSucessResult) {
+                    revert AvatarScheme__AvatarCallFailed({reason: string(returnData)});
+                }
             }
+
             // Cant mint or burn more REP than the allowed percentaged set in the wallet scheme initialization
-            require(
-                ((oldRepSupply * (uint256(100) + maxRepPercentageChange)) / 100 >= getNativeReputationTotalSupply()) &&
-                    ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 <=
-                        getNativeReputationTotalSupply()),
-                "AvatarScheme: maxRepPercentageChange passed"
-            );
-            require(permissionRegistry.checkERC20Limits(address(avatar)), "AvatarScheme: ERC20 limits passed");
+
+            if (
+                ((oldRepSupply * (uint256(100) + maxRepPercentageChange)) / 100 < getNativeReputationTotalSupply()) ||
+                ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 > getNativeReputationTotalSupply())
+            ) {
+                revert AvatarScheme__MaxRepPercentageChangePassed();
+            }
+
+            if (!permissionRegistry.checkERC20Limits(address(avatar))) {
+                revert AvatarScheme__ERC20LimitsPassed();
+            }
         }
         controller.endProposal(_proposalId);
         executingProposal = false;

contracts/dao/schemes/Scheme.sol

@@ -61,6 +61,42 @@ abstract contract Scheme is DXDVotingMachineCallbacks {
 
     event ProposalStateChange(bytes32 indexed _proposalId, uint256 indexed _state);
 
+    /// @notice Emitted when its initialized twice
+    error Scheme__CannotInitTwice();
+
+    /// @notice Emitted if avatar address is zero
+    error Scheme__AvatarAddressCannotBeZero();
+
+    /// @notice Emitted if controller address is zero
+    error Scheme__ControllerAddressCannotBeZero();
+
+    /// @notice Emitted if maxSecondsForExecution is set lower than 86400
+    error Scheme__MaxSecondsForExecutionTooLow();
+
+    /// @notice Emitted when setMaxSecondsForExecution is being called from an address different than the avatar or the scheme
+    error Scheme__SetMaxSecondsForExecutionInvalidCaller();
+
+    /// @notice _to, _callData and _value must have all the same length
+    error Scheme_InvalidParameterArrayLength();
+
+    /// @notice Emitted when the totalOptions paramers is invalid
+    error Scheme__InvalidTotalOptionsOrActionsCallsLength();
+
+    /// @notice Emitted when the proposal is already being executed
+    error Scheme__ProposalExecutionAlreadyRunning();
+
+    /// @notice Emitted when the proposal isn't submitted
+    error Scheme__ProposalMustBeSubmitted();
+
+    /// @notice Emitted when the call failed. Returns the revert error
+    error Scheme__CallFailed(string reason);
+
+    /// @notice Emitted when the maxRepPercentageChange is exceeded
+    error Scheme__MaxRepPercentageChangePassed();
+
+    /// @notice Emitted if the ERC20 limits are exceeded
+    error Scheme__ERC20LimitsPassed();
+
     /**
      * @dev initialize
      * @param _avatar the avatar address
@@ -81,10 +117,22 @@ abstract contract Scheme is DXDVotingMachineCallbacks {
         uint256 _maxSecondsForExecution,
         uint256 _maxRepPercentageChange
     ) external {
-        require(address(avatar) == address(0), "Scheme: cannot init twice");
-        require(_avatar != address(0), "Scheme: avatar cannot be zero");
-        require(_controller != address(0), "Scheme: controller cannot be zero");
-        require(_maxSecondsForExecution >= 86400, "Scheme: _maxSecondsForExecution cant be less than 86400 seconds");
+        if (address(avatar) != address(0)) {
+            revert Scheme__CannotInitTwice();
+        }
+
+        if (_avatar == address(0)) {
+            revert Scheme__AvatarAddressCannotBeZero();
+        }
+
+        if (_controller == address(0)) {
+            revert Scheme__ControllerAddressCannotBeZero();
+        }
+
+        if (_maxSecondsForExecution < 86400) {
+            revert Scheme__MaxSecondsForExecutionTooLow();
+        }
+
         avatar = DAOAvatar(_avatar);
         votingMachine = IDXDVotingMachine(_votingMachine);
         controller = DAOController(_controller);
@@ -99,11 +147,14 @@ abstract contract Scheme is DXDVotingMachineCallbacks {
      * @param _maxSecondsForExecution New max proposal time in seconds to be used
      */
     function setMaxSecondsForExecution(uint256 _maxSecondsForExecution) external virtual {
-        require(
-            msg.sender == address(avatar) || msg.sender == address(this),
-            "Scheme: setMaxSecondsForExecution is callable only from the avatar or the scheme"
-        );
-        require(_maxSecondsForExecution >= 86400, "Scheme: _maxSecondsForExecution cant be less than 86400 seconds");
+        if (msg.sender != address(avatar) && msg.sender != address(this)) {
+            revert Scheme__SetMaxSecondsForExecutionInvalidCaller();
+        }
+
+        if (_maxSecondsForExecution < 86400) {
+            revert Scheme__MaxSecondsForExecutionTooLow();
+        }
+
         maxSecondsForExecution = _maxSecondsForExecution;
     }
 
@@ -125,9 +176,13 @@ abstract contract Scheme is DXDVotingMachineCallbacks {
         string calldata _title,
         string calldata _descriptionHash
     ) public virtual returns (bytes32 proposalId) {
-        require(_to.length == _callData.length, "Scheme: invalid _callData length");
-        require(_to.length == _value.length, "Scheme: invalid _value length");
-        require((_value.length % (_totalOptions - 1)) == 0, "Scheme: Invalid _totalOptions or action calls length");
+        if (_to.length != _callData.length || _to.length != _value.length) {
+            revert Scheme_InvalidParameterArrayLength();
+        }
+
+        if ((_value.length % (_totalOptions - 1)) != 0) {
+            revert Scheme__InvalidTotalOptionsOrActionsCallsLength();
+        }
 
         bytes32 voteParams = controller.getSchemeParameters(address(this));
 
@@ -167,16 +222,16 @@ abstract contract Scheme is DXDVotingMachineCallbacks {
         returns (bool)
     {
         // We use isExecutingProposal variable to avoid re-entrancy in proposal execution
-        require(!executingProposal, "WalletScheme: proposal execution already running");
+        if (executingProposal) {
+            revert Scheme__ProposalExecutionAlreadyRunning();
+        }
         executingProposal = true;
 
         Proposal storage proposal = proposals[_proposalId];
-        require(proposal.state == ProposalState.Submitted, "WalletScheme: must be a submitted proposal");
 
-        require(
-            !controller.getSchemeCanMakeAvatarCalls(address(this)),
-            "WalletScheme: scheme cannot make avatar calls"
-        );
+        if (proposal.state != ProposalState.Submitted) {
+            revert Scheme__ProposalMustBeSubmitted();
+        }
 
         if (proposal.submittedTime + maxSecondsForExecution < block.timestamp) {
             // If the amount of time passed since submission plus max proposal time is lower than block timestamp
@@ -217,22 +272,25 @@ abstract contract Scheme is DXDVotingMachineCallbacks {
                         proposal.callData[callIndex]
                     );
 
-                    require(callsSucessResult, string(returnData));
+                    if (!callsSucessResult) {
+                        revert Scheme__CallFailed({reason: string(returnData)});
+                    }
                 }
             }
 
             proposal.state = ProposalState.ExecutionSucceeded;
 
             // Cant mint or burn more REP than the allowed percentaged set in the wallet scheme initialization
-            require(
-                ((oldRepSupply * (uint256(100) + (maxRepPercentageChange))) / 100 >=
-                    getNativeReputationTotalSupply()) &&
-                    ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 <=
-                        getNativeReputationTotalSupply()),
-                "WalletScheme: maxRepPercentageChange passed"
-            );
-
-            require(permissionRegistry.checkERC20Limits(address(this)), "WalletScheme: ERC20 limits passed");
+            if (
+                ((oldRepSupply * (uint256(100) + (maxRepPercentageChange))) / 100 < getNativeReputationTotalSupply()) ||
+                ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 > getNativeReputationTotalSupply())
+            ) {
+                revert Scheme__MaxRepPercentageChangePassed();
+            }
+
+            if (!permissionRegistry.checkERC20Limits(address(this))) {
+                revert Scheme__ERC20LimitsPassed();
+            }
 
             emit ProposalStateChange(_proposalId, uint256(ProposalState.ExecutionSucceeded));
         }

contracts/dao/schemes/WalletScheme.sol

@@ -13,6 +13,12 @@ import "./Scheme.sol";
 contract WalletScheme is Scheme {
     using Address for address;
 
+    /// @notice Emitted if the number of totalOptions is not 2
+    error WalletScheme__TotalOptionsMustBeTwo();
+
+    /// @notice Emitted if the WalletScheme can make avatar calls
+    error WalletScheme__CannotMakeAvatarCalls();
+
     /**
      * @dev Receive function that allows the wallet to receive ETH when the controller address is not set
      */
@@ -36,7 +42,10 @@ contract WalletScheme is Scheme {
         string calldata _title,
         string calldata _descriptionHash
     ) public override returns (bytes32 proposalId) {
-        require(_totalOptions == 2, "WalletScheme: The total amount of options should be 2");
+        if (_totalOptions != 2) {
+            revert WalletScheme__TotalOptionsMustBeTwo();
+        }
+
         return super.proposeCalls(_to, _callData, _value, _totalOptions, _title, _descriptionHash);
     }
 
@@ -52,10 +61,10 @@ contract WalletScheme is Scheme {
         onlyVotingMachine
         returns (bool)
     {
-        require(
-            !controller.getSchemeCanMakeAvatarCalls(address(this)),
-            "WalletScheme: scheme cannot make avatar calls"
-        );
+        if (controller.getSchemeCanMakeAvatarCalls(address(this))) {
+            revert WalletScheme__CannotMakeAvatarCalls();
+        }
+
         return super.executeProposal(_proposalId, _winningOption);
     }