Verify proxies in contract release verification (#5828)

* Add check to verify proxy bytecode * Verify owners * Upgrade web3 to 1.2.6 and enable esModuleInterop * Add proxy root verification via MPTs * Update imports to match synthetic default imports * Remove legacy web3 imports * Skip eth_getProof on dev network * Update yarn lock
celo-org · Nov 19, 2020 · f4d05ef · f4d05ef
1 parent 2ff8fac
commit f4d05ef
Show file tree

Hide file tree

Showing 27 changed files with 820 additions and 132 deletions.
diff --git a/packages/protocol/lib/bytecode.ts b/packages/protocol/lib/bytecode.ts
@@ -5,7 +5,7 @@
  * https://solidity.readthedocs.io/en/develop/metadata.html#encoding-of-the-metadata-hash-in-the-bytecode
  */
 
-import { trimLeading0x } from '@celo/base/lib/address'
+import { NULL_ADDRESS, trimLeading0x } from '@celo/base/lib/address'
 
 const CONTRACT_METADATA_REGEXPS = [
   // 0.5.8
@@ -68,7 +68,7 @@ const PUSH20_OPCODE = '73'
 // the compiler's output contains a placeholder 0-address, while the onchain
 // bytecode has the correct address inserted.
 // Reference: https://solidity.readthedocs.io/en/v0.5.12/contracts.html#call-protection-for-libraries
-export const verifyLibraryPrefix = (bytecode: string, address: string) => {
+export const verifyAndStripLibraryPrefix = (bytecode: string, address = NULL_ADDRESS) => {
   if (bytecode.slice(2, 4) !== PUSH20_OPCODE) {
     throw new Error(`Library bytecode doesn't start with address load`)
   } else if (bytecode.slice(4, 4 + ADDRESS_LENGTH) !== trimLeading0x(address).toLowerCase()) {

diff --git a/packages/protocol/lib/compatibility/verify-bytecode.ts b/packages/protocol/lib/compatibility/verify-bytecode.ts
@@ -1,10 +1,13 @@
+// tslint:disable: no-console
+import { ensureLeading0x } from '@celo/base/lib/address'
 import {
   LibraryAddresses,
   LibraryPositions,
   linkLibraries,
   stripMetadata,
-  verifyLibraryPrefix,
+  verifyAndStripLibraryPrefix,
 } from '@celo/protocol/lib/bytecode'
+import { verifyProxyStorageProof } from '@celo/protocol/lib/proxy-utils'
 import { ProposalTx } from '@celo/protocol/scripts/truffle/make-release'
 import { BuildArtifacts } from '@openzeppelin/upgrades'
 import { ProxyInstance, RegistryInstance } from 'types'
@@ -23,26 +26,27 @@ interface VerificationContext {
   artifacts: BuildArtifacts
   libraryAddresses: LibraryAddresses
   registry: RegistryInstance
+  governanceAddress: string
   proposal: ProposalTx[]
   Proxy: Truffle.Contract<ProxyInstance>
   web3: Web3
+  network: string
 }
 interface InitializationData {
   [contractName: string]: any[]
 }
 
 const ContractNameExtractorRegex = new RegExp(/(.*)Proxy/)
 
-
 // Checks if the given transaction is a repointing of the Proxy for the given
 // contract.
 const isProxyRepointTransaction = (tx: ProposalTx) =>
   tx.contract.endsWith('Proxy') &&
   (tx.function === '_setImplementation' || tx.function === '_setAndInitializeImplementation')
 
 const isProxyRepointAndInitializeTransaction = (tx: ProposalTx) =>
-  tx.contract.endsWith('Proxy') &&
-    tx.function === '_setAndInitializeImplementation'
+  tx.contract.endsWith('Proxy') && tx.function === '_setAndInitializeImplementation'
+
 const isProxyRepointForIdTransaction = (tx: ProposalTx, contract: string) =>
   tx.contract === `${contract}Proxy` && isProxyRepointTransaction(tx)
 
@@ -69,69 +73,72 @@ const getProposedProxyAddress = (contract: string, context: VerificationContext)
   return relevantTx.args[1]
 }
 
+const getSourceBytecodeFromArtifacts = (contract: string, artifacts: BuildArtifacts): string =>
+  stripMetadata(artifacts.getArtifactByName(contract).deployedBytecode)
+
 const getSourceBytecode = (contract: string, context: VerificationContext): string =>
-  stripMetadata(context.artifacts.getArtifactByName(contract).deployedBytecode)
+  getSourceBytecodeFromArtifacts(contract, context.artifacts)
 
 const getOnchainBytecode = async (address: string, context: VerificationContext) =>
   stripMetadata(await context.web3.eth.getCode(address))
 
 const isLibrary = (contract: string, context: VerificationContext) =>
   contract in context.libraryAddresses.addresses
 
-const getImplementationAddress = async (contract: string, context: VerificationContext) => {
-  // Where we find the implementation address depends on two factors:
-  // 1. Is the contract affected by a governance proposal?
-  // 2. Is the contract registered in the Registry or a linked library?
-
-  if (isImplementationChanged(contract, context)) {
-    return getProposedImplementationAddress(contract, context)
-  }
-
-  if (isLibrary(contract, context)) {
-    return `0x${context.libraryAddresses.addresses[contract]}`
-  }
-
-  // contract is registered but we need to check if the proxy is affected by the proposal
-  const proxyAddress = isProxyChanged(contract, context)
-    ? getProposedProxyAddress(contract, context)
-    : await context.registry.getAddressForString(contract)
-
-  // at() returns a promise despite Typescript labelling the await as extraneous
-  const proxy: ProxyInstance = await context.Proxy.at(
-    context.web3.utils.toChecksumAddress(proxyAddress)
-  )
-  return proxy._getImplementation()
-}
-
 const dfsStep = async (queue: string[], visited: Set<string>, context: VerificationContext) => {
   const contract = queue.pop()
   // mark current DFS node as visited
   visited.add(contract)
 
-  // get source code
+  // check proxy deployment
+  if (isProxyChanged(contract, context)) {
+    const proxyAddress = getProposedProxyAddress(contract, context)
+    // ganache does not support eth_getProof
+    if (
+      context.network !== 'development' &&
+      !(await verifyProxyStorageProof(context.web3, proxyAddress, context.governanceAddress))
+    ) {
+      throw new Error(`Proposed ${contract}Proxy has impure storage`)
+    }
+
+    const onchainProxyBytecode = await getOnchainBytecode(proxyAddress, context)
+    const sourceProxyBytecode = getSourceBytecode(`${contract}Proxy`, context)
+    if (onchainProxyBytecode !== sourceProxyBytecode) {
+      throw new Error(`Proposed ${contract}Proxy does not match compiled proxy bytecode`)
+    }
+
+    console.log(`Proxy deployed at ${proxyAddress} matches ${contract}Proxy (bytecode and storage)`)
+  }
+
+  // check implementation deployment
   const sourceBytecode = getSourceBytecode(contract, context)
   const sourceLibraryPositions = new LibraryPositions(sourceBytecode)
 
-  // get deployed code
-  const implementationAddress = await getImplementationAddress(contract, context)
+  let implementationAddress: string
+  if (isImplementationChanged(contract, context)) {
+    implementationAddress = getProposedImplementationAddress(contract, context)
+  } else if (isLibrary(contract, context)) {
+    implementationAddress = ensureLeading0x(context.libraryAddresses.addresses[contract])
+  } else {
+    const proxyAddress = await context.registry.getAddressForString(contract)
+    const proxy = await context.Proxy.at(proxyAddress) // necessary await
+    implementationAddress = await proxy._getImplementation()
+  }
+
   let onchainBytecode = await getOnchainBytecode(implementationAddress, context)
   context.libraryAddresses.collect(onchainBytecode, sourceLibraryPositions)
 
   let linkedSourceBytecode = linkLibraries(sourceBytecode, context.libraryAddresses.addresses)
 
   // normalize library bytecodes
   if (isLibrary(contract, context)) {
-    linkedSourceBytecode = verifyLibraryPrefix(
-      linkedSourceBytecode,
-      '0000000000000000000000000000000000000000'
-    )
-    onchainBytecode = verifyLibraryPrefix(onchainBytecode, implementationAddress)
+    linkedSourceBytecode = verifyAndStripLibraryPrefix(linkedSourceBytecode)
+    onchainBytecode = verifyAndStripLibraryPrefix(onchainBytecode, implementationAddress)
   }
 
   if (onchainBytecode !== linkedSourceBytecode) {
     throw new Error(`${contract}'s onchain and compiled bytecodes do not match`)
   } else {
-    // tslint:disable-next-line: no-console
     console.log(
       `${
         isLibrary(contract, context) ? 'Library' : 'Contract'
@@ -153,7 +160,7 @@ const assertValidProposalTransactions = (proposal: ProposalTx[]) => {
     throw new Error(`Proposal contains invalid release transactions ${invalidTransactions}`)
   }
 
-  console.info("Proposal contains only valid release transactions!")
+  console.info('Proposal contains only valid release transactions!')
 }
 
 const assertValidInitializationData = (
@@ -168,25 +175,32 @@ const assertValidInitializationData = (
     const contractName = ContractNameExtractorRegex.exec(proposalTx.contract)[1]
 
     if (!initializationData[contractName]) {
-      throw new Error(`Initialization Data for ${contractName} could not be found in reference file`)
+      throw new Error(
+        `Initialization Data for ${contractName} could not be found in reference file`
+      )
     }
 
     const contract = artifacts.getArtifactByName(contractName)
     const initializeAbi = contract.abi.find(
-      (abi: any) => abi.type === 'function' && abi.name === 'initialize')
+      (abi: any) => abi.type === 'function' && abi.name === 'initialize'
+    )
     const args = initializationData[contractName]
     const callData = web3.eth.abi.encodeFunctionCall(initializeAbi, args)
 
     if (callData !== proposalTx.args[1]) {
-      throw new Error(`Intialization Data for ${contractName} in proposal does not match reference file ${initializationData[contractName]}`)
+      throw new Error(
+        `Intialization Data for ${contractName} in proposal does not match reference file ${initializationData[contractName]}`
+      )
     }
 
     contractsInitialized.add(contractName)
   }
 
   for (const referenceContractName of Object.keys(initializationData)) {
     if (!contractsInitialized.has(referenceContractName)) {
-      throw new Error(`Reference file has initialization data for ${referenceContractName}, but proposal does not specify initialization`)
+      throw new Error(
+        `Reference file has initialization data for ${referenceContractName}, but proposal does not specify initialization`
+      )
     }
   }
 
@@ -204,22 +218,29 @@ export const verifyBytecodes = async (
   registry: RegistryInstance,
   proposal: ProposalTx[],
   Proxy: Truffle.Contract<ProxyInstance>,
-  web3: Web3,
-  initializationData: InitializationData = {}
+  _web3: Web3,
+  initializationData: InitializationData = {},
+  network = 'development'
 ) => {
   assertValidProposalTransactions(proposal)
-  assertValidInitializationData(artifacts, proposal, web3, initializationData)
+  assertValidInitializationData(artifacts, proposal, _web3, initializationData)
 
   const queue = contracts.filter((contract) => !ignoredContracts.includes(contract))
   const visited: Set<string> = new Set(queue)
 
+  // truffle web3 version does not have getProof
+  const web3 = new Web3(_web3.currentProvider)
+
+  const governanceAddress = await registry.getAddressForString('Governance')
   const context: VerificationContext = {
     artifacts,
     libraryAddresses: new LibraryAddresses(),
     registry,
+    governanceAddress,
     proposal,
     Proxy,
     web3,
+    network,
   }
 
   while (queue.length > 0) {

diff --git a/packages/protocol/lib/proxy-utils.ts b/packages/protocol/lib/proxy-utils.ts
@@ -1,8 +1,36 @@
-import { Address } from '@celo/utils/lib/address'
-import * as prompts from 'prompts'
+import { Address, bufferToHex, hexToBuffer } from '@celo/base/lib/address'
+import { SecureTrie } from 'merkle-patricia-tree'
+import prompts from 'prompts'
+import { encode as rlpEncode } from 'rlp'
 import { ProxyInstance } from 'types'
 import Web3 from 'web3'
 
+// from Proxy.sol
+
+// bytes32 private constant OWNER_POSITION = bytes32(
+//   uint256(keccak256("eip1967.proxy.admin")) - 1
+// );
+const OWNER_POSITION = '0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103'
+
+// bytes32 private constant IMPLEMENTATION_POSITION = bytes32(
+//   uint256(keccak256("eip1967.proxy.implementation")) - 1
+// );
+const IMPLEMENTATION_POSITION = '0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc'
+
+export async function verifyProxyStorageProof(web3: Web3, proxy: string, owner: string) {
+  const proof = await web3.eth.getProof(
+    web3.utils.toChecksumAddress(proxy),
+    [OWNER_POSITION, IMPLEMENTATION_POSITION],
+    'latest'
+  )
+
+  const trie = new SecureTrie()
+  await trie.put(hexToBuffer(OWNER_POSITION), rlpEncode(owner))
+  // for future use
+  // await trie.put(hexToBuffer(IMPLEMENTATION_POSITION), rlpEncode(implementation))
+
+  return proof.storageHash === bufferToHex(trie.root)
+}
 
 export async function retryTx(fn: any, args: any[]) {
   while (true) {
@@ -43,13 +71,19 @@ export async function setAndInitializeImplementation(
       // Proxy's fallback fn expects the contract's implementation to be set already
       // So we set the implementation first, send the funding, and then set and initialize again.
       await retryTx(proxy._setImplementation, [implementationAddress, { from: txOptions.from }])
-      await retryTx(web3.eth.sendTransaction, [{
-        from: txOptions.from,
-        to: proxy.address,
-        value: txOptions.value,
-      }])
+      await retryTx(web3.eth.sendTransaction, [
+        {
+          from: txOptions.from,
+          to: proxy.address,
+          value: txOptions.value,
+        },
+      ])
     }
-    return retryTx(proxy._setAndInitializeImplementation, [implementationAddress, callData as any, { from: txOptions.from }])
+    return retryTx(proxy._setAndInitializeImplementation, [
+      implementationAddress,
+      callData as any,
+      { from: txOptions.from },
+    ])
   } else {
     return retryTx(proxy._setAndInitializeImplementation, [implementationAddress, callData as any])
   }

diff --git a/packages/protocol/lib/signing-utils.ts b/packages/protocol/lib/signing-utils.ts
@@ -2,11 +2,10 @@
 
 import { parseSignature } from '@celo/utils/lib/signatureUtils'
 import { account as Account, bytes, hash, nat, RLP } from 'eth-lib'
-import * as _ from 'underscore'
-import * as helpers from 'web3-core-helpers'
-import * as utils from 'web3-utils'
-
+import _ from 'underscore'
 import Web3 from 'web3'
+import helpers from 'web3-core-helpers'
+import utils from 'web3-utils'
 
 function isNot(value: any) {
   return _.isUndefined(value) || _.isNull(value)