TimelockController 'salt' is unrecoverable yet required for execution

[0x0scion]

When scheduling a transaction using the TimelockController, one is required to provide a salt to distinguish otherwise identical transactions. The salt parameter is not emitted in the CallScheduled event which makes it impossible to recover. Yet salt is required for executing the scheduled transaction transaction, ~~even though it isn't necessary and more gas efficient if id is used instead~~.

💻 Environment

Openzeppelin v4.5.0 / hardhat

📝 Details

salt is missing from the CallScheduled event: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/458697be32f41946e1ab66e946c7cf373c79577d/contracts/governance/TimelockController.sol#L35

salt is required for the execute & executeBatch calls: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/458697be32f41946e1ab66e946c7cf373c79577d/contracts/governance/TimelockController.sol#L267

~~Instead of salt the execute methods should be consuming the hashed operation (id) directly, since that id is readily available via event data. This would also save on gas since there is no need to re-run the hashing operation.~~

Happy to submit a PR

[Amxx]

Hello @0x0scion and thanks for raising this issue

Instead of salt the execute methods should be consuming the hashed operation (id) directly, since that id is readily available via event data. This would also save on gas since there is no need to re-run the hashing operation.

If we don't run the hashing operation, how do we check that the other parameters (target, value, data) match the id that was scheduled? If we don't check that, you could possibly use any scheduled operation's id with unrelated params, and execute another operation.

It's true that the salt not being present in the event is disturbing. I wonder how we missed that / if we had a reason for not including it.

[0x0scion]

If we don't run the hashing operation, how do we check that the other parameters (target, value, data) match the id that was scheduled? If we don't check that, you could possibly use any scheduled operation's id with unrelated params, and execute another operation.

Ah, yes, did not think this through all the way - striking out that suggestion above. So just a matter of salt not being in the event.

[ashwinYardi]

If we don't run the hashing operation, how do we check that the other parameters (target, value, data) match the id that was scheduled? If we don't check that, you could possibly use any scheduled operation's id with unrelated params, and execute another operation.

@Amxx @0x0scion Hey guys! I was wondering, doesn't this logic also apply to cancel method? cancel method, at the moment, only accepts id as a param. So, its totally possible that, proposer can possibly cancel any other scheduled operation with unrelated params.

[Amxx]

@ashwinYardi Cancel doesn't have other params. It just cancels the proposal that has a given ID.

We could have required the cancel call to send all the details, hash them, and cancel that, but it would have been more expensive, and not safer.

In short

• execute: we are actually executing, so we need the execution details and we have to validate them;
• cancel: we are just internally canceling, we don't need the execution details.

[frangio]

So we need to emit the salt in an event, but we can't change the existing event because it changes the event selector, which is a little concerning because any monitoring infrastructure in place will not see the new event.

We can emit the salt in a new event ProposalSalt(uint256 indexed proposalId, bytes32 salt).

[frangio]

I would assume it's common to use salt=0. Feels like we could emit the event only when it isn't 0.