From 52ae5f11f72f193f5a91363b11d66a76a980ec69 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Sun, 7 Apr 2024 19:00:35 -0400 Subject: [PATCH] fix(pg): throw an error if contracts are above the size limit --- .changeset/curly-rules-boil.md | 6 ++ .../contracts/contracts/foundry/Sphinx.sol | 7 ++ packages/contracts/src/constants.ts | 2 + packages/core/src/utils.ts | 10 +++ .../plugins/contracts/test/MyContracts.sol | 3 + packages/plugins/src/cli/deploy.ts | 6 ++ packages/plugins/src/cli/propose/index.ts | 8 ++ packages/plugins/src/error-messages.ts | 22 ++++++ packages/plugins/src/foundry/utils/index.ts | 35 +++++++++ packages/plugins/test/mocha/dummy.ts | 27 ++++++- .../plugins/test/mocha/error-messages.spec.ts | 25 ++++++ packages/plugins/test/mocha/fake.ts | 22 ++++++ .../plugins/test/mocha/foundry/utils.spec.ts | 76 +++++++++++++++++++ 13 files changed, 248 insertions(+), 1 deletion(-) create mode 100644 .changeset/curly-rules-boil.md create mode 100644 packages/plugins/src/error-messages.ts create mode 100644 packages/plugins/test/mocha/error-messages.spec.ts diff --git a/.changeset/curly-rules-boil.md b/.changeset/curly-rules-boil.md new file mode 100644 index 000000000..f12e96a7a --- /dev/null +++ b/.changeset/curly-rules-boil.md @@ -0,0 +1,6 @@ +--- +'@sphinx-labs/plugins': patch +'@sphinx-labs/core': patch +--- + +Throw an error if contracts are above the size limit diff --git a/packages/contracts/contracts/foundry/Sphinx.sol b/packages/contracts/contracts/foundry/Sphinx.sol index 290c61e40..3d262ead2 100644 --- a/packages/contracts/contracts/foundry/Sphinx.sol +++ b/packages/contracts/contracts/foundry/Sphinx.sol @@ -250,6 +250,13 @@ abstract contract Sphinx { // the delegatecall in our error message. require(success, "Sphinx: Deployment script failed."); Vm.AccountAccess[] memory accesses = vm.stopAndReturnStateDiff(); + // Set the `deployedCode` field for each AccountAccess of kind `Create`. There may be a bug + // in Foundry that causes this field to not always be populated. + for (uint256 i = 0; i < accesses.length; i++) { + if (accesses[i].kind == VmSafe.AccountAccessKind.Create) { + accesses[i].deployedCode = accesses[i].account.code; + } + } // We have to copy fundsRequestedForSafe and safeStartingBalance into deploymentInfo before // calling vm.revertTo because that cheatcode will clear the state variables. diff --git a/packages/contracts/src/constants.ts b/packages/contracts/src/constants.ts index a7e5b1874..9dc786d9b 100644 --- a/packages/contracts/src/constants.ts +++ b/packages/contracts/src/constants.ts @@ -42,3 +42,5 @@ export const EXECUTION_LOCK_TIME = 15 * 60 export const RECOMMENDED_REMAPPING = '@sphinx-labs/contracts/=lib/sphinx/packages/contracts/contracts/foundry' + +export const MAX_CONTRACT_SIZE_LIMIT = 24576 diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index b27f34fa8..8101de69b 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -35,6 +35,7 @@ import { recursivelyConvertResult, DETERMINISTIC_DEPLOYMENT_PROXY_ADDRESS, CreateCallArtifact, + MAX_CONTRACT_SIZE_LIMIT, } from '@sphinx-labs/contracts' import { @@ -1709,4 +1710,13 @@ export const isPublicAsyncMethod = ( return false } +/** + * Returns true if the given bytecode exceeds the contract size size limit as defined by: + * https://github.com/ethereum/EIPs/blob/master/EIPS/eip-170.md + */ +export const exceedsContractSizeLimit = (deployedBytecode: string): boolean => { + const bytesLength = remove0x(deployedBytecode).length / 2 + return bytesLength > MAX_CONTRACT_SIZE_LIMIT +} + export const sphinxCoreUtils = { sleep, callWithTimeout, isPublicAsyncMethod } diff --git a/packages/plugins/contracts/test/MyContracts.sol b/packages/plugins/contracts/test/MyContracts.sol index adbb137d2..b10f8285f 100644 --- a/packages/plugins/contracts/test/MyContracts.sol +++ b/packages/plugins/contracts/test/MyContracts.sol @@ -5,6 +5,7 @@ import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; import { AccessControl } from "@openzeppelin/contracts/access/AccessControl.sol"; import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import { Governor } from "@openzeppelin/contracts/governance/Governor.sol"; +import { SphinxUtils } from "@sphinx-labs/contracts/contracts/foundry/SphinxUtils.sol"; struct TopLevelStruct { int256 a; @@ -200,3 +201,5 @@ contract MyImmutableContract { return myFirstImmutable + uint256(mySecondImmutable); } } + +contract ExceedsContractMaxSizeLimit is SphinxUtils {} diff --git a/packages/plugins/src/cli/deploy.ts b/packages/plugins/src/cli/deploy.ts index 4a7f2fc7f..2b961269f 100644 --- a/packages/plugins/src/cli/deploy.ts +++ b/packages/plugins/src/cli/deploy.ts @@ -52,6 +52,7 @@ import { getSphinxConfigFromScript, parseScriptFunctionCalldata, readInterface, + assertContractSizeLimitNotExceeded, writeSystemContracts, } from '../foundry/utils' import { getFoundryToml } from '../foundry/options' @@ -304,6 +305,11 @@ export const deploy = async ( initCodeWithArgsArray ) + assertContractSizeLimitNotExceeded( + deploymentInfo.accountAccesses, + configArtifacts + ) + await sphinxContext.assertNoLinkedLibraries( scriptPath, foundryToml.cachePath, diff --git a/packages/plugins/src/cli/propose/index.ts b/packages/plugins/src/cli/propose/index.ts index 870d65238..753561f04 100644 --- a/packages/plugins/src/cli/propose/index.ts +++ b/packages/plugins/src/cli/propose/index.ts @@ -43,6 +43,7 @@ import { assertValidVersions, validateProposalNetworks, parseScriptFunctionCalldata, + assertContractSizeLimitNotExceeded, } from '../../foundry/utils' import { SphinxContext } from '../context' import { FoundryToml } from '../../foundry/types' @@ -177,6 +178,13 @@ export const buildNetworkConfigArray: BuildNetworkConfigArray = async ( initCodeWithArgsArray ) + collected.forEach(({ deploymentInfo }) => + assertContractSizeLimitNotExceeded( + deploymentInfo.accountAccesses, + configArtifacts + ) + ) + await sphinxContext.assertNoLinkedLibraries( scriptPath, foundryToml.cachePath, diff --git a/packages/plugins/src/error-messages.ts b/packages/plugins/src/error-messages.ts new file mode 100644 index 000000000..fe8e7a596 --- /dev/null +++ b/packages/plugins/src/error-messages.ts @@ -0,0 +1,22 @@ +export const contractsExceedSizeLimitErrorMessage = ( + contracts: Array<{ address: string; fullyQualifiedName?: string }> +): string => { + const contractLines = contracts + .map((contract) => { + let line = `- ` + if (contract.fullyQualifiedName) { + line += `${contract.fullyQualifiedName} at ` + } + line += `${contract.address}` + if (!contract.fullyQualifiedName) { + line += ' (unlabeled)' + } + return line + }) + .join('\n') + + return ( + `The following contracts are over the contract size limit (24,576 bytes), which means they\n` + + `cannot be deployed on live networks:\n${contractLines}` + ) +} diff --git a/packages/plugins/src/foundry/utils/index.ts b/packages/plugins/src/foundry/utils/index.ts index 1972f1f6a..582d40e3a 100644 --- a/packages/plugins/src/foundry/utils/index.ts +++ b/packages/plugins/src/foundry/utils/index.ts @@ -19,6 +19,7 @@ import { } from '@sphinx-labs/core/dist/languages/solidity/types' import { decodeDeterministicDeploymentProxyData, + exceedsContractSizeLimit, execAsync, fetchNetworkConfigFromDeploymentConfig, formatSolcLongVersion, @@ -101,6 +102,7 @@ import { getMixedNetworkTypeErrorMessage, getUnsupportedNetworkErrorMessage, } from '../error-messages' +import { contractsExceedSizeLimitErrorMessage } from '../../error-messages' const readFileAsync = promisify(readFile) @@ -1772,3 +1774,36 @@ export const validateProposalNetworks = async ( const rpcUrls = valid.map(({ rpcUrl }) => rpcUrl) return { rpcUrls, isTestnet } } + +export const assertContractSizeLimitNotExceeded = ( + accountAccesses: Array, + configArtifacts: ConfigArtifacts +): void => { + const flat = accountAccesses.flatMap((access) => [ + access.root, + ...access.nested, + ]) + + const tooLarge: Array<{ address: string; fullyQualifiedName?: string }> = [] + + for (const access of flat) { + if ( + access.kind === AccountAccessKind.Create && + exceedsContractSizeLimit(access.deployedCode) + ) { + const initCodeWithArgs = access.data + // Find the fully qualified name if it exists. This call returns `undefined` if the contract + // is unlabeled. + const fullyQualifiedName = findFullyQualifiedNameForInitCode( + initCodeWithArgs, + configArtifacts + ) + + tooLarge.push({ address: access.account, fullyQualifiedName }) + } + } + + if (tooLarge.length > 0) { + throw new Error(contractsExceedSizeLimitErrorMessage(tooLarge)) + } +} diff --git a/packages/plugins/test/mocha/dummy.ts b/packages/plugins/test/mocha/dummy.ts index b23bd627e..fcb7443b0 100644 --- a/packages/plugins/test/mocha/dummy.ts +++ b/packages/plugins/test/mocha/dummy.ts @@ -1,4 +1,9 @@ -import { Operation, SphinxMerkleTree } from '@sphinx-labs/contracts' +import { + AccountAccess, + AccountAccessKind, + Operation, + SphinxMerkleTree, +} from '@sphinx-labs/contracts' import { ActionInputType, BuildInfo, @@ -340,3 +345,23 @@ export const getDummyDeployment = (): Deployment => { treeSigners: [], } } + +export const getDummyAccountAccess = (): AccountAccess => { + return { + chainInfo: { + forkId: '0x1', + chainId: '0x3', + }, + kind: AccountAccessKind.Balance, + account: '0xAccount', + accessor: '0xAccessor', + initialized: true, + oldBalance: '1000', + newBalance: '1500', + deployedCode: '0xCode', + value: '500', + data: '0xData', + reverted: false, + storageAccesses: [], + } +} diff --git a/packages/plugins/test/mocha/error-messages.spec.ts b/packages/plugins/test/mocha/error-messages.spec.ts new file mode 100644 index 000000000..2baf70c89 --- /dev/null +++ b/packages/plugins/test/mocha/error-messages.spec.ts @@ -0,0 +1,25 @@ +import { expect } from 'chai' + +import { contractsExceedSizeLimitErrorMessage } from '../../src/error-messages' + +describe('Error Messages', () => { + it('contractsExceedSizeLimitErrorMessage', () => { + const contracts = [ + { + address: '0xABCDEF1234567890ABCDEF1234567890ABCDEF12', + fullyQualifiedName: 'contracts/ExampleContract.sol:ExampleContract', + }, + { address: '0x1234567890ABCDEF1234567890ABCDEF12345678' }, // Unlabeled + ] + // eslint-disable-next-line prettier/prettier + const expectedMessage = +`The following contracts are over the contract size limit (24,576 bytes), which means they +cannot be deployed on live networks: +- contracts/ExampleContract.sol:ExampleContract at 0xABCDEF1234567890ABCDEF1234567890ABCDEF12 +- 0x1234567890ABCDEF1234567890ABCDEF12345678 (unlabeled)` + + const actualMessage = contractsExceedSizeLimitErrorMessage(contracts) + + expect(actualMessage).to.equal(expectedMessage) + }) +}) diff --git a/packages/plugins/test/mocha/fake.ts b/packages/plugins/test/mocha/fake.ts index bfc5a0c67..5522ca585 100644 --- a/packages/plugins/test/mocha/fake.ts +++ b/packages/plugins/test/mocha/fake.ts @@ -6,8 +6,10 @@ import { SphinxTransactionReceipt, } from '@sphinx-labs/core' import { + AccountAccessKind, ContractArtifact, Operation, + ParsedAccountAccess, SphinxModuleABI, } from '@sphinx-labs/contracts' import { EventFragment, ethers } from 'ethers' @@ -15,6 +17,7 @@ import { EventFragment, ethers } from 'ethers' import { dummyBuildInfoId, dummyModuleAddress, + getDummyAccountAccess, getDummyBuildInfos, getDummyCompilerInput, getDummyEventLog, @@ -145,3 +148,22 @@ export const getFakeDeploymentConfig = async ( version: '0', } } + +export const getFakeParsedAccountAccess = (fields: { + kind: AccountAccessKind + data: string + account: string + deployedCode: string +}): ParsedAccountAccess => { + const { kind, data, account, deployedCode } = fields + const root = getDummyAccountAccess() + root.kind = kind + root.data = data + root.account = account + root.deployedCode = deployedCode + + return { + root, + nested: [], + } +} diff --git a/packages/plugins/test/mocha/foundry/utils.spec.ts b/packages/plugins/test/mocha/foundry/utils.spec.ts index b7baafb2f..186020603 100644 --- a/packages/plugins/test/mocha/foundry/utils.spec.ts +++ b/packages/plugins/test/mocha/foundry/utils.spec.ts @@ -6,8 +6,10 @@ import chai from 'chai' import chaiAsPromised from 'chai-as-promised' import { ConstructorFragment, ethers } from 'ethers' import { + AccountAccessKind, ContractArtifact, LinkReferences, + MAX_CONTRACT_SIZE_LIMIT, parseFoundryContractArtifact, remove0x, } from '@sphinx-labs/contracts' @@ -33,6 +35,7 @@ import * as MyContract2Artifact from '../../../out/artifacts/MyContracts.sol/MyC import * as MyContractWithLibrariesArtifact from '../../../out/artifacts/MyContracts.sol/MyContractWithLibraries.json' import * as MyImmutableContractArtifact from '../../../out/artifacts/MyContracts.sol/MyImmutableContract.json' import * as MyLargeContractArtifact from '../../../out/artifacts/MyContracts.sol/MyLargeContract.json' +import * as ExceedsContractMaxSizeLimitArtifact from '../../../out/artifacts/MyContracts.sol/ExceedsContractMaxSizeLimit.json' import { encodeFunctionCalldata, getAnvilRpcUrl, @@ -43,6 +46,7 @@ import { } from '../common' import { FoundryToml } from '../../../src/foundry/types' import { + assertContractSizeLimitNotExceeded, assertNoLinkedLibraries, makeGetConfigArtifacts, parseScriptFunctionCalldata, @@ -59,6 +63,9 @@ import { getMixedNetworkTypeErrorMessage, getUnsupportedNetworkErrorMessage, } from '../../../src/foundry/error-messages' +import { contractsExceedSizeLimitErrorMessage } from '../../../src/error-messages' +import { dummyUnlabeledAddress } from '../dummy' +import { getFakeConfigArtifacts, getFakeParsedAccountAccess } from '../fake' describe('Utils', async () => { let foundryToml: FoundryToml @@ -1188,4 +1195,73 @@ describe('Utils', async () => { expect(actualCalldata).to.equal(calldata) }) }) + + describe('assertContractSizeLimitNotExceeded', () => { + it('does not throw an error when contract size is less than the size limit', async () => { + const artifact = parseFoundryContractArtifact(MyContract2Artifact) + const address = makeAddress(1) + const fullyQualifiedName = `${artifact.sourceName}:${artifact.contractName}` + + const accesses = [ + getFakeParsedAccountAccess({ + kind: AccountAccessKind.Create, + data: artifact.bytecode, + account: address, + deployedCode: artifact.deployedBytecode, + }), + ] + const configArtifacts = await getFakeConfigArtifacts( + [fullyQualifiedName], + foundryToml.artifactFolder + ) + + expect(() => + assertContractSizeLimitNotExceeded(accesses, configArtifacts) + ).to.not.throw + }) + + it('throws an error when a labeled and unlabeled contract exceed the size limit', async () => { + const exceedsMaxSizeAddress = '0x' + '11'.repeat(20) + const exceedsMaxSizeArtifact = parseFoundryContractArtifact( + ExceedsContractMaxSizeLimitArtifact + ) + const exceedsMaxSizeFullyQualifiedName = `${exceedsMaxSizeArtifact.sourceName}:${exceedsMaxSizeArtifact.contractName}` + + const expectedErrorMessageInput = [ + { + address: dummyUnlabeledAddress, + }, + { + address: exceedsMaxSizeAddress, + fullyQualifiedName: exceedsMaxSizeFullyQualifiedName, + }, + ] + + const accesses = [ + getFakeParsedAccountAccess({ + kind: AccountAccessKind.Create, + data: '', // Empty so that the initCodeWithArgs doesn't match a labeled contract + account: dummyUnlabeledAddress, + deployedCode: '0x' + '00'.repeat(MAX_CONTRACT_SIZE_LIMIT + 1), + }), + getFakeParsedAccountAccess({ + kind: AccountAccessKind.Create, + data: ExceedsContractMaxSizeLimitArtifact.bytecode.object, + account: exceedsMaxSizeAddress, + deployedCode: + ExceedsContractMaxSizeLimitArtifact.deployedBytecode.object, + }), + ] + const configArtifacts = await getFakeConfigArtifacts( + [exceedsMaxSizeFullyQualifiedName], + foundryToml.artifactFolder + ) + + expect(() => + assertContractSizeLimitNotExceeded(accesses, configArtifacts) + ).to.throw( + contractsExceedSizeLimitErrorMessage(expectedErrorMessageInput) + ) + }) + }) })