From b0ce871daaf56f0062b0afc555a6914342b0c090 Mon Sep 17 00:00:00 2001 From: Matt Solomon Date: Wed, 13 Apr 2022 09:15:03 -0700 Subject: [PATCH 1/2] chore: remove unused printer code --- checks/check-slither.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/checks/check-slither.ts b/checks/check-slither.ts index 345a2c57811..cb1d79a1ee9 100644 --- a/checks/check-slither.ts +++ b/checks/check-slither.ts @@ -72,21 +72,18 @@ export const checkSlither: ProposalCheck = { } /** - * Tries to run slither via python installation in the specified directory. If a printer name is - * passed, the printer will be run. + * Tries to run slither via python installation in the specified directory. * @dev If you have nix/dapptools installed, you'll need to make sure the path to your python * executables (find this with `which solc-select`) comes before the path to your nix executables. * This may require editing your $PATH variable prior to running this check. If you don't do this, - * the nix version of solc will take precedence over the solc-select version, and slither will fail - * @dev The list of available printers can be found here: https://github.com/crytic/slither/wiki/Printer-documentation + * the nix version of solc will take precedence over the solc-select version, and slither will fail. */ -async function runSlither(address: string, printer: string | undefined = undefined): Promise { +async function runSlither(address: string): Promise { try { - const printerCmd = printer ? ` --print ${printer}` : '' - return await exec(`slither ${address}${printerCmd} --etherscan-apikey ${ETHERSCAN_API_KEY}`) + return await exec(`slither ${address} --etherscan-apikey ${ETHERSCAN_API_KEY}`) } catch (e: any) { - if ('stderr' in e) return e // output is in stderr, but slither reports results as an exception - console.warn(`Error: Could not run slither${printer ? ` printer ${printer}` : ''} via Python: ${JSON.stringify(e)}`) + if ('stderr' in e) return e // Output is in stderr, but slither reports results as an exception. + console.warn(`Error: Could not run slither via Python: ${JSON.stringify(e)}`) return null } } From 3ff56fac8ce863b29b3d68d55ee46378756731b6 Mon Sep 17 00:00:00 2001 From: Matt Solomon Date: Fri, 15 Apr 2022 16:22:37 -0700 Subject: [PATCH 2/2] feat: add solc check, clean up slither check a bit --- checks/check-slither.ts | 11 +++-- checks/check-solc.ts | 92 +++++++++++++++++++++++++++++++++++++++++ checks/index.ts | 4 ++ 3 files changed, 101 insertions(+), 6 deletions(-) create mode 100644 checks/check-solc.ts diff --git a/checks/check-slither.ts b/checks/check-slither.ts index cb1d79a1ee9..9348ed10a59 100644 --- a/checks/check-slither.ts +++ b/checks/check-slither.ts @@ -44,9 +44,9 @@ export const checkSlither: ProposalCheck = { return { info: ['No contracts to analyze: only the timelock and governor are touched'], warnings, errors: [] } } - // For each verified contract we run slither. Slither has a mode to run it directly against a mainnet + // For each unique verified contract we run slither. Slither has a mode to run it directly against a mainnet // contract, which saves us from having to write files to a local temporary directory. - for (const contract of contracts) { + for (const contract of Array.from(new Set(contracts))) { const addr = getAddress(contract.address) if (addressesToSkip.has(addr)) continue @@ -61,10 +61,9 @@ export const checkSlither: ProposalCheck = { // Note that slither supports a `--json` flag we could use, but directly printing the formatted // results in a code block is simpler and sufficient for now. const formatting = info === '' ? '' : '\n- ' - info += `${formatting}Slither report for ${getContractName(contract)}` - if (slitherOutput) { - info += `\n\n
\nView Report\n\n\`\`\`\n${slitherOutput.stderr}\`\`\`\n\n
\n\n` - } + const contractName = getContractName(contract) + info += `${formatting}Slither report for ${contractName}` + info += `\n\n
\nView report for ${contractName}\n\n\`\`\`\n${slitherOutput.stderr}\`\`\`\n\n
\n\n` } return { info: [info], warnings, errors: [] } diff --git a/checks/check-solc.ts b/checks/check-solc.ts new file mode 100644 index 00000000000..deca3ecea16 --- /dev/null +++ b/checks/check-solc.ts @@ -0,0 +1,92 @@ +import util from 'util' +import { exec as execCallback } from 'child_process' +import { getAddress } from '@ethersproject/address' +import { getContractName } from '../utils/clients/tenderly' +import { ETHERSCAN_API_KEY } from '../utils/constants' +import { ProposalCheck } from '../types' + +// Convert exec method from a callback to a promise. +const exec = util.promisify(execCallback) + +// Data returned from command execution. +type ExecOutput = { + stdout: string + stderr: string +} + +/** + * Runs crytic-compile against verified contracts to obtain solc compiler warnings. Assumes crytic-compile + * is already installed. + */ +export const checkSolc: ProposalCheck = { + name: 'Runs solc against the verified contracts', + async checkProposal(proposal, sim, deps) { + let info = '' + let warnings: string[] = [] + + // Skip existing timelock and governor contracts to reduce noise. These contracts are already + // deployed and in use, and if they are being updated, the new contract will be one of the + // touched contracts that get's analyzed. + // NOTE: This requires an archive node since we need to query for the governor implementation + // at the simulation block number, since the implementation may have changed since. + const addressesToSkip = new Set([deps.timelock.address, deps.governor.address]) + try { + addressesToSkip.add(await deps.governor.implementation({ blockTag: sim.transaction.block_number })) + } catch (e) { + const msg = `Could not read address of governor implementation at block \`${sim.transaction.block_number}\`. Make sure the \`RPC_URL\` is an archive node. As a result the Slither check will show warnings on the governor's implementation contract.` + console.warn(`WARNING: ${msg}. Details:`) + console.warn(e) + warnings.push(msg) + } + + // Return early if the only contracts touched are the timelock and governor. + const contracts = sim.contracts.filter((contract) => !addressesToSkip.has(getAddress(contract.address))) + if (contracts.length === 0) { + return { info: ['No contracts to analyze: only the timelock and governor are touched'], warnings, errors: [] } + } + + // For each unique verified contract we run solc against it via crytic-compile. It has a mode to run it directly against + // a mainnet contract, which saves us from having to write files to a local temporary directory. + for (const contract of Array.from(new Set(contracts))) { + const addr = getAddress(contract.address) + if (addressesToSkip.has(addr)) continue + + // Compile the contracts. + const output = await runCryticCompile(contract.address) + if (!output) { + warnings.push(`crytic-compile failed for \`${contract.contract_name}\` at \`${addr}\``) + continue + } + + // Append results to report info. + const formatting = info === '' ? '' : '\n- ' + const contractName = getContractName(contract) + if (output.stderr === '') { + info += `${formatting}No compiler warnings for ${contractName}` + } else { + info += `${formatting}Compiler warnings for ${contractName}` + info += `\n\n
\nView warnings for ${contractName}\n\n\`\`\`\n${output.stderr}\`\`\`\n\n
\n\n` + } + } + + return { info: [info], warnings, errors: [] } + }, +} + +/** + * Tries to run crytic-compile via python installation in the specified directory. + * @dev Exports a zip file which is used by the slither check. + * @dev If you have nix/dapptools installed, you'll need to make sure the path to your python + * executables (find this with `which solc-select`) comes before the path to your nix executables. + * This may require editing your $PATH variable prior to running this check. If you don't do this, + * the nix version of solc will take precedence over the solc-select version, and compilation will fail. + */ +async function runCryticCompile(address: string): Promise { + try { + return await exec(`crytic-compile ${address} --etherscan-apikey ${ETHERSCAN_API_KEY}`) + } catch (e: any) { + if ('stderr' in e) return e // Output is in stderr, but slither reports results as an exception. + console.warn(`Error: Could not run crytic-compile via Python: ${JSON.stringify(e)}`) + return null + } +} diff --git a/checks/index.ts b/checks/index.ts index c2aaffbb85f..f3eac626926 100644 --- a/checks/index.ts +++ b/checks/index.ts @@ -5,6 +5,7 @@ import { import { checkDecodeCalldata } from './check-decode-calldata' import { checkLogs } from './check-logs' import { checkSlither } from './check-slither' +import { checkSolc } from './check-solc' import { checkStateChanges } from './check-state-changes' import { ProposalCheck } from '../types' @@ -16,6 +17,9 @@ const ALL_CHECKS: { checkLogs, checkTargetsVerifiedEtherscan, checkTouchedContractsVerifiedEtherscan, + // The solc check must be run before the slither check, because the compilation exports a zip file + // which is consumed by slither. This prevents us from having to compile the contracts twice. + checkSolc, checkSlither, }