refactor: copy CompTimelock's behaviour in CompTimelockCompatibleExec…

…utionStrategy (#227)
snapshot-labs · Jun 27, 2023 · ff99184 · ff99184
1 parent aff110d
commit ff99184
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 39 deletions.
diff --git a/src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol b/src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol
@@ -5,7 +5,7 @@ pragma solidity ^0.8.18;
 import { ICompTimelock } from "../interfaces/ICompTimelock.sol";
 import { SimpleQuorumExecutionStrategy } from "./SimpleQuorumExecutionStrategy.sol";
 import { SpaceManager } from "../utils/SpaceManager.sol";
-import { MetaTransaction, Proposal, ProposalStatus } from "../types.sol";
+import { MetaTransaction, Proposal, ProposalStatus, TRUE, FALSE } from "../types.sol";
 import { Enum } from "@gnosis.pm/safe-contracts/contracts/common/Enum.sol";
 
 /// @title Comp Timelock Execution Strategy
@@ -42,6 +42,9 @@ contract CompTimelockCompatibleExecutionStrategy is SimpleQuorumExecutionStrateg
     /// @notice The time at which a proposal can be executed. Indexed by the hash of the proposal execution payload.
     mapping(bytes32 => uint256) public proposalExecutionTime;
 
+    /// @notice Mapping of queued transaction hashes.
+    mapping(bytes32 => uint256) public txHashes;
+
     /// @notice Veto guardian is given permission to veto any queued proposal.
     address public vetoGuardian;
 
@@ -104,24 +107,23 @@ contract CompTimelockCompatibleExecutionStrategy is SimpleQuorumExecutionStrateg
 
         MetaTransaction[] memory transactions = abi.decode(payload, (MetaTransaction[]));
 
-        // An array to check that there are no duplicates in the list of transactions.
-        // We must do this because the Compound Timelock will silently "merge" two duplicate transactions.
-        // This could be problematic for off-chain indexers and UI tools.
-        bytes32[] memory txHashes = new bytes32[](transactions.length);
-
         for (uint256 i = 0; i < transactions.length; i++) {
             // Comp Timelock does not support delegate calls.
             if (transactions[i].operation == Enum.Operation.DelegateCall) {
                 revert InvalidTransaction();
             }
 
             // Check there are not duplicates.
-            // Do not include `executionTime` because it's a constant.
-            bytes32 txHash = keccak256(abi.encode(transactions[i].to, transactions[i].value, "", transactions[i].data));
-            for (uint256 j = 0; j < i; j++) {
-                if (txHashes[j] == txHash) revert DuplicateMetaTransaction();
-            }
-            txHashes[i] = txHash;
+            // We must do this because the Compound Timelock will silently "merge" two duplicate transactions.
+            // This could be problematic for off-chain indexers and UI tools.
+            bytes32 txHash = keccak256(
+                abi.encode(transactions[i].to, transactions[i].value, "", transactions[i].data, executionTime)
+            );
+            // We use `!= FALSE` rather than `== TRUE` for gas optimisations.
+            if (txHashes[txHash] != FALSE) revert DuplicateMetaTransaction();
+
+            // Store the transaction hash.
+            txHashes[txHash] = TRUE;
 
             timelock.queueTransaction(
                 transactions[i].to,
@@ -154,6 +156,12 @@ contract CompTimelockCompatibleExecutionStrategy is SimpleQuorumExecutionStrateg
 
         MetaTransaction[] memory transactions = abi.decode(payload, (MetaTransaction[]));
         for (uint256 i = 0; i < transactions.length; i++) {
+            // Clear out the transactions from the mapping.
+            bytes32 txHash = keccak256(
+                abi.encode(transactions[i].to, transactions[i].value, "", transactions[i].data, executionTime)
+            );
+            txHashes[txHash] = FALSE;
+
             timelock.executeTransaction(
                 transactions[i].to,
                 transactions[i].value,

diff --git a/test/CompTimelockExecutionStrategy.t.sol b/test/CompTimelockExecutionStrategy.t.sol
@@ -116,6 +116,43 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest {
         space.execute(proposalId, abi.encode(transactions));
     }
 
+    function testQueueingDuplicateMetaTransactionDifferentProposals() external {
+        MetaTransaction[] memory transactions = new MetaTransaction[](1);
+        transactions[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 0);
+
+        MetaTransaction[] memory duplicateTransactions = new MetaTransaction[](1);
+        // Salt differs but content does not
+        duplicateTransactions[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 1);
+
+        // Create a first proposal with some transactions, and vote on it.
+        uint256 firstProposalId = _createProposal(
+            author,
+            proposalMetadataURI,
+            Strategy(address(timelockExecutionStrategy), abi.encode(transactions)),
+            new bytes(0)
+        );
+        _vote(author, firstProposalId, Choice.For, userVotingStrategies, voteMetadataURI);
+
+        // Create a second proposal with the same transactions, and vote on it.
+        uint256 secondProposalId = _createProposal(
+            author,
+            proposalMetadataURI,
+            Strategy(address(timelockExecutionStrategy), abi.encode(duplicateTransactions)),
+            new bytes(0)
+        );
+        _vote(author, secondProposalId, Choice.For, userVotingStrategies, voteMetadataURI);
+
+        // Move forward in time.
+        vm.roll(block.number + space.maxVotingDuration());
+
+        // Queue the first one: it should work properly.
+        space.execute(firstProposalId, abi.encode(transactions));
+
+        // Ensure an error is thrown for the second proposal!
+        vm.expectRevert(DuplicateMetaTransaction.selector);
+        space.execute(secondProposalId, abi.encode(duplicateTransactions));
+    }
+
     function testQueueingRejectedProposal() external {
         MetaTransaction[] memory transactions = new MetaTransaction[](1);
         transactions[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 0);
@@ -177,33 +214,6 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest {
         space.execute(proposalId2, abi.encode(transactions));
     }
 
-    function testQueueingQueueDuplicateUniqueSalt() external {
-        MetaTransaction[] memory transactions = new MetaTransaction[](1);
-        transactions[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 0);
-        // Same transaction, but different salt
-        MetaTransaction[] memory transactions2 = new MetaTransaction[](1);
-        transactions2[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 1);
-        uint256 proposalId = _createProposal(
-            author,
-            proposalMetadataURI,
-            Strategy(address(timelockExecutionStrategy), abi.encode(transactions)),
-            new bytes(0)
-        );
-        uint256 proposalId2 = _createProposal(
-            author,
-            proposalMetadataURI,
-            Strategy(address(timelockExecutionStrategy), abi.encode(transactions2)),
-            new bytes(0)
-        );
-        _vote(author, proposalId, Choice.For, userVotingStrategies, voteMetadataURI);
-        _vote(author, proposalId2, Choice.For, userVotingStrategies, voteMetadataURI);
-        vm.roll(block.number + space.maxVotingDuration());
-
-        space.execute(proposalId, abi.encode(transactions));
-
-        space.execute(proposalId2, abi.encode(transactions2));
-    }
-
     function testQueueingInvalidPayload() external {
         MetaTransaction[] memory transactions = new MetaTransaction[](1);
         transactions[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 0);