From a10b4dc7e7976dc1dfd8d8928e43e0df1b1f200d Mon Sep 17 00:00:00 2001 From: elenadimitrova Date: Mon, 15 Jun 2020 15:08:44 +0300 Subject: [PATCH 1/2] Cater for no guardians logic when executing recovery --- contracts/modules/RecoveryManager.sol | 4 +++- test/recoveryManager.js | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/contracts/modules/RecoveryManager.sol b/contracts/modules/RecoveryManager.sol index 1614ec440..9b510c2e3 100644 --- a/contracts/modules/RecoveryManager.sol +++ b/contracts/modules/RecoveryManager.sol @@ -195,7 +195,9 @@ contract RecoveryManager is BaseModule, RelayerModuleV2 { function getRequiredSignatures(BaseWallet _wallet, bytes memory _data) public view returns (uint256) { bytes4 methodId = functionPrefix(_data); if (methodId == EXECUTE_RECOVERY_PREFIX) { - return SafeMath.ceil(guardianStorage.guardianCount(_wallet), 2); + uint walletGuardians = guardianStorage.guardianCount(_wallet); + require(walletGuardians > 0, "RM: no guardians set on wallet"); + return SafeMath.ceil(walletGuardians, 2); } if (methodId == FINALIZE_RECOVERY_PREFIX) { return 0; diff --git a/test/recoveryManager.js b/test/recoveryManager.js index 57c8480ef..301f446dc 100644 --- a/test/recoveryManager.js +++ b/test/recoveryManager.js @@ -311,6 +311,23 @@ describe("RecoveryManager", function () { }); describe("Execute Recovery", () => { + it("should not allow recovery to be executed with no guardians", async () => { + const noGuardians = []; + await assert.revertWith(manager.relay( + recoveryManager, + "executeRecovery", + [wallet.contractAddress, newowner.address], + wallet, + noGuardians, + ), "RM: no guardians set on wallet"); + + const isLocked = await lockManager.isLocked(wallet.contractAddress); + assert.isFalse(isLocked, "should not be locked by recovery"); + + const walletOwner = await wallet.owner(); + assert.equal(walletOwner, owner.address, "owner should have not changed"); + }); + describe("EOA Guardians: G = 2", () => { beforeEach(async () => { await addGuardians([guardian1, guardian2]); From 5d6015a50a9754b9364e69f898e6d391196b1cf7 Mon Sep 17 00:00:00 2001 From: elenadimitrova Date: Tue, 16 Jun 2020 13:47:10 +0300 Subject: [PATCH 2/2] Update deployment script for 1.6 release patch --- deployment/7_upgrade_1_6.js | 75 ++----------------------------------- 1 file changed, 3 insertions(+), 72 deletions(-) diff --git a/deployment/7_upgrade_1_6.js b/deployment/7_upgrade_1_6.js index 62a25eab4..b9c2100fc 100644 --- a/deployment/7_upgrade_1_6.js +++ b/deployment/7_upgrade_1_6.js @@ -1,25 +1,19 @@ const semver = require("semver"); const childProcess = require("child_process"); -const ApprovedTransfer = require("../build/ApprovedTransfer"); const RecoveryManager = require("../build/RecoveryManager"); const MultiSig = require("../build/MultiSigWallet"); const ModuleRegistry = require("../build/ModuleRegistry"); const Upgrader = require("../build/SimpleUpgrader"); const DeployManager = require("../utils/deploy-manager.js"); const MultisigExecutor = require("../utils/multisigexecutor.js"); -const TokenPriceProvider = require("../build/TokenPriceProvider"); -const MakerRegistry = require("../build/MakerRegistry"); -const ScdMcdMigration = require("../build/ScdMcdMigration"); -const MakerV2Manager = require("../build/MakerV2Manager"); -const TransferManager = require("../build/TransferManager"); const utils = require("../utils/utilities.js"); const TARGET_VERSION = "1.6.0"; -const MODULES_TO_ENABLE = ["ApprovedTransfer", "RecoveryManager", "MakerV2Manager", "TransferManager"]; -const MODULES_TO_DISABLE = ["UniswapManager"]; +const MODULES_TO_ENABLE = ["RecoveryManager"]; +const MODULES_TO_DISABLE = []; -const BACKWARD_COMPATIBILITY = 1; +const BACKWARD_COMPATIBILITY = 2; const deploy = async (network) => { if (!["kovan", "kovan-fork", "staging", "prod"].includes(network)) { @@ -50,33 +44,10 @@ const deploy = async (network) => { const MultiSigWrapper = await deployer.wrapDeployedContract(MultiSig, config.contracts.MultiSigWallet); const multisigExecutor = new MultisigExecutor(MultiSigWrapper, deploymentWallet, config.multisig.autosign, { gasPrice }); - // ////////////////////////////////// - // Deploy infrastructure contracts - // ////////////////////////////////// - - // Deploy and configure Maker Registry - const ScdMcdMigrationWrapper = await deployer.wrapDeployedContract(ScdMcdMigration, config.defi.maker.migration); - const vatAddress = await ScdMcdMigrationWrapper.vat(); - const MakerRegistryWrapper = await deployer.deploy(MakerRegistry, {}, vatAddress); - const wethJoinAddress = await ScdMcdMigrationWrapper.wethJoin(); - const addCollateralTransaction = await MakerRegistryWrapper.contract.addCollateral(wethJoinAddress, { gasPrice }); - await MakerRegistryWrapper.verboseWaitForTransaction(addCollateralTransaction, `Adding join adapter ${wethJoinAddress} to the MakerRegistry`); - const changeMakerRegistryOwnerTx = await MakerRegistryWrapper.contract.changeOwner(config.contracts.MultiSigWallet, { gasPrice }); - await MakerRegistryWrapper.verboseWaitForTransaction(changeMakerRegistryOwnerTx, "Set the MultiSig as the owner of the MakerRegistry"); - const TokenPriceProviderWrapper = await deployer.wrapDeployedContract(TokenPriceProvider, config.contracts.TokenPriceProvider); - // ////////////////////////////////// // Deploy new modules // ////////////////////////////////// - const ApprovedTransferWrapper = await deployer.deploy( - ApprovedTransfer, - {}, - config.contracts.ModuleRegistry, - config.modules.GuardianStorage, - ); - newModuleWrappers.push(ApprovedTransferWrapper); - const RecoveryManagerWrapper = await deployer.deploy( RecoveryManager, {}, @@ -89,46 +60,12 @@ const deploy = async (network) => { ); newModuleWrappers.push(RecoveryManagerWrapper); - const MakerV2ManagerWrapper = await deployer.deploy( - MakerV2Manager, - {}, - config.contracts.ModuleRegistry, - config.modules.GuardianStorage, - config.defi.maker.migration, - config.defi.maker.pot, - config.defi.maker.jug, - MakerRegistryWrapper.contractAddress, - config.defi.uniswap.factory, - ); - newModuleWrappers.push(MakerV2ManagerWrapper); - - const TransferManagerWrapper = await deployer.deploy( - TransferManager, - {}, - config.contracts.ModuleRegistry, - config.modules.TransferStorage, - config.modules.GuardianStorage, - TokenPriceProviderWrapper.contractAddress, - config.settings.securityPeriod || 0, - config.settings.securityWindow || 0, - config.settings.defaultLimit || "1000000000000000000", - ["test", "staging", "prod"].includes(network) ? config.modules.TransferManager : "0x0000000000000000000000000000000000000000", - ); - newModuleWrappers.push(TransferManagerWrapper); - // ///////////////////////////////////////////////// // Update config and Upload ABIs // ///////////////////////////////////////////////// configurator.updateModuleAddresses({ - ApprovedTransfer: ApprovedTransferWrapper.contractAddress, RecoveryManager: RecoveryManagerWrapper.contractAddress, - MakerV2Manager: MakerV2ManagerWrapper.contractAddress, - TransferManager: TransferManagerWrapper.contractAddress, - }); - - configurator.updateInfrastructureAddresses({ - MakerRegistry: MakerRegistryWrapper.contractAddress, }); const gitHash = childProcess.execSync("git rev-parse HEAD").toString("utf8").replace(/\n$/, ""); @@ -136,11 +73,7 @@ const deploy = async (network) => { await configurator.save(); await Promise.all([ - abiUploader.upload(ApprovedTransferWrapper, "modules"), abiUploader.upload(RecoveryManagerWrapper, "modules"), - abiUploader.upload(MakerV2ManagerWrapper, "modules"), - abiUploader.upload(TransferManagerWrapper, "modules"), - abiUploader.upload(MakerRegistryWrapper, "contracts"), ]); // ////////////////////////////////// @@ -157,7 +90,6 @@ const deploy = async (network) => { // Deploy and Register upgraders // ////////////////////////////////// - let fingerprint; const versions = await versionUploader.load(BACKWARD_COMPATIBILITY); for (let idx = 0; idx < versions.length; idx += 1) { @@ -208,7 +140,6 @@ const deploy = async (network) => { await versionUploader.upload(newVersion); }; - module.exports = { deploy, };