Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

audit: Comp Timelock Doesn't Support Duplicate MetaTransaction Queueing #227

Merged
merged 2 commits into from
Jun 27, 2023
Merged

audit: Comp Timelock Doesn't Support Duplicate MetaTransaction Queueing #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65d9412
refactor: copy CompTimelock's behaviour in CompTimelockCompatibleExec…
pscott Jun 27, 2023
2cf8e8b
Merge branch 'main' of github.com:snapshot-labs/snapshot-oc into fix_…
pscott Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
64 changes: 37 additions & 27 deletions test/CompTimelockExecutionStrategy.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down