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.

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

Add GovernorTimelockControl address to TimelockController salt #4432

Merged

Conversation

ernestognw
Copy link
Member

The current implementation of GovernorTimelockControl schedules and executes batches within a TimelockController and uses the description hash as an operation salt. However, as a result of the Certora audit we identified that two governor instances can block each other if they're executing the same operation.

The solution proposed in this PR is to add the governor's address to the salt operation so that each governor will get different operation ids avoiding unexpected collisions.

Fixes LIB-834

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@ernestognw ernestognw requested a review from Amxx July 6, 2023 19:16
@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2023

🦋 Changeset detected

Latest commit: 9dd7f39

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ernestognw ernestognw requested review from frangio and Amxx and removed request for Amxx July 6, 2023 19:16
* same timelock.
*/
function _timelockSalt(bytes32 descriptionHash) private view returns (bytes32) {
return bytes20(address(this)) ^ descriptionHash;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to use an ^ instead of a keccak256 because it's cheaper and it doesn't matter if the preimage is known since the objective is to avoid unintentional collisions between governors.

If a governor instance wants to purposely block another, it'll require to mine the most significant 20 bytes of the descriptionHash which is infeasible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an interesting proposal. I'd have done a full hash, just because it would have been clearer to everyone what is going on ... but IMO that is also a valid option.

_timelock.scheduleBatch(targets, values, calldatas, 0, descriptionHash, delay);
bytes32 salt = _timelockSalt(descriptionHash);
_timelockIds[proposalId] = _timelock.hashOperationBatch(targets, values, calldatas, 0, salt);
_timelock.scheduleBatch(targets, values, calldatas, 0, salt, delay);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The decision to not have schedule and scheduleBatch return the id that they compute really was a bad one.

Amxx
Amxx previously approved these changes Jul 7, 2023
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I don't think its a serious issue that 2 governors that use the same TimelockController would submit the same proposal with the same description.

And if it ever happens ... then one could argue that they are the same proposal and should only be execute once.

In fact that might actually be a feature: you have multiple governor with different rule, you propose the same thing on both side, any governor can decide to execute it alone, but you have a guarantee that it won't be execute twice if both governor vote yes.

Anyway, If we want to do it, then I think this PR is good. I'm still not convinced we need this feature though ... but I also thing its not adding any security issue.

@ernestognw
Copy link
Member Author

I agree it's not a serious issue and I see the use case you mention @Amxx, but both the use case you propose and the issue we're solving with this PR is really low likelihood. I think you can workaround the need for a Governor executing alone with a different mechanism in the target contract, I don't think it should be Governor's responsibility to allow those settings.

I'm also not fully convinced about this change but it's kinda in a grey. For me, the reason to add it is that it doesn't add any security issue (apparently) and it's removing a (very) low likelihood issue. The net value added is small but not negative.

@Amxx
Copy link
Collaborator

Amxx commented Jul 12, 2023

(Meta / prioritization of tasks)

The net value added is small but not negative.

Well, if you consider the code change that is true. If you consider the time we spend on this, that could have been dedicated to other tasks, then one may argue that the outcome is negative.

@ernestognw
Copy link
Member Author

So far the minimal reproduction for the CI fails I've been able to get is with this command:

npm run test test/(access|governance)/**/*.js

Looks like there's something in access messing things up here.

@ernestognw
Copy link
Member Author

If you consider the time we spend on this, that could have been dedicated to other tasks, then one may argue that the outcome is negative.

This is something I've expressed before, I would not go into that rabbit hole rn.

frangio
frangio previously approved these changes Jul 12, 2023
Co-authored-by: Francisco <fg@frang.io>
This was referenced Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants