From 7a5244d0a0a70b023d23af59659e0c055be7cca2 Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Tue, 9 Mar 2021 19:10:47 +0100 Subject: [PATCH] OOG invalidates proposal (#12) --- README.md | 13 ++ contracts/DaoModule.sol | 82 ++++++-- contracts/test/TestExecutor.sol | 2 +- hardhat.config.ts | 2 +- src/tasks/setup.ts | 3 +- test/DaoModule.spec.ts | 362 +++++++++++++++++++++++++++++--- 6 files changed, 418 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 8929a0e..d718813 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,19 @@ The reference oracle implementations are the Realitio contracts. These can be fo - Ether: https://github.com/realitio/realitio-contracts/blob/master/truffle/contracts/Realitio.sol - ERC20: https://github.com/realitio/realitio-contracts/blob/master/truffle/contracts/RealitioERC20.sol +### Failed transactions + +It is required that the transactions from a proposal are successful (should not internally revert for any reason). If any of the transactions of a proposal fail it will not be possible to continue with the execution of the following transactions. This is to prevent subsequent transactions being executed in a scenario where earlier transactions failed due to the gas limit being too low or due to other errors. + +Transactions that failed will *not* be marked as executed and therefore can be executed at any later point in time. This is a potential risk and therefore it is recommended to either set an answer expiration time or invalidate the proposal (e.g. via another proposal). + +### Answer expiration + +The module can be configured so that positive answers will expire after a certain time. This can be done by calling `setAnswerExpiration` with a time duration in seconds. If the transactions related to the proposal are not executed before the answer expires, it will not be possible to execute them. This is useful in the case of transactions that revert and therefore cannot be executed, to prevent that they are unexpectedly executed in the future. Negative answers (no or invalid) cannot expire. + +Note: If the expiration time is set to `0` answers will never expire. This also means answers that expired before will become available again. To prevent this, it is recommended to call `markProposalWithExpiredAnswerAsInvalid` immediately after any proposal expires (or on all outstanding expired answers prior to setting the expiration date to `0`), this will mark a proposal with an expired answer as invalid. This method can be called by anyone. + + ### EIP-712 details [EIP-712](https://github.com/Ethereum/EIPs/blob/master/EIPS/eip-712.md) is used to generate the hashes for the transactions to be executed. The following EIP-712 domain and types are used. diff --git a/contracts/DaoModule.sol b/contracts/DaoModule.sol index b95a07e..2befde3 100644 --- a/contracts/DaoModule.sol +++ b/contracts/DaoModule.sol @@ -44,6 +44,7 @@ contract DaoModule { uint256 public template; uint32 public questionTimeout; uint32 public questionCooldown; + uint32 public answerExpiration; address public questionArbitrator; uint256 public minimumBond; // Mapping of question hash to question id. Special case: INVALIDATED for question hashes that have been invalidated @@ -51,24 +52,34 @@ contract DaoModule { // Mapping of questionHash to transactionHash to execution state mapping(bytes32 => mapping(bytes32 => bool)) public executedProposalTransactions; - constructor(Executor _executor, Realitio _oracle, uint32 timeout, uint32 cooldown, uint256 bond, uint256 templateId) { + /// @param _executor Address of the executor (e.g. a Safe) + /// @param _oracle Address of the oracle (e.g. Realitio) + /// @param timeout Timeout in seconds that should be required for the oracle + /// @param cooldown Cooldown in seconds that should be required after a oracle provided answer + /// @param expiration Duration that a positive answer of the oracle is valid in seconds (or 0 if valid forever) + /// @param bond Minimum bond that is required for an answer to be accepted + /// @param templateId ID of the template that should be used for proposal questions (see https://github.com/realitio/realitio-dapp#structuring-and-fetching-information) + /// @notice There need to be at least 60 seconds between end of cooldown and expiration + constructor(Executor _executor, Realitio _oracle, uint32 timeout, uint32 cooldown, uint32 expiration, uint256 bond, uint256 templateId) { require(timeout > 0, "Timeout has to be greater 0"); + require(expiration == 0 || expiration - cooldown >= 60 , "There need to be at least 60s between end of cooldown and expiration"); executor = _executor; oracle = _oracle; + answerExpiration = expiration; questionTimeout = timeout; questionCooldown = cooldown; questionArbitrator = address(_executor); minimumBond = bond; template = templateId; } - + modifier executorOnly() { require(msg.sender == address(executor), "Not authorized"); _; } /// @notice This can only be called by the executor - function setQuestionTimeout(uint32 timeout) + function setQuestionTimeout(uint32 timeout) public executorOnly() { @@ -76,14 +87,34 @@ contract DaoModule { questionTimeout = timeout; } + /// @dev Sets the cooldown before an answer is usable. + /// @param cooldown Cooldown in seconds that should be required after a oracle provided answer /// @notice This can only be called by the executor - function setQuestionCooldown(uint32 cooldown) + /// @notice There need to be at least 60 seconds between end of cooldown and expiration + function setQuestionCooldown(uint32 cooldown) public executorOnly() { + uint32 expiration = answerExpiration; + require(expiration == 0 || expiration - cooldown >= 60 , "There need to be at least 60s between end of cooldown and expiration"); questionCooldown = cooldown; } + /// @dev Sets the duration for which a positive answer is valid. + /// @param expiration Duration that a positive answer of the oracle is valid in seconds (or 0 if valid forever) + /// @notice A proposal with an expired answer is the same as a proposal that has been marked invalid + /// @notice There need to be at least 60 seconds between end of cooldown and expiration + /// @notice This can only be called by the executor + function setAnswerExpiration(uint32 expiration) + public + executorOnly() + { + require(expiration == 0 || expiration - questionCooldown >= 60 , "There need to be at least 60s between end of cooldown and expiration"); + answerExpiration = expiration; + } + + /// @dev Sets the question arbitrator that will be used for future questions. + /// @param arbitrator Address of the arbitrator /// @notice This can only be called by the executor function setArbitrator(address arbitrator) public @@ -92,6 +123,8 @@ contract DaoModule { questionArbitrator = arbitrator; } + /// @dev Sets the minimum bond that is required for an answer to be accepted. + /// @param bond Minimum bond that is required for an answer to be accepted /// @notice This can only be called by the executor function setMinimumBond(uint256 bond) public @@ -100,6 +133,9 @@ contract DaoModule { minimumBond = bond; } + /// @dev Sets the template that should be used for future questions. + /// @param templateId ID of the template that should be used for proposal questions + /// @notice Check https://github.com/realitio/realitio-dapp#structuring-and-fetching-information for more information /// @notice This can only be called by the executor function setTemplate(uint256 templateId) public @@ -152,8 +188,8 @@ contract DaoModule { /// @param proposalId Id that should identify the proposal uniquely /// @param txHashes EIP-712 hashes of the transactions that should be executed /// @notice This can only be called by the executor - function markProposalAsInvalid(string memory proposalId, bytes32[] memory txHashes) - public + function markProposalAsInvalid(string memory proposalId, bytes32[] memory txHashes) + public // Executor only is checked in markProposalAsInvalidByHash(bytes32) { string memory question = buildQuestion(proposalId, txHashes); @@ -164,13 +200,29 @@ contract DaoModule { /// @dev Marks a question hash as invalid, preventing execution of the connected transactions /// @param questionHash Question hash calculated based on the proposal id and txHashes /// @notice This can only be called by the executor - function markProposalAsInvalidByHash(bytes32 questionHash) - public + function markProposalAsInvalidByHash(bytes32 questionHash) + public executorOnly() { questionIds[questionHash] = INVALIDATED; } + /// @dev Marks a proposal with an expired answer as invalid, preventing execution of the connected transactions + /// @param questionHash Question hash calculated based on the proposal id and txHashes + function markProposalWithExpiredAnswerAsInvalid(bytes32 questionHash) + public + { + uint32 expirationDuration = answerExpiration; + require(expirationDuration > 0, "Answers are valid forever"); + bytes32 questionId = questionIds[questionHash]; + require(questionId != INVALIDATED, "Proposal is already invalidated"); + require(questionId != bytes32(0), "No question id set for provided proposal"); + require(oracle.resultFor(questionId) == bytes32(uint256(1)), "Only positive answers can expire"); + uint32 finalizeTs = oracle.getFinalizeTS(questionId); + require(finalizeTs + uint256(expirationDuration) < block.timestamp, "Answer has not expired yet"); + questionIds[questionHash] = INVALIDATED; + } + /// @dev Executes the transactions of a proposal via the executor if accepted /// @param proposalId Id that should identify the proposal uniquely /// @param txHashes EIP-712 hashes of the transactions that should be executed @@ -208,16 +260,18 @@ contract DaoModule { uint256 minBond = minimumBond; require(minBond == 0 || minBond <= oracle.getBond(questionId), "Bond on question not high enough"); uint32 finalizeTs = oracle.getFinalizeTS(questionId); + // The answer is valid in the time after the cooldown and before the expiration time (if set). require(finalizeTs + uint256(questionCooldown) < block.timestamp, "Wait for additional cooldown"); + uint32 expiration = answerExpiration; + require(expiration == 0 || finalizeTs + uint256(expiration) >= block.timestamp, "Answer has expired"); // Check this is either the first transaction in the list or that the previous question was already approved require(txIndex == 0 || executedProposalTransactions[questionHash][txHashes[txIndex - 1]], "Previous transaction not executed yet"); // Check that this question was not executed yet require(!executedProposalTransactions[questionHash][txHash], "Cannot execute transaction again"); // Mark transaction as executed executedProposalTransactions[questionHash][txHash] = true; - // Execute the transaction via the executor. We do not care about the return value (indicating if the internal tx was a success). - // But if the transaction reverts it will be propagated up (in case this module was not allowed to execute transactions). - executor.execTransactionFromModule(to, value, data, operation); + // Execute the transaction via the executor. + require(executor.execTransactionFromModule(to, value, data, operation), "Module transaction failed"); } /// @dev Build the question by combining the proposalId and the hex string of the hash of the txHashes @@ -234,7 +288,7 @@ contract DaoModule { bytes32 contentHash = keccak256(abi.encodePacked(templateId, openingTs, question)); return keccak256(abi.encodePacked(contentHash, arbitrator, timeout, this, nonce)); } - + /// @dev Returns the chain id used by this contract. function getChainId() public view returns (uint256) { uint256 id; @@ -244,7 +298,7 @@ contract DaoModule { } return id; } - + /// @dev Generates the data for the module transaction hash (required for signing) function generateTransactionHashData( address to, @@ -281,4 +335,4 @@ contract DaoModule { if (b < 10) return bytes1(b + 0x30); else return bytes1(b + 0x57); } -} \ No newline at end of file +} diff --git a/contracts/test/TestExecutor.sol b/contracts/test/TestExecutor.sol index ce743e6..03aed35 100644 --- a/contracts/test/TestExecutor.sol +++ b/contracts/test/TestExecutor.sol @@ -13,7 +13,7 @@ contract TestExecutor { function exec(address payable to, uint256 value, bytes calldata data) external { bool success; bytes memory response; - (success,response) = to.call{value: value}(data); + (success, response) = to.call{value: value}(data); if(!success) { assembly { revert(add(response, 0x20), mload(response)) diff --git a/hardhat.config.ts b/hardhat.config.ts index 9ac2d39..d5077a7 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -48,7 +48,7 @@ export default { }, solidity: { compilers: [ - { version: "0.8.1" }, + { version: "0.8.0" }, { version: "0.6.12" }, ] }, diff --git a/src/tasks/setup.ts b/src/tasks/setup.ts index 98e8b0e..1e1ecbc 100644 --- a/src/tasks/setup.ts +++ b/src/tasks/setup.ts @@ -8,6 +8,7 @@ task("setup", "Provides the clearing price to an auction") .addParam("oracle", "Address of the oracle (e.g. Realitio)", undefined, types.string) .addParam("timeout", "Timeout in seconds that should be required for the oracle", 48 * 3600, types.int, true) .addParam("cooldown", "Cooldown in seconds that should be required after a oracle provided answer", 24 * 3600, types.int, true) + .addParam("expiration", "Time duration in seconds for which a positive answer is valid. After this time the answer is expired", 7 * 24 * 3600, types.int, true) .addParam("bond", "Minimum bond that is required for an answer to be accepted", "0", types.string, true) .addParam( "template", @@ -20,7 +21,7 @@ task("setup", "Provides the clearing price to an auction") const [caller] = await hardhatRuntime.ethers.getSigners(); console.log("Using the account:", caller.address); const Module = await hardhatRuntime.ethers.getContractFactory("DaoModule"); - const module = await Module.deploy(taskArgs.dao, taskArgs.oracle, taskArgs.timeout, taskArgs.cooldown, taskArgs.bond, taskArgs.template); + const module = await Module.deploy(taskArgs.dao, taskArgs.oracle, taskArgs.timeout, taskArgs.cooldown, taskArgs.expiration, taskArgs.bond, taskArgs.template); console.log("Module deployed to:", module.address); }); diff --git a/test/DaoModule.spec.ts b/test/DaoModule.spec.ts index 3f73ad5..2fe085e 100644 --- a/test/DaoModule.spec.ts +++ b/test/DaoModule.spec.ts @@ -1,7 +1,7 @@ import { expect } from "chai"; import hre, { deployments, ethers, waffle } from "hardhat"; import "@nomiclabs/hardhat-ethers"; -import { nextBlockTime } from "./utils"; +import { logGas, nextBlockTime } from "./utils"; import { _TypedDataEncoder } from "@ethersproject/hash"; const EIP712_TYPES = { @@ -47,24 +47,36 @@ describe("DaoModule", async () => { const setupTestWithTestExecutor = deployments.createFixture(async () => { const base = await baseSetup(); const Module = await hre.ethers.getContractFactory("DaoModule"); - const module = await Module.deploy(base.executor.address, base.mock.address, 42, 23, 0, 1337); + const module = await Module.deploy(base.executor.address, base.mock.address, 42, 23, 0, 0, 1337); return { ...base, Module, module }; }) const setupTestWithMockExecutor = deployments.createFixture(async () => { const base = await baseSetup(); const Module = await hre.ethers.getContractFactory("DaoModule"); - const module = await Module.deploy(base.mock.address, base.mock.address, 42, 23, 0, 1337); + const module = await Module.deploy(base.mock.address, base.mock.address, 42, 23, 0, 0, 1337); return { ...base, Module, module }; }) const [user1] = waffle.provider.getWallets(); describe("constructor", async () => { it("throws if timeout is 0", async () => { - const Module = await hre.ethers.getContractFactory("DaoModule"); + const Module = await hre.ethers.getContractFactory("DaoModule") await expect( - Module.deploy(user1.address, user1.address, 0, 0, 0, 0) - ).to.be.revertedWith("Timeout has to be greater 0"); + Module.deploy(user1.address, user1.address, 0, 0, 0, 0, 0) + ).to.be.revertedWith("Timeout has to be greater 0") + }) + + it("throws if not enough time between cooldown and expiration", async () => { + const Module = await hre.ethers.getContractFactory("DaoModule") + await expect( + Module.deploy(user1.address, user1.address, 1, 0, 59, 0, 0) + ).to.be.revertedWith("There need to be at least 60s between end of cooldown and expiration") + }) + + it("answer expiration can be 0", async () => { + const Module = await hre.ethers.getContractFactory("DaoModule") + await Module.deploy(user1.address, user1.address, 1, 10, 0, 0, 0) }) }) @@ -108,6 +120,46 @@ describe("DaoModule", async () => { ).to.be.revertedWith("Not authorized"); }) + it("throws if not enough time between cooldown and expiration", async () => { + const { module, executor } = await setupTestWithTestExecutor(); + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [100]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const setQuestionCooldownInvalid = module.interface.encodeFunctionData("setQuestionCooldown", [41]) + await expect( + executor.exec(module.address, 0, setQuestionCooldownInvalid) + ).to.be.revertedWith("There need to be at least 60s between end of cooldown and expiration") + + const setQuestionCooldown = module.interface.encodeFunctionData("setQuestionCooldown", [40]) + await executor.exec(module.address, 0, setQuestionCooldown) + + expect( + await module.questionCooldown() + ).to.be.equals(40); + }) + + it("can reset to 0", async () => { + const { module, executor } = await setupTestWithTestExecutor(); + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [100]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const setQuestionCooldown = module.interface.encodeFunctionData("setQuestionCooldown", [40]) + await executor.exec(module.address, 0, setQuestionCooldown) + + expect( + await module.questionCooldown() + ).to.be.equals(40); + + const resetQuestionCooldown = module.interface.encodeFunctionData("setQuestionCooldown", [0]) + await executor.exec(module.address, 0, resetQuestionCooldown) + + expect( + await module.questionCooldown() + ).to.be.equals(0); + }) + it("updates question cooldown", async () => { const { module, executor } = await setupTestWithTestExecutor(); @@ -124,6 +176,49 @@ describe("DaoModule", async () => { }) }) + describe("setAnswerExpiration", async () => { + it("throws if not authorized", async () => { + const { module } = await setupTestWithTestExecutor(); + await expect( + module.setAnswerExpiration(2) + ).to.be.revertedWith("Not authorized"); + }) + + it("throws if not enough time between cooldown and expiration", async () => { + const { module, executor } = await setupTestWithTestExecutor(); + + const setQuestionCooldown = module.interface.encodeFunctionData("setQuestionCooldown", [40]) + await executor.exec(module.address, 0, setQuestionCooldown) + + const setAnswerExpirationInvalid = module.interface.encodeFunctionData("setAnswerExpiration", [99]) + await expect( + executor.exec(module.address, 0, setAnswerExpirationInvalid) + ).to.be.revertedWith("There need to be at least 60s between end of cooldown and expiration") + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [100]) + await executor.exec(module.address, 0, setAnswerExpiration) + + expect( + await module.answerExpiration() + ).to.be.equals(100); + }) + + it("updates question cooldown", async () => { + const { module, executor } = await setupTestWithTestExecutor(); + + expect( + await module.answerExpiration() + ).to.be.equals(0); + + const calldata = module.interface.encodeFunctionData("setAnswerExpiration", [511]) + await executor.exec(module.address, 0, calldata) + + expect( + await module.answerExpiration() + ).to.be.equals(511); + }) + }) + describe("setArbitrator", async () => { it("throws if not authorized", async () => { const { module } = await setupTestWithTestExecutor(); @@ -260,7 +355,7 @@ describe("DaoModule", async () => { const { module, executor } = await setupTestWithTestExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(question)) @@ -302,6 +397,131 @@ describe("DaoModule", async () => { }) }) + describe.only("markProposalWithExpiredAnswerAsInvalid", async () => { + it("throws if answer cannot expire", async () => { + const { module } = await setupTestWithTestExecutor(); + + const id = "some_random_id"; + const txHash = ethers.utils.solidityKeccak256(["string"], ["some_tx_data"]); + const question = await module.buildQuestion(id, [txHash]); + const questionHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(question)) + + await expect( + module.markProposalWithExpiredAnswerAsInvalid(questionHash) + ).to.be.revertedWith("Answers are valid forever"); + }) + + it("throws if answer is already invalidated", async () => { + const { module, executor } = await setupTestWithTestExecutor(); + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [90]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const id = "some_random_id"; + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } + const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) + const question = await module.buildQuestion(id, [txHash]); + const questionHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(question)) + + const markProposalAsInvalidByHash = module.interface.encodeFunctionData("markProposalAsInvalidByHash", [questionHash]) + await executor.exec(module.address, 0, markProposalAsInvalidByHash) + + await expect( + module.markProposalWithExpiredAnswerAsInvalid(questionHash) + ).to.be.revertedWith("Proposal is already invalidated"); + }) + + it("throws if question is unknown", async () => { + const { module, executor } = await setupTestWithTestExecutor(); + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [90]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const id = "some_random_id"; + const txHash = ethers.utils.solidityKeccak256(["string"], ["some_tx_data"]); + const question = await module.buildQuestion(id, [txHash]); + const questionHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(question)) + + await expect( + module.markProposalWithExpiredAnswerAsInvalid(questionHash) + ).to.be.revertedWith("No question id set for provided proposal"); + }) + + it("throws if answer was not accepted", async () => { + const { mock, module, executor, oracle } = await setupTestWithTestExecutor(); + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [90]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const id = "some_random_id"; + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } + const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) + const question = await module.buildQuestion(id, [txHash]); + const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) + const questionHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(question)) + + const block = await ethers.provider.getBlock("latest") + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) + await mock.givenMethodReturnUint(oracle.interface.getSighash("resultFor"), INVALIDATED_STATE) + await module.addProposal(id, [txHash]) + + await expect( + module.markProposalWithExpiredAnswerAsInvalid(questionHash) + ).to.be.revertedWith("Only positive answers can expire"); + }) + + it("throws if answer is not expired", async () => { + const { mock, module, executor, oracle } = await setupTestWithTestExecutor(); + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [90]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const id = "some_random_id"; + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } + const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) + const question = await module.buildQuestion(id, [txHash]); + const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) + const questionHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(question)) + + const block = await ethers.provider.getBlock("latest") + await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) + await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) + await module.addProposal(id, [txHash]) + + await expect( + module.markProposalWithExpiredAnswerAsInvalid(questionHash) + ).to.be.revertedWith("Answer has not expired yet"); + }) + + it("can mark proposal with expired accepted answer as invalid", async () => { + const { mock, module, executor, oracle } = await setupTestWithTestExecutor(); + + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [90]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const id = "some_random_id"; + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } + const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) + const question = await module.buildQuestion(id, [txHash]); + const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) + const questionHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(question)) + + const block = await ethers.provider.getBlock("latest") + await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) + await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) + await module.addProposal(id, [txHash]) + + await nextBlockTime(hre, block.timestamp + 91) + + await module.markProposalWithExpiredAnswerAsInvalid(questionHash); + expect( + await module.questionIds(questionHash) + ).to.be.deep.equals(INVALIDATED_STATE); + }) + }) + describe("getTransactionHash", async () => { it("correctly generates hash for tx without data", async () => { const { module } = await setupTestWithTestExecutor(); @@ -310,7 +530,7 @@ describe("DaoModule", async () => { "chainId": chainId, "verifyingContract": module.address, } - const tx = { to: user1.address, value: 42, data: "0x", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0x", operation: 0, nonce: 0 } expect( await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) ).to.be.equals(_TypedDataEncoder.hash(domain, EIP712_TYPES, tx)); @@ -400,7 +620,7 @@ describe("DaoModule", async () => { await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId); await module.addProposal(id, [txHash]) - + const updateQuestionTimeout = module.interface.encodeFunctionData( "setQuestionTimeout", [31] @@ -467,7 +687,7 @@ describe("DaoModule", async () => { const previousQuestionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), previousQuestionId) await module.addProposal(id, [txHash]) - + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) const resultForCalldata = oracle.interface.encodeFunctionData("resultFor", [previousQuestionId]) await mock.givenCalldataReturnUint(resultForCalldata, INVALIDATED_STATE) @@ -539,7 +759,7 @@ describe("DaoModule", async () => { const previousQuestionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), previousQuestionId) await module.addProposal(id, [txHash]) - + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) await mock.givenCalldataReturnUint(oracle.interface.encodeFunctionData("resultFor", [previousQuestionId]), INVALIDATED_STATE) @@ -549,7 +769,7 @@ describe("DaoModule", async () => { expect( await module.questionIds(questionHash) ).to.be.deep.equals(questionId) - + // Nonce doesn't need to increase 1 by 1 const finalQuestionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 1337) await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), finalQuestionId) @@ -611,7 +831,7 @@ describe("DaoModule", async () => { const previousQuestionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), previousQuestionId) await module.addProposal(id, [txHash]) - + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) await mock.givenCalldataReturnUint(oracle.interface.encodeFunctionData("resultFor", [previousQuestionId]), INVALIDATED_STATE) @@ -621,7 +841,7 @@ describe("DaoModule", async () => { expect( await module.questionIds(questionHash) ).to.be.deep.equals(questionId) - + await mock.givenCalldataReturnBool(oracle.interface.encodeFunctionData("resultFor", [questionId]), true) await expect( @@ -635,7 +855,7 @@ describe("DaoModule", async () => { const { module } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) await expect( @@ -647,7 +867,7 @@ describe("DaoModule", async () => { const { executor, mock, module, oracle } = await setupTestWithTestExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) @@ -670,7 +890,7 @@ describe("DaoModule", async () => { const { executor, mock, module, oracle } = await setupTestWithTestExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) @@ -703,7 +923,7 @@ describe("DaoModule", async () => { const { mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 1 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 1 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, mock.address, 42, 0, 0) @@ -720,7 +940,7 @@ describe("DaoModule", async () => { const { mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, []); const questionId = await module.getQuestionId(1337, question, mock.address, 42, 0, 0) @@ -739,7 +959,7 @@ describe("DaoModule", async () => { const { mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, mock.address, 42, 0, 0) @@ -758,7 +978,7 @@ describe("DaoModule", async () => { const { executor, mock, module, oracle } = await setupTestWithTestExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) @@ -783,7 +1003,7 @@ describe("DaoModule", async () => { const { executor, mock, module, oracle } = await setupTestWithTestExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) @@ -816,7 +1036,7 @@ describe("DaoModule", async () => { const { mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, mock.address, 42, 0, 0) @@ -833,11 +1053,57 @@ describe("DaoModule", async () => { ).to.be.revertedWith("Wait for additional cooldown"); }) + it("throws if answer expired", async () => { + const { mock, module, oracle, executor } = await setupTestWithTestExecutor(); + + await user1.sendTransaction({ to: executor.address, value: 100 }) + await executor.setModule(module.address) + const setAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [90]) + await executor.exec(module.address, 0, setAnswerExpiration) + + const id = "some_random_id"; + const tx = { to: mock.address, value: 42, data: "0x", operation: 0, nonce: 0 } + const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) + const question = await module.buildQuestion(id, [txHash]); + const questionId = await module.getQuestionId(1337, question, executor.address, 42, 0, 0) + + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) + await module.addProposal(id, [txHash]) + const block = await ethers.provider.getBlock("latest") + await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) + await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnBool(executor.interface.getSighash("execTransactionFromModule"), true) + await nextBlockTime(hre, block.timestamp + 91) + await expect( + module.executeProposal(id, [txHash], tx.to, tx.value, tx.data, tx.operation) + ).to.be.revertedWith("Answer has expired"); + + // Reset answer expiration time, so that we can execute the transaction + const resetAnswerExpiration = module.interface.encodeFunctionData("setAnswerExpiration", [0]) + await executor.exec(module.address, 0, resetAnswerExpiration) + + expect( + (await mock.callStatic.invocationCount()).toNumber() + ).to.be.equals(1) + expect( + (await hre.ethers.provider.getBalance(mock.address)).toNumber() + ).to.be.equals(0) + + await module.executeProposal(id, [txHash], tx.to, tx.value, tx.data, tx.operation) + + expect( + (await mock.callStatic.invocationCount()).toNumber() + ).to.be.equals(2) + expect( + (await hre.ethers.provider.getBalance(mock.address)).toNumber() + ).to.be.equals(42) + }) + it("throws if tx was already executed for that question", async () => { - const { mock, module, oracle } = await setupTestWithMockExecutor(); + const { mock, module, oracle, executor } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, mock.address, 42, 0, 0) @@ -847,6 +1113,7 @@ describe("DaoModule", async () => { const block = await ethers.provider.getBlock("latest") await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnBool(executor.interface.getSighash("execTransactionFromModule"), true) await nextBlockTime(hre, block.timestamp + 24) await module.executeProposal(id, [txHash], tx.to, tx.value, tx.data, tx.operation); await expect( @@ -854,11 +1121,45 @@ describe("DaoModule", async () => { ).to.be.revertedWith("Cannot execute transaction again"); }) + it("throws if module transaction failed", async () => { + const { executor, mock, module, oracle } = await setupTestWithMockExecutor(); + + const id = "some_random_id"; + const tx = { to: mock.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } + const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) + const question = await module.buildQuestion(id, [txHash]); + const questionId = await module.getQuestionId(1337, question, mock.address, 42, 0, 0) + + await mock.givenMethodReturnUint(oracle.interface.getSighash("askQuestion"), questionId) + await module.addProposal(id, [txHash]) + const block = await ethers.provider.getBlock("latest") + await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) + await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnBool(executor.interface.getSighash("execTransactionFromModule"), false) + await nextBlockTime(hre, block.timestamp + 24) + expect( + (await mock.callStatic.invocationCount()).toNumber() + ).to.be.equals(1) + await expect( + module.executeProposalWithIndex(id, [txHash], tx.to, tx.value, tx.data, tx.operation, tx.nonce) + ).to.be.revertedWith("Module transaction failed"); + expect( + await module.executedProposalTransactions(ethers.utils.solidityKeccak256(["string"], [question]), txHash) + ).to.be.equals(false) + + // Return success and check that it can be executed + await mock.givenMethodReturnBool(executor.interface.getSighash("execTransactionFromModule"), true) + await module.executeProposalWithIndex(id, [txHash], tx.to, tx.value, tx.data, tx.operation, tx.nonce); + expect( + await module.executedProposalTransactions(ethers.utils.solidityKeccak256(["string"], [question]), txHash) + ).to.be.equals(true) + }) + it("triggers module transaction", async () => { const { executor, mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const txHash = await module.getTransactionHash(tx.to, tx.value, tx.data, tx.operation, tx.nonce) const question = await module.buildQuestion(id, [txHash]); const questionId = await module.getQuestionId(1337, question, mock.address, 42, 0, 0) @@ -870,6 +1171,7 @@ describe("DaoModule", async () => { await mock.reset() await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnBool(executor.interface.getSighash("execTransactionFromModule"), true) await nextBlockTime(hre, block.timestamp + 23) await expect( module.executeProposal(id, [txHash], tx.to, tx.value, tx.data, tx.operation) @@ -902,7 +1204,7 @@ describe("DaoModule", async () => { const { mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx1 = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx1 = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const tx1Hash = await module.getTransactionHash(tx1.to, tx1.value, tx1.data, tx1.operation, tx1.nonce) const tx2 = { to: user1.address, value: 23, data: "0xdeaddeed", operation: 0, nonce: 1 } const tx2Hash = await module.getTransactionHash(tx2.to, tx2.value, tx2.data, tx2.operation, tx2.nonce) @@ -924,7 +1226,7 @@ describe("DaoModule", async () => { const { executor, mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx1 = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx1 = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const tx1Hash = await module.getTransactionHash(tx1.to, tx1.value, tx1.data, tx1.operation, tx1.nonce) const tx2 = { to: user1.address, value: 23, data: "0xdeaddeed", operation: 0, nonce: 1 } const tx2Hash = await module.getTransactionHash(tx2.to, tx2.value, tx2.data, tx2.operation, tx2.nonce) @@ -937,6 +1239,7 @@ describe("DaoModule", async () => { await mock.reset() await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnBool(executor.interface.getSighash("execTransactionFromModule"), true) await nextBlockTime(hre, block.timestamp + 24) await module.executeProposal(id, [tx1Hash, tx2Hash], tx1.to, tx1.value, tx1.data, tx1.operation) @@ -975,7 +1278,7 @@ describe("DaoModule", async () => { const { executor, mock, module, oracle } = await setupTestWithMockExecutor(); const id = "some_random_id"; - const tx1 = { to: user1.address, value: 42, data: "0xbaddad", operation: 0, nonce: 0 } + const tx1 = { to: user1.address, value: 0, data: "0xbaddad", operation: 0, nonce: 0 } const tx1Hash = await module.getTransactionHash(tx1.to, tx1.value, tx1.data, tx1.operation, tx1.nonce) const tx2 = { ...tx1, nonce: 1 } const tx2Hash = await module.getTransactionHash(tx2.to, tx2.value, tx2.data, tx2.operation, tx2.nonce) @@ -990,6 +1293,7 @@ describe("DaoModule", async () => { await mock.reset() await mock.givenMethodReturnBool(oracle.interface.getSighash("resultFor"), true) await mock.givenMethodReturnUint(oracle.interface.getSighash("getFinalizeTS"), block.timestamp) + await mock.givenMethodReturnBool(executor.interface.getSighash("execTransactionFromModule"), true) await nextBlockTime(hre, block.timestamp + 24) await module.executeProposal(id, [tx1Hash, tx2Hash], tx1.to, tx1.value, tx1.data, tx1.operation)