Merge pull request #25 from 0xJurassicPunk/minor-refactoring

refactor: Minor adjustments for revert statements

Author: 0xJurassicPunk

contracts/OwnableTwoSteps.sol

@@ -10,36 +10,37 @@ import {IOwnableTwoSteps} from "./interfaces/IOwnableTwoSteps.sol";
  */
 abstract contract OwnableTwoSteps is IOwnableTwoSteps {
     // Address of the current owner
-    address private _owner;
+    address public owner;
 
     // Address of the potential owner
-    address private _potentialOwner;
+    address public potentialOwner;
+
+    // Delay for the timelock (in seconds)
+    uint256 public delay;
 
     // Earliest ownership renouncement timestamp
-    uint256 private _earliestOwnershipRenouncementTime;
+    uint256 public earliestOwnershipRenouncementTime;
 
     // Ownership status
     Status public status;
 
-    // Current delay for the timelock (in seconds)
-    uint256 public delay;
-
     /**
      * @notice Modifier to wrap functions for contracts that inherit this contract
      */
     modifier onlyOwner() {
-        if (msg.sender != _owner) {
+        if (msg.sender != owner) {
             revert NotOwner();
         }
         _;
     }
 
     /**
      * @notice Constructor
-     * @notice Initial owner is the deployment address while initial delay (for the timelock) must be set by contract that inherit from this.
+     *         Initial owner is the deployment address.
+     *         Delay (for the timelock) must be set by the contract that inherits from this.
      */
     constructor() {
-        _owner = msg.sender;
+        owner = msg.sender;
     }
 
     /**
@@ -51,9 +52,9 @@ abstract contract OwnableTwoSteps is IOwnableTwoSteps {
         if (status == Status.NoOngoingTransfer) revert NoOngoingTransferInProgress();
 
         if (status == Status.TransferInProgress) {
-            delete _potentialOwner;
+            delete potentialOwner;
         } else if (status == Status.RenouncementInProgress) {
-            delete _earliestOwnershipRenouncementTime;
+            delete earliestOwnershipRenouncementTime;
         }
 
         delete status;
@@ -66,10 +67,10 @@ abstract contract OwnableTwoSteps is IOwnableTwoSteps {
      */
     function confirmOwnershipRenouncement() external onlyOwner {
         if (status != Status.RenouncementInProgress) revert RenouncementNotInProgress();
-        if (block.timestamp < _earliestOwnershipRenouncementTime) revert RenouncementTooEarly();
+        if (block.timestamp < earliestOwnershipRenouncementTime) revert RenouncementTooEarly();
 
-        delete _earliestOwnershipRenouncementTime;
-        delete _owner;
+        delete earliestOwnershipRenouncementTime;
+        delete owner;
         delete status;
 
         emit NewOwner(address(0));
@@ -81,26 +82,26 @@ abstract contract OwnableTwoSteps is IOwnableTwoSteps {
      */
     function confirmOwnershipTransfer() external {
         if (status != Status.TransferInProgress) revert TransferNotInProgress();
-        if (msg.sender != _potentialOwner) revert WrongPotentialOwner();
+        if (msg.sender != potentialOwner) revert WrongPotentialOwner();
 
-        _owner = msg.sender;
+        owner = msg.sender;
         delete status;
-        delete _potentialOwner;
+        delete potentialOwner;
 
-        emit NewOwner(_owner);
+        emit NewOwner(owner);
     }
 
     /**
      * @notice Initiate transfer of ownership to a new owner
-     * @param newPotentialOwner address of the new potential owner
+     * @param newPotentialOwner New potential owner address
      */
     function initiateOwnershipTransfer(address newPotentialOwner) external onlyOwner {
         if (status != Status.NoOngoingTransfer) revert TransferAlreadyInProgress();
 
         status = Status.TransferInProgress;
-        _potentialOwner = newPotentialOwner;
+        potentialOwner = newPotentialOwner;
 
-        emit InitiateOwnershipTransfer(_owner, newPotentialOwner);
+        emit InitiateOwnershipTransfer(owner, newPotentialOwner);
     }
 
     /**
@@ -110,40 +111,16 @@ abstract contract OwnableTwoSteps is IOwnableTwoSteps {
         if (status != Status.NoOngoingTransfer) revert TransferAlreadyInProgress();
 
         status = Status.RenouncementInProgress;
-        _earliestOwnershipRenouncementTime = block.timestamp + delay;
-
-        emit InitiateOwnershipRenouncement(_earliestOwnershipRenouncementTime);
-    }
-
-    /**
-     * @notice Returns owner address
-     * @return owner address
-     */
-    function owner() external view returns (address) {
-        return _owner;
-    }
-
-    /**
-     * @notice Returns potential owner address
-     * @return potential owner address
-     */
-    function potentialOwner() external view returns (address) {
-        return _potentialOwner;
-    }
+        earliestOwnershipRenouncementTime = block.timestamp + delay;
 
-    /**
-     * @notice Returns the earliest timestamp after which the ownership renouncement
-     *         can be confirmed by the current owner.
-     * @return earliest timestamp for renouncement confirmation
-     */
-    function earliestOwnershipRenouncementTime() external view returns (uint256) {
-        return _earliestOwnershipRenouncementTime;
+        emit InitiateOwnershipRenouncement(earliestOwnershipRenouncementTime);
     }
 
     /**
      * @notice Set up the timelock delay for renouncing ownership
-     * @param _delay timelock delay for the owner to confirm renouncing the ownership
+     * @param _delay Timelock delay for the owner to confirm renouncing the ownership
      * @dev This function is expected to be included in the constructor of the contract that inherits this contract.
+     *      If it is not set, there is no timelock to renounce the ownership.
      */
     function _setupDelayForRenouncingOwnership(uint256 _delay) internal {
         delay = _delay;

contracts/ReentrancyGuard.sol

@@ -6,7 +6,7 @@ import {IReentrancyGuard} from "./interfaces/IReentrancyGuard.sol";
 /**
  * @title ReentrancyGuard
  * @notice This contract protects against reentrancy attacks.
- * It is adjusted from OpenZeppelin.
+ *         It is adjusted from OpenZeppelin.
  */
 abstract contract ReentrancyGuard is IReentrancyGuard {
     uint256 private _status;
@@ -19,9 +19,7 @@ abstract contract ReentrancyGuard is IReentrancyGuard {
      * @notice Modifier to wrap functions to prevent reentrancy calls
      */
     modifier nonReentrant() {
-        if (_status == 2) {
-            revert ReentrancyFail();
-        }
+        if (_status == 2) revert ReentrancyFail();
 
         _status = 2;
         _;

contracts/SignatureChecker.sol

@@ -43,40 +43,30 @@ abstract contract SignatureChecker is ISignatureChecker {
             revert WrongSignatureLength(signature.length);
         }
 
-        // https://ethereum.stackexchange.com/questions/83174/is-it-best-practice-to-check-signature-malleability-in-ecrecover
-        // https://crypto.iacr.org/2019/affevents/wac/medias/Heninger-BiasedNonceSense.pdf
-        if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
-            revert BadSignatureS();
-        }
+        if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) revert BadSignatureS();
 
-        if (v != 27 && v != 28) {
-            revert BadSignatureV(v);
-        }
+        if (v != 27 && v != 28) revert BadSignatureV(v);
     }
 
     /**
      * @notice Recover the signer of a signature (for EOA)
-     * @param hash the hash of the signed message
-     * @param signature bytes containing the signature (64 or 65 bytes)
+     * @param hash Hash of the signed message
+     * @param signature Bytes containing the signature (64 or 65 bytes)
      */
-    function _recoverEOASigner(bytes32 hash, bytes memory signature) internal pure returns (address) {
+    function _recoverEOASigner(bytes32 hash, bytes memory signature) internal pure returns (address signer) {
         (bytes32 r, bytes32 s, uint8 v) = _splitSignature(signature);
 
         // If the signature is valid (and not malleable), return the signer address
-        address signer = ecrecover(hash, v, r, s);
-
-        if (signer == address(0)) {
-            revert NullSignerAddress();
-        }
+        signer = ecrecover(hash, v, r, s);
 
-        return signer;
+        if (signer == address(0)) revert NullSignerAddress();
     }
 
     /**
      * @notice Checks whether the signer is valid
-     * @param hash data hash
-     * @param signer the signer address (to confirm message validity)
-     * @param signature signature parameters encoded (v, r, s)
+     * @param hash Data hash
+     * @param signer Signer address (to confirm message validity)
+     * @param signature Signature parameters encoded (v, r, s)
      * @dev For EIP-712 signatures, the hash must be the digest (computed with signature hash and domain separator)
      */
     function _verify(
@@ -85,13 +75,9 @@ abstract contract SignatureChecker is ISignatureChecker {
         bytes memory signature
     ) internal view {
         if (signer.code.length == 0) {
-            if (_recoverEOASigner(hash, signature) != signer) {
-                revert InvalidSignatureEOA();
-            }
+            if (_recoverEOASigner(hash, signature) != signer) revert InvalidSignatureEOA();
         } else {
-            if (IERC1271(signer).isValidSignature(hash, signature) != 0x1626ba7e) {
-                revert InvalidSignatureERC1271();
-            }
+            if (IERC1271(signer).isValidSignature(hash, signature) != 0x1626ba7e) revert InvalidSignatureERC1271();
         }
     }
 }

contracts/interfaces/IReentrancyGuard.sol

@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
+/**
+ * @title IReentrancyGuard
+ */
 interface IReentrancyGuard {
     error ReentrancyFail();
 }

contracts/interfaces/ISignatureChecker.sol

@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
+/**
+ * @title ISignatureChecker
+ */
 interface ISignatureChecker {
     // Custom errors
     error BadSignatureS();