Consider adding a contract existence check in TimelockController's _execute function

[Jaime-Iglesias]

I noticed that the _execute function in the TimeLockController contract does not have a contract existence check, which, in the case that data is not empty and target has no code will cause the proposal to be deemed "executed successfully" even though no side-effects have been triggered.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3d7a93876a2e5e1d7fe29b5a0e96e222afdc4cfa/contracts/governance/TimelockController.sol#L350

I think the contract existence check should be made by the function when data is not empty to prevent cases in which a mistake during the proposal creation or execution goes unnoticed.

This also affects the upgradeable version:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/25aabd286e002a1526c345c8db259d57bdf0ad28/contracts/governance/TimelockControllerUpgradeable.sol#L360

One the other hand I think one could also make the argument that if an operation reaches the state of "execution" and no mistakes have been noticed there is probably something wrong elsewhere in the system (probably in a process); however, this fact (i.e that proposals will be deemed "successfully executed" even when no side-effects have been triggered) is not documented anywhere AFAIK so I think at the very least a documentation effort should be made.

Finally, as a side note I think this import is unused - I believe it was added here https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3d7a93876a2e5e1d7fe29b5a0e96e222afdc4cfa/contracts/governance/TimelockController.sol#L9

[Amxx]

Hello @Jaime-Iglesias

In case the timelock hold some ETH, and you want that ETH sent to an EOA, you need to do a call with value (data is irrelevant in that case) to an address that has no code.

Preventing such calls would break some legitimate usecases.

[Jaime-Iglesias]

Hello @Jaime-Iglesias

In case the timelock hold some ETH, and you want that ETH sent to an EOA, you need to do a call with value (data is irrelevant in that case) to an address that has no code.

Preventing such calls would break some legitimate usecases.

Wouldn't making the check only when data.length > 0 still allow for this functionality? Unless you are making the argument that you might want to send data along with ETH to an EOA.

[tupokraju]

Hello!

Yes, you are correct that making the check only when data.length > 0 would still allow for sending ETH to an EOA. This would not break any legitimate use cases and would prevent cases where a proposal is deemed to have been "executed successfully" even though no side-effects were triggered.

@Jaime-Iglesias As for the import statement you mentioned, I checked and I have not found any use case for it in the code, so it is safe to assume that it can be removed safely.

Overall, I think adding a contract existence check in the _execute function when data is not empty is a good idea. It would help prevent mistakes from going unnoticed and would improve the overall reliability of the system.

[Jaime-Iglesias]

What do you think @Amxx ?

[Amxx]

It all comes down to this one thing: is it ok / legitimate to send data along during an ETH transfer to an EOA.

The core protocol somehow says yes. You can send a native TX from EOA to EOA with ETH and/or data.

This has usecases that are clear, and getting the corresponding data is way easier than getting the equivalent data when dealing with a subcall by a contract.

Does that mean such contracts call should be prevented from passing data? IMO passing such data should be a thing, because it's not breaking and it might actually be useful one day... but I get the point that it can be confusing.

[Jaime-Iglesias]

It all comes down to this one thing: is it ok / legitimate to send data along during an ETH transfer to an EOA.

The core protocol somehow says yes. You can send a native TX from EOA to EOA with ETH and/or data.

This has usecases that are clear, and getting the corresponding data is way easier than getting the equivalent data when dealing with a subcall by a contract.

Does that mean such contracts call should be prevented from passing data? IMO passing such data should be a thing, because it's not breaking and it might actually be useful one day... but I get the point that it can be confusing.

I think people being able to send data along with ETH to an EOA is a fair argument regardless of how much demand there might be for that; however I think there is certainly an edge case here w.r.t proposals that target non-existent contracts being deemed "successfully executed", perhaps a documentation effort is enough to make users aware of this; however I would rather have a programmatic solution if possible.

Perhaps adding a flag to the schedule and/or execute functions that says wether target is a supposed to be a contract could be a solution?

1. If flag is set to true -> target is expected to be a contract -> check contract existence on operation execution.
2. if flag is set to false -> target not expected to be a contract -> do not check contract existence on operation execution.

[frangio]

Adding a flag is complexity that we should try to avoid.

I agree with @Amxx's points, I think sending ETH to an EOA is a legitimate operation that we shouldn't make more difficult. The reality is that at the EVM level making a CALL to a non-existent contract is a successful execution, and there is nothing specific about TimelockController's execute that could justify having different semantics for calls...

I can see the point: across the library we try to help prevent mistakes, and in many places we have additional on-chain checks that don't strictly need to be on-chain, but I don't think any of those checks get in the way of legitimate use cases.

In practice, timelocks, governance, and other forms of indirect execution should be validated in some off-chain way. This is something we are already seeing, for example running a Tenderly simulation like Gnosis Safe offers, and similar things are done for governance proposals.

[Jaime-Iglesias]

I think the arguments are very much valid and I agree with the idea of adding a flag being extra overhead, it was not meant as an actual solution more like an idea hahaha.

Perhaps a middle ground would be a documentation effort w.r.t assumptions about proposal's correctness and edge-cases such as the one I'm trying to point to - would that be something you guys feel like it would add value here?

[frangio]

We want to have more documentation answering the question "what could go wrong, and what to do about it" for each contract. We're not sure what is the best place for that kind of content, and we may need to make a new place for it in the docs site. I think this could fit in that category, but IMO in a more general form like "your proposal parameters could be wrong and you might only find out after execution".