Core Contracts

Summary

The TimeLockController is responsible for executing governance proposals that include calls to external accounts, which can be contracts.
Due to a missing check on contract existence this could result in silently failing calls where funds get lost.

Vulnerability Details

The executeBatch() function on the TimeLockController is used to execute governance proposals once they've been queued. The Governance contract takes care of passing the necessary calldata and call targets from the proposals to the time lock controller.

Here are the important lines of code that perform the batch execution:

The idea is simple, it iterates over all targets and performs a CALL on each of them with the corresponding calldata.
The problem with this, however, is that if the target is a non-existent address, then the call to it will silently fail.

This can happen when between the time of creating a proposal via Governance#propose() and Governance#execute() the target contract has been destructed. In this case, there'd be no code associated with the target address, but the call would still return success = true, making it look like the batch execution was successful, while in reality funds might have been lost.

Impact

If a value is provided to such a call where the receiver is a non-existing address, then those funds will be lost.

Tools Used

Manual review.

Recommendations

There should be a check on whether the target address actually has a code associated to it. This however only needs to happen when the provided calldata is not empty, because we still want to allow for standard ETH transfers to EOA accounts.

Ensuring the target address is a contract in can be done by checking its codesize. OpenZeppelin has great utilities for that:

Relevant links

• Governance#execute

• Governance#propose

• TimeLockController#executeBatch