audit: fix silent metatx merge in comp timelock (#212)

* audit: fix silent metatx merge in comp timelock * chore: use uint256 insetad of uint --------- Co-authored-by: Orland0x <[email protected]>
snapshot-labs · Jun 20, 2023 · 6db831d · 6db831d
1 parent 4027118
commit 6db831d
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 5 deletions.
diff --git a/.forge-snapshots/ProposeSigComp.snap b/.forge-snapshots/ProposeSigComp.snap
@@ -1 +1 @@
-148841
+148980
diff --git a/.forge-snapshots/VoteSigComp.snap b/.forge-snapshots/VoteSigComp.snap
@@ -1 +1 @@
-50322
+50452
diff --git a/.forge-snapshots/VoteSigCompMetadata.snap b/.forge-snapshots/VoteSigCompMetadata.snap
@@ -1 +1 @@
-51865
+51995
diff --git a/.forge-snapshots/VoteTxComp.snap b/.forge-snapshots/VoteTxComp.snap
@@ -1 +1 @@
-43892
+44022
diff --git a/.forge-snapshots/VoteTxCompMetadata.snap b/.forge-snapshots/VoteTxCompMetadata.snap
@@ -1 +1 @@
-45476
+45606
diff --git a/src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol b/src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol
@@ -17,6 +17,8 @@ contract CompTimelockCompatibleExecutionStrategy is SimpleQuorumExecutionStrateg
     error ProposalNotQueued();
     /// @notice Thrown if the proposal execution payload hash is already queued.
     error DuplicateExecutionPayloadHash();
+    /// @notice Thrown if the same MetaTransaction appears twice in the same payload (salt is not taken into account).
+    error DuplicateMetaTransaction();
     /// @notice Thrown if veto caller is not the veto guardian.
     error OnlyVetoGuardian();
     /// @notice Thrown if the transaction is invalid.
@@ -103,11 +105,26 @@ contract CompTimelockCompatibleExecutionStrategy is SimpleQuorumExecutionStrateg
         proposalExecutionTime[proposal.executionPayloadHash] = executionTime;
 
         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;
+
             timelock.queueTransaction(
                 transactions[i].to,
                 transactions[i].value,

diff --git a/test/CompTimelockExecutionStrategy.t.sol b/test/CompTimelockExecutionStrategy.t.sol
@@ -28,6 +28,7 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest {
     error TimelockDelayNotMet();
     error ProposalNotQueued();
     error DuplicateExecutionPayloadHash();
+    error DuplicateMetaTransaction();
     error OnlyVetoGuardian();
     error InvalidTransaction();
     event CompTimelockCompatibleExecutionStrategySetUp(
@@ -97,6 +98,24 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest {
         space.execute(proposalId, abi.encode(transactions));
     }
 
+    function testQueueingDuplicateMetaTransaction() external {
+        MetaTransaction[] memory transactions = new MetaTransaction[](2);
+        transactions[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 0);
+        // Salt differs but content does not
+        transactions[1] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 1);
+        uint256 proposalId = _createProposal(
+            author,
+            proposalMetadataURI,
+            Strategy(address(timelockExecutionStrategy), abi.encode(transactions)),
+            new bytes(0)
+        );
+        _vote(author, proposalId, Choice.For, userVotingStrategies, voteMetadataURI);
+        vm.roll(block.number + space.maxVotingDuration());
+
+        vm.expectRevert(DuplicateMetaTransaction.selector);
+        space.execute(proposalId, abi.encode(transactions));
+    }
+
     function testQueueingRejectedProposal() external {
         MetaTransaction[] memory transactions = new MetaTransaction[](1);
         transactions[0] = MetaTransaction(recipient, 1, "", Enum.Operation.Call, 0);