diff --git a/contracts/modules/common/RelayerModule.sol b/contracts/modules/common/RelayerModule.sol index 4d75b366c..7b6cc59a5 100644 --- a/contracts/modules/common/RelayerModule.sol +++ b/contracts/modules/common/RelayerModule.sol @@ -94,17 +94,17 @@ contract RelayerModule is BaseModule { uint startGas = gasleft(); bytes32 signHash = getSignHash(address(this), address(_wallet), 0, _data, _nonce, _gasPrice, _gasLimit); require(checkAndUpdateUniqueness(_wallet, _nonce, signHash), "RM: Duplicate request"); - require(verifyData(address(_wallet), _data), "RM: the wallet authorized is different then the target of the relayed data"); + require(verifyData(address(_wallet), _data), "RM: Target of _data != _wallet"); uint256 requiredSignatures = getRequiredSignatures(_wallet, _data); - if ((requiredSignatures * 65) == _signatures.length) { + require(requiredSignatures * 65 == _signatures.length, "RM: Wrong number of signatures"); + 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)) { - if (requiredSignatures == 0 || validateSignatures(_wallet, _data, signHash, _signatures)) { // 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); } diff --git a/test/approvedTransfer.js b/test/approvedTransfer.js index 77df9754f..b9956be9e 100644 --- a/test/approvedTransfer.js +++ b/test/approvedTransfer.js @@ -20,6 +20,8 @@ const KYBER_RATE = 51 * 10 ** 13; // 1 TOKN = 0.00051 ETH const ZERO_BYTES32 = ethers.constants.HashZero; +const WRONG_SIGNATURE_NUMBER_REVERT_MSG = "RM: Wrong number of signatures"; + describe("Test Approved Transfer", function () { this.timeout(10000); @@ -27,10 +29,10 @@ describe("Test Approved Transfer", function () { const infrastructure = accounts[0].signer; const owner = accounts[1].signer; - const guardian1 = accounts[3].signer; - const guardian2 = accounts[4].signer; - const guardian3 = accounts[5].signer; - const recipient = accounts[6].signer; + const guardian1 = accounts[2].signer; + const guardian2 = accounts[3].signer; + const guardian3 = accounts[4].signer; + const recipient = accounts[5].signer; let deployer; let wallet; @@ -90,162 +92,188 @@ describe("Test Approved Transfer", function () { } describe("Transfer approved by EOA guardians", () => { - it("should transfer ETH with 1 confirmations for 1 guardians", async () => { + it('should transfer ETH with 1 confirmations for 1 guardians', async () => { const amountToTransfer = 10000; - await addGuardians([guardian1]); + await addGuardians([guardian1]) const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 1, "1 guardians should be active"); + assert.equal(count, 1, '1 guardians should be active'); const before = await deployer.provider.getBalance(recipient.address); // should succeed with one confirmation - await manager.relay(transferModule, "transferToken", + await manager.relay(transferModule, "transferToken", [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); const after = await deployer.provider.getBalance(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ETH amount'); }); - it("should transfer ETH with 1 confirmations for 2 guardians", async () => { + it('should transfer ETH with 1 confirmations for 2 guardians', async () => { const amountToTransfer = 10000; - await addGuardians([guardian1, guardian2]); + await addGuardians([guardian1, guardian2]) const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 2, "2 guardians should be active"); + assert.equal(count, 2, '2 guardians should be active'); const before = await deployer.provider.getBalance(recipient.address); // should succeed with one confirmation - await manager.relay(transferModule, "transferToken", + await manager.relay(transferModule, "transferToken", [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); const after = await deployer.provider.getBalance(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ETH amount'); }); - it("should only transfer ETH with 2 confirmations for 3 guardians", async () => { + it('should only transfer ETH with 2 confirmations for 3 guardians', async () => { const amountToTransfer = 10000; - await addGuardians([guardian1, guardian2, guardian3]); + await addGuardians([guardian1, guardian2, guardian3]) const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 3, "3 guardians should be active"); + assert.equal(count, 3, '3 guardians should be active'); const before = await deployer.provider.getBalance(recipient.address); // should fail with one confirmation - const txReceipt = await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transfer should fail with 1 guardian confirmation"); + await assert.revertWith( + manager.relay( + transferModule, + "transferToken", + [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], + wallet, + [owner, guardian1] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); // should succeed with 2 confirmations - await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, - [owner, ...sortWalletByAddress([guardian1, guardian2])]); + await manager.relay( + transferModule, + "transferToken", + [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], + wallet, + [owner, ...sortWalletByAddress([guardian1, guardian2])] + ); const after = await deployer.provider.getBalance(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ETH amount'); }); - it("should fail to transfer ETH when signer is not a guardians", async () => { + it('should fail to transfer ETH when signer is not a guardian', async () => { const amountToTransfer = 10000; const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 0, "0 guardians should be active"); + assert.equal(count, 0, '0 guardians should be active'); // should fail - const txReceipt = await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transfer should fail when signer is not a guardian"); + await assert.revertWith( + manager.relay( + transferModule, + "transferToken", + [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], + wallet, + [owner, guardian1] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); }); - it("should transfer ERC20 with 1 confirmations for 1 guardians", async () => { + it('should transfer ERC20 with 1 confirmations for 1 guardian', async () => { const amountToTransfer = 10000; - await addGuardians([guardian1]); + await addGuardians([guardian1]) const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 1, "1 guardians should be active"); + assert.equal(count, 1, '1 guardians should be active'); const before = await erc20.balanceOf(recipient.address); // should succeed with one confirmation - await manager.relay(transferModule, "transferToken", + await manager.relay(transferModule, "transferToken", [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); const after = await erc20.balanceOf(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ERC20 amount"); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ERC20 amount'); }); - it("should only transfer ERC20 with 2 confirmations for 3 guardians", async () => { + it('should only transfer ERC20 with 2 confirmations for 3 guardians', async () => { const amountToTransfer = 10000; - await addGuardians([guardian1, guardian2, guardian3]); + await addGuardians([guardian1, guardian2, guardian3]) const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 3, "3 guardians should be active"); + assert.equal(count, 3, '3 guardians should be active'); const before = await erc20.balanceOf(recipient.address); // should fail with one confirmation - const txReceipt = await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transfer with 1 guardian signature should fail"); + await assert.revertWith( + manager.relay( + transferModule, + "transferToken", + [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], + wallet, + [owner, guardian1] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); // should succeed with 2 confirmations - await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, - [owner, ...sortWalletByAddress([guardian1, guardian2])]); + await manager.relay(transferModule, "transferToken", + [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, ...sortWalletByAddress([guardian1, guardian2])]); const after = await erc20.balanceOf(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ERC20 amount"); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ERC20 amount'); }); }); describe("Transfer approved by smart-contract guardians", () => { - it("should transfer ETH with 1 confirmations for 1 guardians", async () => { - const amountToTransfer = 10000; - await addGuardians(await createSmartContractGuardians([guardian1])); - const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 1, "1 guardians should be active"); - const before = await deployer.provider.getBalance(recipient.address); - // should succeed with one confirmation - await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const after = await deployer.provider.getBalance(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); + it('should transfer ETH with 1 confirmations for 1 guardian', async () => { + const amountToTransfer = 10000; + await addGuardians(await createSmartContractGuardians([guardian1])); + const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); + assert.equal(count, 1, '1 guardian should be active'); + const before = await deployer.provider.getBalance(recipient.address); + // should succeed with one confirmation + await manager.relay( + transferModule, + "transferToken", + [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], + wallet, + [owner, guardian1] + ); + const after = await deployer.provider.getBalance(recipient.address); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ETH amount'); }); - it("should transfer ETH with 1 confirmations for 2 guardians", async () => { - const amountToTransfer = 10000; - await addGuardians(await createSmartContractGuardians([guardian1, guardian2])); - const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 2, "2 guardians should be active"); - const before = await deployer.provider.getBalance(recipient.address); - // should succeed with one confirmation - await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const after = await deployer.provider.getBalance(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); + it('should transfer ETH with 1 confirmation for 2 guardians', async () => { + const amountToTransfer = 10000; + await addGuardians(await createSmartContractGuardians([guardian1, guardian2])); + const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); + assert.equal(count, 2, '2 guardians should be active'); + const before = await deployer.provider.getBalance(recipient.address); + // should succeed with one confirmation + await manager.relay(transferModule, "transferToken", + [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); + const after = await deployer.provider.getBalance(recipient.address); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ETH amount'); }); - it("should only transfer ETH with 2 confirmations for 3 guardians", async () => { - const amountToTransfer = 10000; - await addGuardians(await createSmartContractGuardians([guardian1, guardian2, guardian3])); - const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 3, "3 guardians should be active"); - const before = await deployer.provider.getBalance(recipient.address); - // should fail with one confirmation - const txReceipt = await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transfer with 1 guardian signature should fail"); - // should succeed with 2 confirmations - await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, - [owner, ...sortWalletByAddress([guardian1, guardian2])]); - const after = await deployer.provider.getBalance(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); + it('should only transfer ETH with 2 confirmations for 3 guardians', async () => { + const amountToTransfer = 10000; + await addGuardians(await createSmartContractGuardians([guardian1, guardian2, guardian3])); + const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); + assert.equal(count, 3, '3 guardians should be active'); + const before = await deployer.provider.getBalance(recipient.address); + // should fail with one confirmation + await assert.revertWith( + manager.relay( + transferModule, + "transferToken", + [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], + wallet, + [owner, guardian1] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); + // should succeed with 2 confirmations + await manager.relay(transferModule, "transferToken", + [wallet.contractAddress, ETH_TOKEN, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, ...sortWalletByAddress([guardian1, guardian2])]); + const after = await deployer.provider.getBalance(recipient.address); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ETH amount'); }); - it("should transfer ERC20 with 1 confirmations for 1 guardians", async () => { - const amountToTransfer = 10000; - await addGuardians(await createSmartContractGuardians([guardian1])); - const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 1, "1 guardians should be active"); - const before = await erc20.balanceOf(recipient.address); - // should succeed with one confirmation - await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const after = await erc20.balanceOf(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); + it('should transfer ERC20 with 1 confirmation for 1 guardian', async () => { + const amountToTransfer = 10000; + await addGuardians(await createSmartContractGuardians([guardian1])); + const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); + assert.equal(count, 1, '1 guardian should be active'); + const before = await erc20.balanceOf(recipient.address); + // should succeed with one confirmation + await manager.relay(transferModule, "transferToken", + [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); + const after = await erc20.balanceOf(recipient.address); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ETH amount'); }); - it("should only transfer ERC20 with 2 confirmations for 3 guardians", async () => { - const amountToTransfer = 10000; - await addGuardians(await createSmartContractGuardians([guardian1, guardian2, guardian3])); - const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); - assert.equal(count, 3, "3 guardians should be active"); - const before = await erc20.balanceOf(recipient.address); - // should fail with one confirmation - const txReceipt = await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transfer with 1 guardian signature should throw"); - // should succeed with 2 confirmations - await manager.relay(transferModule, "transferToken", - [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, - [owner, ...sortWalletByAddress([guardian1, guardian2])]); - const after = await erc20.balanceOf(recipient.address); - assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ERC20 amount"); + it('should only transfer ERC20 with 2 confirmations for 3 guardians', async () => { + const amountToTransfer = 10000; + await addGuardians(await createSmartContractGuardians([guardian1, guardian2, guardian3])); + const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); + assert.equal(count, 3, '3 guardians should be active'); + const before = await erc20.balanceOf(recipient.address); + // should fail with one confirmation + await assert.revertWith( + manager.relay( + transferModule, + "transferToken", + [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], + wallet, + [owner, guardian1] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); + // should succeed with 2 confirmations + await manager.relay(transferModule, "transferToken", + [wallet.contractAddress, erc20.contractAddress, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, [owner, ...sortWalletByAddress([guardian1, guardian2])]); + const after = await erc20.balanceOf(recipient.address); + assert.equal(after.sub(before).toNumber(), amountToTransfer, 'should have transfered the ERC20 amount'); }); }); @@ -277,7 +305,7 @@ describe("Test Approved Transfer", function () { after = await deployer.provider.getBalance(recipient.address); assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); }); - it("should transfer ETH with 2 EOA guardian and 1 smart-contract guardians", async () => { + it("should transfer ETH with 2 EOA guardians and 1 smart-contract guardian", async () => { const amountToTransfer = 10000; await addGuardians([guardian1, guardian2, ...await createSmartContractGuardians([guardian3])]); const count = (await guardianManager.guardianCount(wallet.contractAddress)).toNumber(); diff --git a/test/lockManager.js b/test/lockManager.js index e05a24e8a..d688dc905 100644 --- a/test/lockManager.js +++ b/test/lockManager.js @@ -7,7 +7,7 @@ const Registry = require("../build/ModuleRegistry"); const RecoveryManager = require("../build/RecoveryManager"); const TestManager = require("../utils/test-manager"); -const { parseRelayReceipt } = require("../utils/utilities.js"); +const INVALID_SIGNATURES_REVERT_MSG = "RM: Invalid signatures"; describe("LockManager", function () { this.timeout(10000); @@ -109,9 +109,14 @@ describe("LockManager", function () { }); it("should fail to lock/unlock by Smart Contract guardians when signer is not authorized (relayed transaction)", async () => { - const txReceipt = await manager.relay(lockManager, "lock", [wallet.contractAddress], wallet, [nonguardian]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "locking from non-guardian should fail"); + await assert.revertWith( + manager.relay( + lockManager, + "lock", + [wallet.contractAddress], + wallet, + [nonguardian] + ), INVALID_SIGNATURES_REVERT_MSG); }); }); diff --git a/test/recoveryManager.js b/test/recoveryManager.js index 17d857287..2dfa5d7ea 100644 --- a/test/recoveryManager.js +++ b/test/recoveryManager.js @@ -10,6 +10,8 @@ const Registry = require("../build/ModuleRegistry"); const TestManager = require("../utils/test-manager"); const { sortWalletByAddress, parseRelayReceipt, signOffchain } = require("../utils/utilities.js"); +const WRONG_SIGNATURE_NUMBER_REVERT_MSG = "RM: Wrong number of signatures"; +const INVALID_SIGNATURES_REVERT_MSG = "RM: Invalid signatures"; describe("RecoveryManager", function () { this.timeout(10000); @@ -91,27 +93,43 @@ describe("RecoveryManager", function () { }); it("should not let owner execute the recovery procedure", async () => { - const txReceipt = await manager.relay(recoveryManager, "executeRecovery", [wallet.contractAddress, newowner.address], wallet, [owner]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "executeRecovery should fail"); + const expectedRevertMsg = guardians.length >= 3 ? WRONG_SIGNATURE_NUMBER_REVERT_MSG : INVALID_SIGNATURES_REVERT_MSG + await assert.revertWith( + manager.relay( + recoveryManager, + "executeRecovery", + [wallet.contractAddress, newowner.address], + wallet, + [owner] + ), expectedRevertMsg); }); it("should not let a majority of guardians and owner execute the recovery procedure", async () => { const majority = guardians.slice(0, Math.ceil((guardians.length) / 2) - 1); - const txReceipt = await manager.relay(recoveryManager, "executeRecovery", - [wallet.contractAddress, newowner.address], wallet, [owner, ...sortWalletByAddress(majority)]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "executeRecovery should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "executeRecovery", + [wallet.contractAddress, newowner.address], + wallet, + [owner, ...sortWalletByAddress(majority)] + ), INVALID_SIGNATURES_REVERT_MSG); + const isLocked = await lockManager.isLocked(wallet.contractAddress); assert.isFalse(isLocked, "should not be locked"); }); it("should not let a minority of guardians execute the recovery procedure", async () => { const minority = guardians.slice(0, Math.ceil((guardians.length) / 2) - 1); - const txReceipt = await manager.relay(recoveryManager, "executeRecovery", - [wallet.contractAddress, newowner.address], wallet, sortWalletByAddress(minority)); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "executeRecovery should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "executeRecovery", + [wallet.contractAddress, newowner.address], + wallet, + sortWalletByAddress(minority) + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); + const isLocked = await lockManager.isLocked(wallet.contractAddress); assert.isFalse(isLocked, "should not be locked"); }); @@ -173,34 +191,57 @@ describe("RecoveryManager", function () { }); it("should not let 1 guardian cancel the recovery procedure", async () => { - const txReceipt = await manager.relay(recoveryManager, "cancelRecovery", [wallet.contractAddress], wallet, [guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "cancelRecovery should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "cancelRecovery", + [wallet.contractAddress], + wallet, + [guardian1] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); + const isLocked = await lockManager.isLocked(wallet.contractAddress); assert.isTrue(isLocked, "should still be locked"); }); it("should not let the owner cancel the recovery procedure", async () => { - const txReceipt = await manager.relay(recoveryManager, "cancelRecovery", [wallet.contractAddress], wallet, [owner]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "cancelRecovery should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "cancelRecovery", + [wallet.contractAddress], + wallet, + [owner] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); + const isLocked = await lockManager.isLocked(wallet.contractAddress); assert.isTrue(isLocked, "should still be locked"); }); it("should not allow duplicate guardian signatures", async () => { - const txReceipt = await manager.relay(recoveryManager, "cancelRecovery", [wallet.contractAddress], wallet, [guardian1, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "cancelRecovery should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "cancelRecovery", + [wallet.contractAddress], + wallet, + [guardian1, guardian1] + ), INVALID_SIGNATURES_REVERT_MSG); + const isLocked = await lockManager.isLocked(wallet.contractAddress); assert.isTrue(isLocked, "should still be locked"); }); it("should not allow non guardians signatures", async () => { - const txReceipt = await manager.relay(recoveryManager, "cancelRecovery", - [wallet.contractAddress], wallet, sortWalletByAddress([guardian1, nonowner])); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "cancelRecovery should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "cancelRecovery", + [wallet.contractAddress], + wallet, + sortWalletByAddress([guardian1, nonowner]) + ), INVALID_SIGNATURES_REVERT_MSG); + const isLocked = await lockManager.isLocked(wallet.contractAddress); assert.isTrue(isLocked, "should still be locked"); }); @@ -218,26 +259,32 @@ describe("RecoveryManager", function () { it("should not let owner + minority of guardians execute an ownership transfer", async () => { const minority = guardians.slice(0, Math.ceil((guardians.length) / 2) - 1); - - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, newowner.address], wallet, [owner, ...sortWalletByAddress(minority)]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transferOwnership should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "transferOwnership", + [wallet.contractAddress, newowner.address], + wallet, + [owner, ...sortWalletByAddress(minority)] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); const walletOwner = await wallet.owner(); - assert.equal(walletOwner, owner.address, "owner should not have been changed"); + assert.equal(walletOwner, owner.address, 'owner should not have been changed'); }); it("should not let majority of guardians execute an ownership transfer without owner", async () => { const majority = guardians.slice(0, Math.ceil((guardians.length) / 2)); - - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, newowner.address], wallet, [...sortWalletByAddress(majority)]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transferOwnership should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "transferOwnership", + [wallet.contractAddress, newowner.address], + wallet, + [...sortWalletByAddress(majority)] + ), WRONG_SIGNATURE_NUMBER_REVERT_MSG); const walletOwner = await wallet.owner(); - assert.equal(walletOwner, owner.address, "owner should not have been changed"); + assert.equal(walletOwner, owner.address, 'owner should not have been changed'); }); } @@ -270,11 +317,15 @@ describe("RecoveryManager", function () { testExecuteRecovery([guardian1, guardian2, guardian3]); it("should not allow duplicate guardian signatures", async () => { - const badMajority = [guardian1, guardian1]; - const txReceipt = await manager.relay(recoveryManager, "executeRecovery", - [wallet.contractAddress, newowner.address], wallet, [...sortWalletByAddress(badMajority)]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "executeRecovery should fail"); + const badMajority = [guardian1, guardian1] + await assert.revertWith( + manager.relay( + recoveryManager, + "executeRecovery", + [wallet.contractAddress, newowner.address], + wallet, + [...sortWalletByAddress(badMajority)] + ), INVALID_SIGNATURES_REVERT_MSG); }); }); @@ -320,7 +371,6 @@ describe("RecoveryManager", function () { assert.isNotOk(success, "executeRecovery should fail"); }); - it("should revert if an unknown method is executed", async () => { const nonce = await manager.getNonceForRelay(); let methodData = recoveryManager.contract.interface.functions.executeRecovery.encode([wallet.contractAddress, ethers.constants.AddressZero]); @@ -341,9 +391,8 @@ describe("RecoveryManager", function () { describe("Finalize Recovery", () => { beforeEach(async () => { - await addGuardians([guardian1, guardian2, guardian3]); - await manager.relay(recoveryManager, "executeRecovery", - [wallet.contractAddress, newowner.address], wallet, sortWalletByAddress([guardian1, guardian2])); + await addGuardians([guardian1, guardian2, guardian3]) + await manager.relay(recoveryManager, 'executeRecovery', [wallet.contractAddress, newowner.address], wallet, sortWalletByAddress([guardian1, guardian2])); }); testFinalizeRecovery(); @@ -352,9 +401,8 @@ describe("RecoveryManager", function () { describe("Cancel Recovery with 3 guardians", () => { describe("EOA Guardians", () => { beforeEach(async () => { - await addGuardians([guardian1, guardian2, guardian3]); - await manager.relay(recoveryManager, "executeRecovery", - [wallet.contractAddress, newowner.address], wallet, sortWalletByAddress([guardian1, guardian2])); + await addGuardians([guardian1, guardian2, guardian3]) + await manager.relay(recoveryManager, 'executeRecovery', [wallet.contractAddress, newowner.address], wallet, sortWalletByAddress([guardian1, guardian2])); }); testCancelRecovery(); @@ -362,8 +410,7 @@ describe("RecoveryManager", function () { describe("Smart Contract Guardians", () => { beforeEach(async () => { await addGuardians(await createSmartContractGuardians([guardian1, guardian2, guardian3])); - await manager.relay(recoveryManager, "executeRecovery", - [wallet.contractAddress, newowner.address], wallet, sortWalletByAddress([guardian1, guardian2])); + await manager.relay(recoveryManager, 'executeRecovery', [wallet.contractAddress, newowner.address], wallet, sortWalletByAddress([guardian1, guardian2])); }); testCancelRecovery(); @@ -373,49 +420,63 @@ describe("RecoveryManager", function () { describe("Ownership Transfer", () => { it("should not allow transfer to an empty address", async () => { await addGuardians([guardian1]); - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, ethers.constants.AddressZero], wallet, [owner, guardian1]); + let txReceipt = await manager.relay(recoveryManager, 'transferOwnership', [wallet.contractAddress, ethers.constants.AddressZero], wallet, [owner, guardian1]); const success = parseRelayReceipt(txReceipt); assert.isNotOk(success, "transferOwnership should fail"); }); it("when no guardians, owner should be able to transfer alone", async () => { - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, newowner.address], wallet, [owner]); + let txReceipt = await manager.relay(recoveryManager, 'transferOwnership', [wallet.contractAddress, newowner.address], wallet, [owner]); const success = parseRelayReceipt(txReceipt); assert.isOk(success, "transferOwnership should succeed"); }); it("should not allow owner not signing", async () => { await addGuardians([guardian1]); - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, newowner.address], wallet, [nonowner, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transferOwnership should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "transferOwnership", + [wallet.contractAddress, newowner.address], + wallet, + [nonowner, guardian1] + ), INVALID_SIGNATURES_REVERT_MSG); }); it("should not allow duplicate owner signatures", async () => { await addGuardians([guardian1]); - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, newowner.address], wallet, [owner, owner]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transferOwnership should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "transferOwnership", + [wallet.contractAddress, newowner.address], + wallet, + [owner, owner] + ), INVALID_SIGNATURES_REVERT_MSG); }); it("should not allow duplicate guardian signatures", async () => { await addGuardians([guardian1, guardian2, guardian3]); - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, newowner.address], wallet, [owner, guardian1, guardian1]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transferOwnership should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "transferOwnership", + [wallet.contractAddress, newowner.address], + wallet, + [owner, guardian1, guardian1] + ), INVALID_SIGNATURES_REVERT_MSG); }); it("should not allow non guardian signatures", async () => { await addGuardians([guardian1]); - const txReceipt = await manager.relay(recoveryManager, "transferOwnership", - [wallet.contractAddress, newowner.address], wallet, [owner, nonowner]); - const success = parseRelayReceipt(txReceipt); - assert.isNotOk(success, "transferOwnership should fail"); + await assert.revertWith( + manager.relay( + recoveryManager, + "transferOwnership", + [wallet.contractAddress, newowner.address], + wallet, + [owner, nonowner] + ), INVALID_SIGNATURES_REVERT_MSG); }); describe("Guardians: G = 1", () => {