diff --git a/packages/celotool/src/cmds/deploy/upgrade/hotfix.ts b/packages/celotool/src/cmds/deploy/upgrade/hotfix.ts index c36675f0d8f..b23e433e26a 100644 --- a/packages/celotool/src/cmds/deploy/upgrade/hotfix.ts +++ b/packages/celotool/src/cmds/deploy/upgrade/hotfix.ts @@ -2,12 +2,13 @@ import { newKit } from '@celo/contractkit' import { + hotfixToHash, ProposalBuilder, - proposalToHash, proposalToJSON, } from '@celo/contractkit/lib/governance/proposals' import { privateKeyToAddress } from '@celo/utils/lib/address' import { concurrentMap } from '@celo/utils/lib/async' +import { randomBytes } from 'crypto' import { getFornoUrl } from 'src/lib/endpoints' import { envVar, fetchEnv } from 'src/lib/env-utils' import { AccountType, getPrivateKeysFor } from 'src/lib/generate_utils' @@ -78,7 +79,9 @@ export const handler = async (argv: EthstatsArgv) => { console.error('\nPlease see examples in hotfix.ts and add transactions') process.exit(1) } - const proposalHash = proposalToHash(kit, proposal) + + const salt = randomBytes(32) + const proposalHash = hotfixToHash(kit, proposal, salt) // If your proposal is just made of Celo Registry contract methods, you can print it out console.info('Proposal: ', await proposalToJSON(kit, proposal)) @@ -119,7 +122,7 @@ export const handler = async (argv: EthstatsArgv) => { throw new Error() } console.info('\nExecute the hotfix') - await governance.executeHotfix(proposal).sendAndWaitForReceipt({ from: addresses[0] }) + await governance.executeHotfix(proposal, salt).sendAndWaitForReceipt({ from: addresses[0] }) hotfixRecord = await governance.getHotfixRecord(proposalHash) console.info('\nHotfix Record: ', hotfixRecord) diff --git a/packages/cli/src/commands/governance/executehotfix.ts b/packages/cli/src/commands/governance/executehotfix.ts index 0c9902160d4..0bdd1d884b4 100644 --- a/packages/cli/src/commands/governance/executehotfix.ts +++ b/packages/cli/src/commands/governance/executehotfix.ts @@ -11,6 +11,7 @@ export default class ExecuteHotfix extends BaseCommand { ...BaseCommand.flags, from: Flags.address({ required: true, description: "Executors's address" }), jsonTransactions: flags.string({ required: true, description: 'Path to json transactions' }), + salt: flags.string({ required: true, description: 'Secret salt associated with hotfix' }), } static examples = [] @@ -20,7 +21,9 @@ export default class ExecuteHotfix extends BaseCommand { const governance = await this.kit.contracts.getGovernance() const hotfix = await buildProposalFromJsonFile(this.kit, res.flags.jsonTransactions) - const tx = governance.executeHotfix(hotfix) + + const saltBuff = Buffer.from(res.flags.salt, 'hex') + const tx = governance.executeHotfix(hotfix, saltBuff) await displaySendTx('executeHotfixTx', tx, { from: res.flags.from }) } } diff --git a/packages/contractkit/src/governance/proposals.ts b/packages/contractkit/src/governance/proposals.ts index b4eab5368ba..5682c119cd8 100644 --- a/packages/contractkit/src/governance/proposals.ts +++ b/packages/contractkit/src/governance/proposals.ts @@ -7,19 +7,18 @@ import { obtainKitContractDetails } from '../explorer/base' import { BlockExplorer } from '../explorer/block-explorer' import { ABI as GovernanceABI } from '../generated/Governance' import { ContractKit } from '../kit' +import { getAbiTypes } from '../utils/web3-utils' import { CeloTransactionObject, valueToString } from '../wrappers/BaseWrapper' -import { GovernanceWrapper, Proposal, ProposalTransaction } from '../wrappers/Governance' +import { hotfixToParams, Proposal, ProposalTransaction } from '../wrappers/Governance' import { setImplementationOnProxy } from './proxy' -export const PROPOSE_PARAM_ABI_TYPES = (GovernanceABI.find( - (abiEntry) => abiEntry.name! === 'propose' -)!.inputs! as Array<{ type: string }>).map((abiInput) => abiInput.type) +export const HOTFIX_PARAM_ABI_TYPES = getAbiTypes(GovernanceABI as any, 'executeHotfix') -export const proposalToEncodedParams = (kit: ContractKit, proposal: Proposal) => - kit.web3.eth.abi.encodeParameters(PROPOSE_PARAM_ABI_TYPES, GovernanceWrapper.toParams(proposal)) +export const hotfixToEncodedParams = (kit: ContractKit, proposal: Proposal, salt: Buffer) => + kit.web3.eth.abi.encodeParameters(HOTFIX_PARAM_ABI_TYPES, hotfixToParams(proposal, salt)) -export const proposalToHash = (kit: ContractKit, proposal: Proposal) => - keccak256(proposalToEncodedParams(kit, proposal)) as Buffer +export const hotfixToHash = (kit: ContractKit, proposal: Proposal, salt: Buffer) => + keccak256(hotfixToEncodedParams(kit, proposal, salt)) as Buffer export interface ProposalTransactionJSON { contract: CeloContract diff --git a/packages/contractkit/src/utils/web3-utils.ts b/packages/contractkit/src/utils/web3-utils.ts index ad50eeb0886..a79b83c3e00 100644 --- a/packages/contractkit/src/utils/web3-utils.ts +++ b/packages/contractkit/src/utils/web3-utils.ts @@ -1,5 +1,6 @@ import debugFactory from 'debug' import Web3 from 'web3' +import { ABIDefinition } from 'web3/eth/abi' import { CeloProvider } from '../providers/celo-provider' const debug = debugFactory('kit:web3:utils') @@ -18,3 +19,6 @@ export function addLocalAccount(web3: Web3, privateKey: string): Web3 { debug('Providers configured') return web3 } + +export const getAbiTypes = (abi: ABIDefinition[], methodName: string) => + abi.find((entry) => entry.name! === methodName)!.inputs!.map((input) => input.type) diff --git a/packages/contractkit/src/wrappers/Governance.ts b/packages/contractkit/src/wrappers/Governance.ts index c40fe469df9..bad3f837bda 100644 --- a/packages/contractkit/src/wrappers/Governance.ts +++ b/packages/contractkit/src/wrappers/Governance.ts @@ -54,6 +54,16 @@ export type ProposalParams = Parameters export type ProposalTransaction = Pick export type Proposal = ProposalTransaction[] +export const proposalToParams = (proposal: Proposal): ProposalParams => { + const data = proposal.map((tx) => stringToBuffer(tx.input)) + return [ + proposal.map((tx) => tx.value), + proposal.map((tx) => tx.to), + bufferToBytes(Buffer.concat(data)), + data.map((inp) => inp.length), + ] +} + export interface ProposalRecord { stage: ProposalStage metadata: ProposalMetadata @@ -80,8 +90,13 @@ export interface Votes { [VoteValue.Abstain]: BigNumber } +export type HotfixParams = Parameters +export const hotfixToParams = (proposal: Proposal, salt: Buffer): HotfixParams => { + const p = proposalToParams(proposal) + return [p[0], p[1], p[2], p[3], bufferToString(salt)] +} + export interface HotfixRecord { - hash: Buffer approved: boolean executed: boolean preparedEpoch: BigNumber @@ -183,16 +198,6 @@ export class GovernanceWrapper extends BaseWrapper { }) ) - static toParams = (proposal: Proposal): ProposalParams => { - const data = proposal.map((tx) => stringToBuffer(tx.input)) - return [ - proposal.map((tx) => tx.value), - proposal.map((tx) => tx.to), - bufferToBytes(Buffer.concat(data)), - data.map((inp) => inp.length), - ] - } - /** * Returns whether a given proposal is approved. * @param proposalID Governance proposal UUID @@ -259,7 +264,7 @@ export class GovernanceWrapper extends BaseWrapper { * Submits a new governance proposal. * @param proposal Governance proposal */ - propose = proxySend(this.kit, this.contract.methods.propose, GovernanceWrapper.toParams) + propose = proxySend(this.kit, this.contract.methods.propose, proposalToParams) /** * Returns whether a governance proposal exists with the given ID. @@ -509,7 +514,6 @@ export class GovernanceWrapper extends BaseWrapper { async getHotfixRecord(hash: Buffer): Promise { const res = await this.contract.methods.getHotfixRecord(bufferToString(hash)).call() return { - hash, approved: res[0], executed: res[1], preparedEpoch: valueToBigNumber(res[2]), @@ -575,11 +579,8 @@ export class GovernanceWrapper extends BaseWrapper { /** * Executes a given sequence of transactions if the corresponding hash is prepared and approved. * @param hotfix Governance hotfix proposal + * @param salt Secret which guarantees uniqueness of hash * @notice keccak256 hash of abi encoded transactions computed on-chain */ - executeHotfix = proxySend( - this.kit, - this.contract.methods.executeHotfix, - GovernanceWrapper.toParams - ) + executeHotfix = proxySend(this.kit, this.contract.methods.executeHotfix, hotfixToParams) } diff --git a/packages/docs/command-line-interface/governance.md b/packages/docs/command-line-interface/governance.md index 24311162e4f..5bbd20be4b2 100644 --- a/packages/docs/command-line-interface/governance.md +++ b/packages/docs/command-line-interface/governance.md @@ -60,6 +60,7 @@ USAGE OPTIONS --from=0xc1912fEE45d61C87Cc5EA59DaE31190FFFFf232d (required) Executors's address --jsonTransactions=jsonTransactions (required) Path to json transactions + --salt=salt (required) Secret salt associated with hotfix ``` _See code: [packages/cli/src/commands/governance/executehotfix.ts](https://github.com/celo-org/celo-monorepo/tree/master/packages/cli/src/commands/governance/executehotfix.ts)_ diff --git a/packages/protocol/contracts/governance/Governance.sol b/packages/protocol/contracts/governance/Governance.sol index 9cc29e257d7..c8807db9d95 100644 --- a/packages/protocol/contracts/governance/Governance.sol +++ b/packages/protocol/contracts/governance/Governance.sol @@ -655,6 +655,7 @@ contract Governance is * @param hash The abi encoded keccak256 hash of the hotfix transaction(s) to be whitelisted. */ function approveHotfix(bytes32 hash) external { + require(!hotfixes[hash].executed, "hotfix already executed"); require(msg.sender == approver, "Not approver"); hotfixes[hash].approved = true; emit HotfixApproved(hash); @@ -674,6 +675,7 @@ contract Governance is * @param hash The abi encoded keccak256 hash of the hotfix transaction(s) to be whitelisted. */ function whitelistHotfix(bytes32 hash) external { + require(!hotfixes[hash].executed, "hotfix already executed"); hotfixes[hash].whitelisted[msg.sender] = true; emit HotfixWhitelisted(hash, msg.sender); } @@ -683,6 +685,7 @@ contract Governance is * @param hash The hash of the hotfix to be prepared. */ function prepareHotfix(bytes32 hash) external { + require(!hotfixes[hash].executed, "hotfix already executed"); require(isHotfixPassing(hash), "hotfix not whitelisted by 2f+1 validators"); uint256 epoch = getEpochNumber(); require(hotfixes[hash].preparedEpoch < epoch, "hotfix already prepared for this epoch"); @@ -696,15 +699,17 @@ contract Governance is * @param destinations The destination addresses of the proposed transactions. * @param data The concatenated data to be included in the proposed transactions. * @param dataLengths The lengths of each transaction's data. + * @param salt Secret associated with hotfix which guarantees uniqueness of hash. * @dev Reverts if hotfix is already executed, not approved, or not prepared for current epoch. */ function executeHotfix( uint256[] calldata values, address[] calldata destinations, bytes calldata data, - uint256[] calldata dataLengths + uint256[] calldata dataLengths, + bytes32 salt ) external { - bytes32 hash = keccak256(abi.encode(values, destinations, data, dataLengths)); + bytes32 hash = keccak256(abi.encode(values, destinations, data, dataLengths, salt)); (bool approved, bool executed, uint256 preparedEpoch) = getHotfixRecord(hash); require(!executed, "hotfix already executed"); diff --git a/packages/protocol/test/governance/governance.ts b/packages/protocol/test/governance/governance.ts index 890deb915a4..783e7e71123 100644 --- a/packages/protocol/test/governance/governance.ts +++ b/packages/protocol/test/governance/governance.ts @@ -1,3 +1,16 @@ +import { CeloContractName } from '@celo/protocol/lib/registry-utils' +import { + assertBalance, + assertEqualBN, + assertLogMatches2, + assertRevert, + matchAny, + mineBlocks, + NULL_ADDRESS, + stripHexEncoding, + timeTravel, +} from '@celo/protocol/lib/test-utils' +import { fixed1, multiply, toFixed } from '@celo/utils/lib/fixidity' import BigNumber from 'bignumber.js' import { keccak256 } from 'ethereumjs-util' import { @@ -13,20 +26,6 @@ import { TestTransactionsInstance, } from 'types' -import { CeloContractName } from '@celo/protocol/lib/registry-utils' -import { - assertBalance, - assertEqualBN, - assertLogMatches2, - assertRevert, - matchAny, - mineBlocks, - NULL_ADDRESS, - stripHexEncoding, - timeTravel, -} from '@celo/protocol/lib/test-utils' -import { fixed1, multiply, toFixed } from '@celo/utils/lib/fixidity' - const Governance: GovernanceTestContract = artifacts.require('GovernanceTest') const Accounts: AccountsContract = artifacts.require('Accounts') const MockLockedGold: MockLockedGoldContract = artifacts.require('MockLockedGold') @@ -103,8 +102,9 @@ contract('Governance', (accounts: string[]) => { let transactionSuccess1: Transaction let transactionSuccess2: Transaction let transactionFail: Transaction - let proposalHash: Buffer - let proposalHashStr: string + let salt: string + let hotfixHash: Buffer + let hotfixHashStr: string beforeEach(async () => { accountsInstance = await Accounts.new() governance = await Governance.new() @@ -164,18 +164,20 @@ contract('Governance', (accounts: string[]) => { 'hex' ), } - proposalHash = keccak256( + salt = '0x657ed9d64e84fa3d1af43b3a307db22aba2d90a158015df1c588c02e24ca08f0' + hotfixHash = keccak256( web3.eth.abi.encodeParameters( - ['uint256[]', 'address[]', 'bytes', 'uint256[]'], + ['uint256[]', 'address[]', 'bytes', 'uint256[]', 'bytes32'], [ [String(transactionSuccess1.value)], [transactionSuccess1.destination.toString()], transactionSuccess1.data, [String(transactionSuccess1.data.length)], + salt, ] ) ) as Buffer - proposalHashStr = '0x' + proposalHash.toString('hex') + hotfixHashStr = '0x' + hotfixHash.toString('hex') }) describe('#initialize()', () => { @@ -1940,13 +1942,13 @@ contract('Governance', (accounts: string[]) => { describe('#approveHotfix()', () => { it('should mark the hotfix record approved when called by approver', async () => { - await governance.approveHotfix(proposalHashStr, { from: approver }) - const [approved, ,] = await governance.getHotfixRecord.call(proposalHashStr) + await governance.approveHotfix(hotfixHashStr, { from: approver }) + const [approved, ,] = await governance.getHotfixRecord.call(hotfixHashStr) assert.isTrue(approved) }) it('should emit the HotfixApproved event', async () => { - const resp = await governance.approveHotfix(proposalHashStr, { from: approver }) + const resp = await governance.approveHotfix(hotfixHashStr, { from: approver }) assert.equal(resp.logs.length, 1) const log = resp.logs[0] assertLogMatches2(log, { @@ -1955,11 +1957,11 @@ contract('Governance', (accounts: string[]) => { hash: matchAny, }, }) - assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(proposalHash)) + assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(hotfixHash)) }) it('should revert when called by non-approver', async () => { - await assertRevert(governance.approveHotfix(proposalHashStr, { from: accounts[2] })) + await assertRevert(governance.approveHotfix(hotfixHashStr, { from: accounts[2] })) }) }) @@ -1971,7 +1973,7 @@ contract('Governance', (accounts: string[]) => { }) it('should emit the HotfixWhitelist event', async () => { - const resp = await governance.whitelistHotfix(proposalHashStr, { from: accounts[3] }) + const resp = await governance.whitelistHotfix(hotfixHashStr, { from: accounts[3] }) assert.equal(resp.logs.length, 1) const log = resp.logs[0] assertLogMatches2(log, { @@ -1981,7 +1983,7 @@ contract('Governance', (accounts: string[]) => { whitelister: accounts[3], }, }) - assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(proposalHash)) + assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(hotfixHash)) }) }) @@ -1994,20 +1996,20 @@ contract('Governance', (accounts: string[]) => { }) it('should return false when hotfix has not been whitelisted', async () => { - const passing = await governance.isHotfixPassing.call(proposalHashStr) + const passing = await governance.isHotfixPassing.call(hotfixHashStr) assert.isFalse(passing) }) it('should return false when hotfix has been whitelisted but not by quorum', async () => { - await governance.whitelistHotfix(proposalHashStr, { from: accounts[2] }) - const passing = await governance.isHotfixPassing.call(proposalHashStr) + await governance.whitelistHotfix(hotfixHashStr, { from: accounts[2] }) + const passing = await governance.isHotfixPassing.call(hotfixHashStr) assert.isFalse(passing) }) it('should return true when hotfix is whitelisted by quorum', async () => { - await governance.whitelistHotfix(proposalHashStr, { from: accounts[2] }) - await governance.whitelistHotfix(proposalHashStr, { from: accounts[3] }) - const passing = await governance.isHotfixPassing.call(proposalHashStr) + await governance.whitelistHotfix(hotfixHashStr, { from: accounts[2] }) + await governance.whitelistHotfix(hotfixHashStr, { from: accounts[3] }) + const passing = await governance.isHotfixPassing.call(hotfixHashStr) assert.isTrue(passing) }) }) @@ -2019,24 +2021,24 @@ contract('Governance', (accounts: string[]) => { }) it('should revert when hotfix is not passing', async () => { - await assertRevert(governance.prepareHotfix(proposalHashStr)) + await assertRevert(governance.prepareHotfix(hotfixHashStr)) }) describe('when hotfix is passing', () => { beforeEach(async () => { await mineBlocks(EPOCH, web3) - await governance.whitelistHotfix(proposalHashStr, { from: accounts[2] }) + await governance.whitelistHotfix(hotfixHashStr, { from: accounts[2] }) }) it('should mark the hotfix record prepared epoch', async () => { - await governance.prepareHotfix(proposalHashStr) - const [, , preparedEpoch] = await governance.getHotfixRecord.call(proposalHashStr) + await governance.prepareHotfix(hotfixHashStr) + const [, , preparedEpoch] = await governance.getHotfixRecord.call(hotfixHashStr) const currEpoch = new BigNumber(await governance.getEpochNumber()) assertEqualBN(preparedEpoch, currEpoch) }) it('should emit the HotfixPrepared event', async () => { - const resp = await governance.prepareHotfix(proposalHashStr) + const resp = await governance.prepareHotfix(hotfixHashStr) const currEpoch = new BigNumber(await governance.getEpochNumber()) assert.equal(resp.logs.length, 1) const log = resp.logs[0] @@ -2047,18 +2049,18 @@ contract('Governance', (accounts: string[]) => { epoch: currEpoch, }, }) - assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(proposalHash)) + assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(hotfixHash)) }) it('should revert when epoch == preparedEpoch', async () => { - await governance.prepareHotfix(proposalHashStr) - await assertRevert(governance.prepareHotfix(proposalHashStr)) + await governance.prepareHotfix(hotfixHashStr) + await assertRevert(governance.prepareHotfix(hotfixHashStr)) }) it('should succeed for epoch != preparedEpoch', async () => { - await governance.prepareHotfix(proposalHashStr) + await governance.prepareHotfix(hotfixHashStr) await mineBlocks(EPOCH, web3) - await governance.prepareHotfix(proposalHashStr) + await governance.prepareHotfix(hotfixHashStr) }) }) }) @@ -2070,7 +2072,8 @@ contract('Governance', (accounts: string[]) => { [transactionSuccess1.destination], // @ts-ignore bytes type transactionSuccess1.data, - [transactionSuccess1.data.length] + [transactionSuccess1.data.length], + salt ) it('should revert when hotfix not approved', async () => { @@ -2079,28 +2082,28 @@ contract('Governance', (accounts: string[]) => { it('should revert when hotfix not prepared for current epoch', async () => { await mineBlocks(EPOCH, web3) - await governance.approveHotfix(proposalHashStr, { from: approver }) + await governance.approveHotfix(hotfixHashStr, { from: approver }) await assertRevert(executeHotfixTx()) }) it('should revert when hotfix prepared but not for current epoch', async () => { - await governance.approveHotfix(proposalHashStr, { from: approver }) + await governance.approveHotfix(hotfixHashStr, { from: approver }) await governance.addValidator(accounts[2]) await accountsInstance.createAccount({ from: accounts[2] }) - await governance.whitelistHotfix(proposalHashStr, { from: accounts[2] }) - await governance.prepareHotfix(proposalHashStr, { from: accounts[2] }) + await governance.whitelistHotfix(hotfixHashStr, { from: accounts[2] }) + await governance.prepareHotfix(hotfixHashStr, { from: accounts[2] }) await mineBlocks(EPOCH, web3) await assertRevert(executeHotfixTx()) }) describe('when hotfix is approved and prepared for current epoch', () => { beforeEach(async () => { - await governance.approveHotfix(proposalHashStr, { from: approver }) + await governance.approveHotfix(hotfixHashStr, { from: approver }) await mineBlocks(EPOCH, web3) await governance.addValidator(accounts[2]) await accountsInstance.createAccount({ from: accounts[2] }) - await governance.whitelistHotfix(proposalHashStr, { from: accounts[2] }) - await governance.prepareHotfix(proposalHashStr) + await governance.whitelistHotfix(hotfixHashStr, { from: accounts[2] }) + await governance.prepareHotfix(hotfixHashStr) }) it('should execute the hotfix tx', async () => { @@ -2110,7 +2113,7 @@ contract('Governance', (accounts: string[]) => { it('should mark the hotfix record as executed', async () => { await executeHotfixTx() - const [, executed] = await governance.getHotfixRecord.call(proposalHashStr) + const [, executed] = await governance.getHotfixRecord.call(hotfixHashStr) assert.isTrue(executed) }) @@ -2124,7 +2127,7 @@ contract('Governance', (accounts: string[]) => { hash: matchAny, }, }) - assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(proposalHash)) + assert.isTrue(Buffer.from(stripHexEncoding(log.args.hash), 'hex').equals(hotfixHash)) }) it('should not be executable again', async () => {