From 68f5be64e255e016c1a8a3180c8bcf93640b1bc2 Mon Sep 17 00:00:00 2001
From: AlienTornadosaurusHex <>
Date: Mon, 22 May 2023 20:25:59 +0000
Subject: [PATCH] Implement suggestion:
 https://hackmd.io/@mLAPku2WQZmso5oX4kzPOg/Bkn8SBKSn

Signed-off-by: AlienTornadosaurusHex <>
---
 contracts/v4-patch/GovernancePatchUpgrade.sol | 15 ++-----------
 package.json                                  |  3 ++-
 test/patch/patch.test.js                      | 21 +++----------------
 3 files changed, 7 insertions(+), 32 deletions(-)

diff --git a/contracts/v4-patch/GovernancePatchUpgrade.sol b/contracts/v4-patch/GovernancePatchUpgrade.sol
index 3799212..36907cc 100644
--- a/contracts/v4-patch/GovernancePatchUpgrade.sol
+++ b/contracts/v4-patch/GovernancePatchUpgrade.sol
@@ -9,8 +9,6 @@ import "../v3-relayer-registry/GovernanceStakingUpgrade.sol";
 contract GovernancePatchUpgrade is GovernanceStakingUpgrade {
   mapping(uint256 => bytes32) public proposalCodehashes;
 
-  event CodehashDifferent(address target, bytes32 oldHash, bytes32 newHash);
-
   // The stakingRewardsAddress sets the immutable to the new staking contract
   constructor(
     address stakingRewardsAddress,
@@ -32,18 +30,9 @@ contract GovernancePatchUpgrade is GovernanceStakingUpgrade {
       proposalCodehash := extcodehash(target)
     }
 
-    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;
+    require(proposalCodehash == proposalCodehashes[proposalId], "Governance::propose: metamorphic contracts not allowed");
 
-      // Let the event signify it was metamorphic
-      emit CodehashDifferent(target, proposalCodehashes[proposalId], proposalCodehash);
-    }
+    super.execute(proposalId);
   }
 
   // This should store the proposal extcodehash
diff --git a/package.json b/package.json
index 1fa8524..d1fa8e2 100644
--- a/package.json
+++ b/package.json
@@ -17,7 +17,8 @@
     "test:f": "yarn prettier:fix && yarn test",
     "clean": "yarn prettier:fix && yarn lint",
     "compile": "yarn prettier:fix && yarn hardhat compile",
-    "coverage": "yarn hardhat coverage"
+    "coverage": "yarn hardhat coverage",
+    "layout": "yarn hardhat check"
   },
   "author": "Tornado.cash team <hello@tornado.cash>",
   "license": "MIT",
diff --git a/test/patch/patch.test.js b/test/patch/patch.test.js
index 382e000..a0f5383 100644
--- a/test/patch/patch.test.js
+++ b/test/patch/patch.test.js
@@ -264,25 +264,10 @@ describe('Gov Exploit Patch Upgrade Tests', () => {
       // Load the contract
       const maliciousProposal = await ethers.getContractAt('MaliciousProposal', maliciousProposalAddress)
 
-      // Check that the malicious proposer is the deployer
-      const deployer = await maliciousProposal.deployer()
-
-      // Get bal before
-      const deployerBalanceBefore = await torn.balanceOf(deployer)
-      const governanceBalanceBefore = await torn.balanceOf(governance.address)
-
-      expect(governanceBalanceBefore).to.be.gt(zero)
-
       // Now execute
-      await governance.execute(proposalId)
-
-      // Check bal after
-      const deployerBalanceAfter = await torn.balanceOf(deployer)
-      const governanceBalanceAfter = await torn.balanceOf(governance.address)
-
-      // Protected
-      expect(deployerBalanceAfter).to.equal(deployerBalanceBefore)
-      expect(governanceBalanceAfter).to.equal(governanceBalanceBefore)
+      await expect(governance.execute(proposalId)).to.be.revertedWith(
+        'Governance::propose: metamorphic contracts not allowed',
+      )
 
       // Terminate the contract for the next test
       await maliciousProposal.emergencyStop()