Core Contracts

Summary

The state() function generates an id which is used to process and query queueing and execution of proposals in timelock. This is not necessarily unique:

The same is done in _queueProposal() and _executeProposal().

The expected execution flow by the protocol is that once proposal voting duration ends and proposal is successful, execute() needs to be called twice:

• In first call, execute() --> _queueProposal() results in a timelock of 2 days (as configured in tests)

• After 2 days, in the second call execute() --> _executeProposal() results in the proposal getting executed.

However, the queue timelock of 2 days can be bypassed. More importantly, someone else's proposal's second call to execute() can be permanently reverted by an attacker.

Description & Impact

First some context on proposals - Two separate proposals by Alice and Bob could have identical targets, values, calldatas, description and yet be different. For example they both could propose to call setValue(42) but inside the function setValue(), the output could be different based on the block.timestamp i.e. WHEN the proposal got executed and hence the call to the function made. The protocol allows this because a unique monotonically increasing proposalId is assigned to each proposal, even with identical parameters.

This is important because Alice could float a proposal on 21st March which if passed, gets executed 10 days later (as per test config) on 31st March and gives airdrop. Bob however floats his proposal on 23rd March and hence ideally this will get executed 10 days later on 2nd April i.e. the new financial year (FY from 1st April). Hence the airdrop would be taxable in the next year and will have to be shown in recipients' tax returns much later. Similar time-sensitive use cases may exist regarding WHEN an interest payment is to be credited or deposits to be stopped and 2-3 proposals may be floated one after another with a gap of few days to ensure staggerred execution.

Note: Even if one somehow disagrees with the above statements, the VARIATION-2 of the attack shown below still holds and does not require the above pre-condition explicitly.

A malicious (or even an honest) proposer can use this understanding and trust to hasten the timelines in two ways:

VARIATION-1: (coded as PoC-1)

Imagine the scenario:

1. Current config as per tests: voting ends 8 days after proposal is floated. Then, first call to execute() can be made and the proposal is queued. 2 days later, second call to execute() can be made and the proposal is executed. That is, it takes 10 days from start to finish.

2. Alice floats proposal with some targets, values, calldatas, description on 10-Feb. Her proposalId = 0. She would be eligible to call execute(0) the first time on 18-Feb and the second time on 20-Feb.

3. Bob the malicious user waits for 2 days & floats his proposal on 12-Feb with identical params targets, values, calldatas, description. His proposalId = 1. So far so good. He would be eligible to call execute(1) the first time on 20-Feb and the second time on 22-Feb.

4. Votes are cast and Alice's proposal is successful. She calls execute(0) on 18-Feb and proposal is queued in timelock.

5. Bob's proposal is successful too and he can call his first execute(1) no earlier than 20-Feb.

6. On 20-Feb, Alice prepares to call her second execute(0). Bob front-runs her and calls execute(1). Since the id generated by hashing targets, values, calldatas, description is the same for both the proposals, state() assumes Bob's proposal is already past the queue stage and pushes into execution stage. This is in spite of the fact that Bob called execute() with his proposalId = 1 ! (Note: There's a variation possible here. Alice could cancel her proposal just before 20-Feb and Bob would still be able to call execute(1) and bypass the queue time. This is because the timelock queue is not flushed of the id when a proposal is cancelled).

7. Bob's proposal gets executed immediately with no queue wait and is marked as ProposalState.Executed.

8. Alice's tx containing the execute(0) call also goes through after this but now reverts with error OperationAlreadyScheduled(). There`s no way for her now to get her proposal executed.

Note that the aforementioned scenario could happen inadvertently too, without Bob being malicious.

Here's a table showing this behaviour which is the output of the coded PoC-1 provided in the next section:

VARIATION-2: (coded as PoC-2)

1. Steps 1-4 same as above. Alice has created a proposal with proposalId = 0 and her proposal is queued in timelock after her call to execute(0) on 18-Feb.

2. Alice cancels her proposal on 19-Feb, before calling the second execute(0).

3. Bob feels the proposal carried value for the ecosystem and is disappointed that Alice cancelled it before finalization. Bob floats his proposal on 21-Feb with identical parameters. He would be eligible to call his first execute(1) 8 days later on 01-Mar. Bob's proposalId = 1.

4. Bob calls execute(1) on 01-Mar. Instead of queueing this in timelock for 2 days, it goes straight through for execution! This is because the id is same as Alice's proposal and the timelock has not flushed it out of the queue.

5. Bob's proposal just bypassed the mandatory queue time.

Proof of Concepts

PoC-1:

PoC-2:

Mitigation

While hashing and calculating the id in state() and other functions, simply add proposalId too to ensure uniqueness.
Additionally, internally call timelock controller's cancel() when Governance contract's cancel() is called. This results in delete _operations[id] getting executed and the queue is cleared.