OOG invalidates proposal (#12)

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.

contracts/DaoModule.sol

@@ -44,46 +44,77 @@ 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
     mapping(bytes32 => bytes32) public questionIds;
     // 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()
     {
         require(timeout > 0, "Timeout has to be greater 0");
         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);
     }
-}
+}

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))

hardhat.config.ts

@@ -48,7 +48,7 @@ export default {
   },
   solidity: {
     compilers: [
-      { version: "0.8.1" },
+      { version: "0.8.0" },
       { version: "0.6.12" },
     ]
   },

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);
     });