Refactoring validateSignatures() to share the part common to several …

…modules in RelayerModule
argentlabs · Apr 1, 2020 · 3640d72 · 3640d72
1 parent 6cf62a0
commit 3640d72
Show file tree

Hide file tree

Showing 6 changed files with 70 additions and 83 deletions.
diff --git a/contracts/modules/ApprovedTransfer.sol b/contracts/modules/ApprovedTransfer.sol
@@ -117,30 +117,7 @@ contract ApprovedTransfer is BaseModule, RelayerModule, BaseTransfer {
         view
         returns (bool)
     {
-        address lastSigner = address(0);
-        address[] memory guardians = guardianStorage.getGuardians(_wallet);
-        bool isGuardian = false;
-        for (uint8 i = 0; i < _signatures.length / 65; i++) {
-            address signer = recoverSigner(_signHash, _signatures, i);
-            if (i == 0) {
-                // AT: first signer must be owner
-                if (!isOwner(_wallet, signer)) {
-                    return false;
-                }
-            } else {
-                // "AT: signers must be different"
-                if (signer <= lastSigner) {
-                    return false;
-                }
-                lastSigner = signer;
-                (isGuardian, guardians) = GuardianUtils.isGuardian(guardians, signer);
-                // "AT: signatures not valid"
-                if (!isGuardian) {
-                    return false;
-                }
-            }
-        }
-        return true;
+        return validateSignatures(_wallet, _signHash, _signatures, OwnerSignature.Required);
     }
 
     function getRequiredSignatures(BaseWallet _wallet, bytes memory /* _data */) internal view returns (uint256) {

diff --git a/contracts/modules/GuardianManager.sol b/contracts/modules/GuardianManager.sol
@@ -221,8 +221,7 @@ contract GuardianManager is BaseModule, RelayerModule {
         view
         returns (bool)
     {
-        address signer = recoverSigner(_signHash, _signatures, 0);
-        return isOwner(_wallet, signer); // "GM: signer must be owner"
+        return validateSignatures(_wallet, _signHash, _signatures, OwnerSignature.Required);
     }
 
     function getRequiredSignatures(BaseWallet /* _wallet */, bytes memory _data) internal view returns (uint256) {

diff --git a/contracts/modules/LockManager.sol b/contracts/modules/LockManager.sol
@@ -132,8 +132,7 @@ contract LockManager is BaseModule, RelayerModule {
         view
         returns (bool)
     {
-        (bool isGuardian, ) = GuardianUtils.isGuardian(guardianStorage.getGuardians(_wallet), recoverSigner(_signHash, _signatures, 0));
-        return isGuardian; // "LM: must be a guardian to lock or unlock"
+        return validateSignatures(_wallet, _signHash, _signatures, OwnerSignature.Disallowed);
     }
 
     function getRequiredSignatures(BaseWallet /* _wallet */, bytes memory /* _data */) internal view returns (uint256) {

diff --git a/contracts/modules/RecoveryManager.sol b/contracts/modules/RecoveryManager.sol
@@ -19,7 +19,6 @@ import "./common/BaseModule.sol";
 import "./common/RelayerModule.sol";
 import "../storage/GuardianStorage.sol";
 import "../../lib/utils/SafeMath.sol";
-import "../utils/GuardianUtils.sol";
 
 /**
  * @title RecoveryManager
@@ -45,12 +44,6 @@ contract RecoveryManager is BaseModule, RelayerModule {
         uint32 guardianCount;
     }
 
-    enum OwnerSignature {
-        Required,
-        Optional,
-        Disallowed
-    }
-
     // Wallet specific storage
     mapping (address => RecoveryConfig) internal recoveryConfigs;
 
@@ -200,46 +193,6 @@ contract RecoveryManager is BaseModule, RelayerModule {
         }
     }
 
-    function validateSignatures(
-        BaseWallet _wallet,
-        bytes32 _signHash,
-        bytes memory _signatures,
-        OwnerSignature _option
-    ) internal view returns (bool)
-    {
-        address lastSigner = address(0);
-        address[] memory guardians = guardianStorage.getGuardians(_wallet);
-        bool isGuardian = false;
-
-        for (uint8 i = 0; i < _signatures.length / 65; i++) {
-            address signer = recoverSigner(_signHash, _signatures, i);
-
-            if (i == 0) {
-                if (_option == OwnerSignature.Required) {
-                    // First signer must be owner
-                    if (isOwner(_wallet, signer)) {
-                        continue;
-                    }
-                    return false;
-                } else if (_option == OwnerSignature.Optional) {
-                    // First signer can be owner
-                    if (isOwner(_wallet, signer)) {
-                        continue;
-                    }
-                }
-            }
-            if (signer <= lastSigner) {
-                return false;
-            } // Signers must be different
-            lastSigner = signer;
-            (isGuardian, guardians) = GuardianUtils.isGuardian(guardians, signer);
-            if (!isGuardian) {
-                return false;
-            }
-        }
-        return true;
-    }
-
     function getRequiredSignatures(BaseWallet _wallet, bytes memory _data) internal view returns (uint256) {
         bytes4 methodId = functionPrefix(_data);
         if (methodId == EXECUTE_RECOVERY_PREFIX) {

diff --git a/contracts/modules/common/OnlyOwnerModule.sol b/contracts/modules/common/OnlyOwnerModule.sol
@@ -65,8 +65,7 @@ contract OnlyOwnerModule is BaseModule, RelayerModule {
         view
         returns (bool)
     {
-        address signer = recoverSigner(_signHash, _signatures, 0);
-        return isOwner(_wallet, signer); // "OOM: signer must be owner"
+        return validateSignatures(_wallet, _signHash, _signatures, OwnerSignature.Required);
     }
 
     function getRequiredSignatures(BaseWallet /* _wallet */, bytes memory /* _data */) internal view returns (uint256) {

diff --git a/contracts/modules/common/RelayerModule.sol b/contracts/modules/common/RelayerModule.sol
@@ -16,6 +16,7 @@
 pragma solidity ^0.5.4;
 import "../../wallet/BaseWallet.sol";
 import "../../interfaces/Module.sol";
+import "../../utils/GuardianUtils.sol";
 import "./BaseModule.sol";
 
 /**
@@ -34,6 +35,12 @@ contract RelayerModule is BaseModule {
         mapping (bytes32 => bool) executedTx;
     }
 
+    enum OwnerSignature {
+        Required,
+        Optional,
+        Disallowed
+    }
+
     event TransactionExecuted(address indexed wallet, bool indexed success, bytes32 signedHash);
 
     /**
@@ -67,7 +74,60 @@ contract RelayerModule is BaseModule {
         BaseWallet _wallet,
         bytes memory _data,
         bytes32 _signHash,
-        bytes memory _signatures) internal view returns (bool);
+        bytes memory _signatures
+    )
+        internal view returns (bool);
+
+    /**
+    * @dev Validates the signatures provided with a relayed transaction.
+    * The method MUST throw if one or more signatures are not valid.
+    * @param _wallet The target wallet.
+    * @param _signHash The signed hash representing the relayed transaction.
+    * @param _signatures The signatures as a concatenated byte array.
+    * @param _option An enum indicating whether the owner is required, optional or disallowed.
+    */
+    function validateSignatures(
+        BaseWallet _wallet,
+        bytes32 _signHash,
+        bytes memory _signatures,
+        OwnerSignature _option
+    )
+        internal view returns (bool)
+    {
+        address lastSigner = address(0);
+        address[] memory guardians;
+        if (_signatures.length > 65 || _option != OwnerSignature.Required)
+            guardians = guardianStorage.getGuardians(_wallet); // guardians are read only if they may be needed
+        bool isGuardian;
+
+        for (uint8 i = 0; i < _signatures.length / 65; i++) {
+            address signer = recoverSigner(_signHash, _signatures, i);
+
+            if (i == 0) {
+                if (_option == OwnerSignature.Required) {
+                    // First signer must be owner
+                    if (isOwner(_wallet, signer)) {
+                        continue;
+                    }
+                    return false;
+                } else if (_option == OwnerSignature.Optional) {
+                    // First signer can be owner
+                    if (isOwner(_wallet, signer)) {
+                        continue;
+                    }
+                }
+            }
+            if (signer <= lastSigner) {
+                return false; // Signers must be different
+            }
+            lastSigner = signer;
+            (isGuardian, guardians) = GuardianUtils.isGuardian(guardians, signer);
+            if (!isGuardian) {
+                return false;
+            }
+        }
+        return true;
+    }
 
     /* ************************************************************ */
 
@@ -100,11 +160,11 @@ contract RelayerModule is BaseModule {
         require(requiredSignatures == 0 || validateSignatures(_wallet, _data, signHash, _signatures), "RM: Invalid signatures");
         // The correctness of the refund is checked on the next line using an `if` instead of a `require`
         // in order to prevent a failing refund from being replayable in the future.
-            if (verifyRefund(_wallet, _gasLimit, _gasPrice, requiredSignatures)) {
-                    // solium-disable-next-line security/no-call-value
-                    (success,) = address(this).call(_data);
-                    refund(_wallet, startGas - gasleft(), _gasPrice, _gasLimit, requiredSignatures, msg.sender);
-                }
+        if (verifyRefund(_wallet, _gasLimit, _gasPrice, requiredSignatures)) {
+            // solium-disable-next-line security/no-call-value
+            (success,) = address(this).call(_data);
+            refund(_wallet, startGas - gasleft(), _gasPrice, _gasLimit, requiredSignatures, msg.sender);
+        }
         emit TransactionExecuted(address(_wallet), success, signHash);
     }