Quick Review

v4-patch Governance upgrades

Proposed changes:

https://git.tornado.ws/AlienTornadosaurusHex/tornado-governance/src/branch/main/contracts/v4-patch/GovernancePatchUpgrade.sol

function execute(uint256 proposalId)

function _propose(
    address proposer,
    address target,
    string memory description
  )

Function override of prior version of https://github.com/tornadocash/tornado-governance/blob/master/contracts/v1/Governance.sol

The suggested implementation adds a check to ensure the code of a proposal can't change between the time it is proposed to the when it is actually executed.

L35-46:

if (proposalCodehash == proposalCodehashes[proposalId]) {
      super.execute(proposalId);
    } else {
      // So this should guarantee that the proposal should not be able to be executed in future
      // But also, the proposal will be skipped each time. So the question is whether this, a silent
      // skip, or a require can be used to stay "concise", and abandon the safety of having also this assign
      // Outside dependencies, which don't exist as of yet, might believe that the proposal code was executed
      proposal.executed = true;

      // Let the event signify it was metamorphic
      emit CodehashDifferent(target, proposalCodehashes[proposalId], proposalCodehash);
    }

I think the else branch could be omitted since a proposal that is not executed before proposal.endTime.add(EXECUTION_DELAY).add(EXECUTION_EXPIRATION) will be in expired state.

This way we don't have to consider any other side-effects here.

In absence of an actual "invalid" state any proposal that fails to execute no matter the reason will simply expire (currently configured to be 72 hours after endTime + delay).

So I'd prefer something like:

require(proposalCodehash == proposalCodehashes[proposalId],
    "Governance::propose: metamorphic proposal not allowed");
super.execute(proposalId);

Misc

• add new version() to return precise version of Governance