diff --git a/pkg/solidity-utils/contracts/helpers/BaseSplitCodeFactory.sol b/pkg/solidity-utils/contracts/helpers/BaseSplitCodeFactory.sol index f7fd47f05a..603508babb 100644 --- a/pkg/solidity-utils/contracts/helpers/BaseSplitCodeFactory.sol +++ b/pkg/solidity-utils/contracts/helpers/BaseSplitCodeFactory.sol @@ -71,7 +71,12 @@ abstract contract BaseSplitCodeFactory { // Memory: [ A.length ] [ A.data ] [ B.data ] // ^ creationCodeA - _creationCodeContractA = CodeDeployer.deploy(creationCodeA); + // The first half is the beginning of the real contract (as opposed to the second half, which could be + // anything), so we don't strictly need to protect the A contract. Fork tests should test both, + // for completeness. + bool preventExecution = false; + + _creationCodeContractA = CodeDeployer.deploy(creationCodeA, preventExecution); // Creating B's array is a bit more involved: since we cannot move B's contents, we are going to create a 'new' // memory array starting at A's last 32 bytes, which will be replaced with B's length. We'll back-up this last @@ -91,7 +96,11 @@ abstract contract BaseSplitCodeFactory { // Memory: [ A.length ] [ A.data[ : -1] ] [ B.length ][ B.data ] // ^ creationCodeA ^ creationCodeB - _creationCodeContractB = CodeDeployer.deploy(creationCodeB); + // The code for contract B starts at a random point, and could accidentally contain valid opcodes. + // The `preventExecution` flag prepends an invalid opcode to ensure the "contract" cannot be accidentally + // (or maliciously) executed. We need to remove this extra byte when reassembling the creation code. + preventExecution = true; + _creationCodeContractB = CodeDeployer.deploy(creationCodeB, preventExecution); // We now restore the original contents of `creationCode` by writing back the original length and A's last byte. assembly { @@ -149,7 +158,8 @@ abstract contract BaseSplitCodeFactory { // Next, we concatenate the creation code stored in A and B. let dataStart := add(code, 32) extcodecopy(creationCodeContractA, dataStart, 0, creationCodeSizeA) - extcodecopy(creationCodeContractB, add(dataStart, creationCodeSizeA), 0, creationCodeSizeB) + // Start at offset 1 in contract B, to skip the inserted invalid opcode. + extcodecopy(creationCodeContractB, add(dataStart, creationCodeSizeA), 1, creationCodeSizeB) } // Finally, we copy the constructorArgs to the end of the array. Unfortunately there is no way to avoid this diff --git a/pkg/solidity-utils/contracts/helpers/CodeDeployer.sol b/pkg/solidity-utils/contracts/helpers/CodeDeployer.sol index 6bd819b610..f7dace3ece 100644 --- a/pkg/solidity-utils/contracts/helpers/CodeDeployer.sol +++ b/pkg/solidity-utils/contracts/helpers/CodeDeployer.sol @@ -54,16 +54,63 @@ library CodeDeployer { // // The padding is just the 0xfe sequence (invalid opcode). It is important as it lets us work in-place, avoiding // memory allocation and copying. + bytes32 private constant _DEPLOYER_CREATION_CODE = 0x602038038060206000396000f3fefefefefefefefefefefefefefefefefefefe; + // Sometimes (e.g., when deploying the second or "B" half of the creation code in BaseSplitCodeFactory), we need to + // protect the bare contract from being accidentally (or maliciously) executed, in case the bytes at the boundary + // happen to be valid opcodes. It's especially dangerous if the bytes contained the selfdestruct opcode, which would + // destroy the contract (and, if it's a factory, effectively disable it and prevent further pool creation). + // + // To guard against this, if the "preventExecution" flag is set, we prepend an invalid opcode to the contract, + // to ensure that it cannot be executed, regardless of its content. + // + // This corresponds to the following contract: + // + // contract CodeDeployer { + // constructor() payable { + // uint256 size; + // assembly { + // mstore8(0, 0xfe) // store invalid opcode at position 0 + // size := sub(codesize(), 32) // size of appended data, as constructor is 32 bytes long + // codecopy(1, 32, size) // copy all appended data to memory at position 1 + // return(0, add(size, 1)) // return appended data (plus the extra byte) for it to be stored as code + // } + // } + // } + // + // More specifically, it is composed of the following opcodes (plus padding, described above): + // + // [1] PUSH1 0xfe + // [3] PUSH1 0x00 + // [4] MSTORE8 + // [6] PUSH1 0x20 + // [7] CODESIZE + // [8] SUB + // [9] DUP1 + // [11] PUSH1 0x20 + // [13] PUSH1 0x01 + // [14] CODECOPY + // [16] PUSH1 0x01 + // [17] ADD + // [19] PUSH1 0x00 + // [20] RETURN + + // solhint-disable max-line-length + bytes32 + private constant _PROTECTED_DEPLOYER_CREATION_CODE = 0x60fe600053602038038060206001396001016000f3fefefefefefefefefefefe; + /** * @dev Deploys a contract with `code` as its code, returning the destination address. + * If preventExecution is set, prepend an invalid opcode to ensure the "contract" cannot be executed. + * Rather than add a flag, we could simply always prepend the opcode, but there might be use cases where fidelity + * is required. * * Reverts if deployment fails. */ - function deploy(bytes memory code) internal returns (address destination) { - bytes32 deployerCreationCode = _DEPLOYER_CREATION_CODE; + function deploy(bytes memory code, bool preventExecution) internal returns (address destination) { + bytes32 deployerCreationCode = preventExecution ? _PROTECTED_DEPLOYER_CREATION_CODE : _DEPLOYER_CREATION_CODE; // We need to concatenate the deployer creation code and `code` in memory, but want to avoid copying all of // `code` (which could be quite long) into a new memory location. Therefore, we operate in-place using diff --git a/pkg/solidity-utils/contracts/test/CodeDeployerFactory.sol b/pkg/solidity-utils/contracts/test/CodeDeployerFactory.sol index fc18a58f57..7d530ce3c5 100644 --- a/pkg/solidity-utils/contracts/test/CodeDeployerFactory.sol +++ b/pkg/solidity-utils/contracts/test/CodeDeployerFactory.sol @@ -19,8 +19,8 @@ import "../helpers/CodeDeployer.sol"; contract CodeDeployerFactory { event CodeDeployed(address destination); - function deploy(bytes memory data) external { - address destination = CodeDeployer.deploy(data); + function deploy(bytes memory data, bool preventExecution) external { + address destination = CodeDeployer.deploy(data, preventExecution); emit CodeDeployed(destination); } } diff --git a/pkg/solidity-utils/test/BaseSplitCodeFactory.test.ts b/pkg/solidity-utils/test/BaseSplitCodeFactory.test.ts index 8662f1d998..a24c8a4fcc 100644 --- a/pkg/solidity-utils/test/BaseSplitCodeFactory.test.ts +++ b/pkg/solidity-utils/test/BaseSplitCodeFactory.test.ts @@ -8,26 +8,40 @@ import { sharedBeforeEach } from '@balancer-labs/v2-common/sharedBeforeEach'; import { ONES_BYTES32, ZERO_BYTES32 } from '@balancer-labs/v2-helpers/src/constants'; import { takeSnapshot } from '@nomicfoundation/hardhat-network-helpers'; import { randomBytes } from 'ethers/lib/utils'; +import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; describe('BasePoolCodeFactory', function () { let factory: Contract; + let admin: SignerWithAddress; const INVALID_ID = '0x0000000000000000000000000000000000000000000000000000000000000000'; const id = '0x0123456789012345678901234567890123456789012345678901234567890123'; + before('setup signers', async () => { + [, admin] = await ethers.getSigners(); + }); + sharedBeforeEach(async () => { factory = await deploy('MockSplitCodeFactory', { args: [] }); }); - it('returns the contract creation code storage addresses', async () => { - const { contractA, contractB } = await factory.getCreationCodeContracts(); + function itReproducesTheCreationCode() { + it('returns the contract creation code storage addresses', async () => { + const { contractA, contractB } = await factory.getCreationCodeContracts(); - const codeA = await ethers.provider.getCode(contractA); - const codeB = await ethers.provider.getCode(contractB); + const codeA = await ethers.provider.getCode(contractA); + const codeB = await ethers.provider.getCode(contractB); - const artifact = getArtifact('MockFactoryCreatedContract'); - expect(codeA.concat(codeB.slice(2))).to.equal(artifact.bytecode); // Slice to remove the '0x' prefix - }); + const artifact = getArtifact('MockFactoryCreatedContract'); + // Slice to remove the '0x' prefix and inserted invalid opcode on code B. + expect(codeA.concat(codeB.slice(4))).to.equal(artifact.bytecode); + + // Code B should have a pre-pending invalid opcode. + expect(codeB.slice(0, 4)).to.eq('0xfe'); + }); + } + + itReproducesTheCreationCode(); it('returns the contract creation code', async () => { const artifact = getArtifact('MockFactoryCreatedContract'); @@ -41,6 +55,28 @@ describe('BasePoolCodeFactory', function () { expectEvent.inReceipt(receipt, 'ContractCreated'); }); + context('half contracts', () => { + it('cannot execute the contract halves', async () => { + const { contractA, contractB } = await factory.getCreationCodeContracts(); + + const txA = { + to: contractA, + value: ethers.utils.parseEther('0.001'), + }; + + const txB = { + to: contractB, + value: ethers.utils.parseEther('0.001'), + }; + + await expect(admin.sendTransaction(txA)).to.be.reverted; + await expect(admin.sendTransaction(txB)).to.be.reverted; + }); + + // And the code is still there after trying + itReproducesTheCreationCode(); + }); + context('when the creation reverts', () => { it('reverts and bubbles up revert reasons', async () => { await expect(factory.create(INVALID_ID, ZERO_BYTES32)).to.be.revertedWith('NON_ZERO_ID'); diff --git a/pkg/solidity-utils/test/CodeDeployer.test.ts b/pkg/solidity-utils/test/CodeDeployer.test.ts index 3c2fc6e073..27914012bb 100644 --- a/pkg/solidity-utils/test/CodeDeployer.test.ts +++ b/pkg/solidity-utils/test/CodeDeployer.test.ts @@ -5,9 +5,15 @@ import { ethers } from 'hardhat'; import { deploy } from '@balancer-labs/v2-helpers/src/contract'; import * as expectEvent from '@balancer-labs/v2-helpers/src/test/expectEvent'; import { sharedBeforeEach } from '@balancer-labs/v2-common/sharedBeforeEach'; +import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; describe('CodeDeployer', function () { let factory: Contract; + let admin: SignerWithAddress; + + before('setup signers', async () => { + [, admin] = await ethers.getSigners(); + }); sharedBeforeEach(async () => { factory = await deploy('CodeDeployerFactory', { args: [] }); @@ -28,16 +34,79 @@ describe('CodeDeployer', function () { context('with code over 24kB long', () => { it('reverts', async () => { const data = `0x${'00'.repeat(24 * 1024 + 1)}`; - await expect(factory.deploy(data)).to.be.revertedWith('CODE_DEPLOYMENT_FAILED'); + await expect(factory.deploy(data, false)).to.be.revertedWith('CODE_DEPLOYMENT_FAILED'); }); }); function itStoresArgumentAsCode(data: string) { it('stores its constructor argument as its code', async () => { - const receipt = await (await factory.deploy(data)).wait(); + const receipt = await (await factory.deploy(data, false)).wait(); const event = expectEvent.inReceipt(receipt, 'CodeDeployed'); expect(await ethers.provider.getCode(event.args.destination)).to.equal(data); }); } + + describe('CodeDeployer protection', () => { + let deployedContract: string; + + context('raw selfdestruct', () => { + // PUSH0 + // SELFDESTRUCT + // STOP (optional - works without this) + const code = '0x5fff00'; + + sharedBeforeEach('deploy contract', async () => { + const receipt = await (await factory.deploy(code, false)).wait(); + const event = expectEvent.inReceipt(receipt, 'CodeDeployed'); + + deployedContract = event.args.destination; + }); + + itStoresArgumentAsCode(code); + + it('self destructs', async () => { + const tx = { + to: deployedContract, + value: ethers.utils.parseEther('0.001'), + }; + + await admin.sendTransaction(tx); + + expect(await ethers.provider.getCode(deployedContract)).to.equal('0x'); + }); + }); + + context('protected selfdestruct', () => { + // INVALID + // PUSH0 + // SELFDESTRUCT + // STOP (optional - works without this) + const code = '0x5fff00'; + const safeCode = '0xfe5fff00'; + + sharedBeforeEach('deploy contract', async () => { + // Pass it the unmodified code + const receipt = await (await factory.deploy(code, true)).wait(); + const event = expectEvent.inReceipt(receipt, 'CodeDeployed'); + + deployedContract = event.args.destination; + }); + + // It should actually store the safecode + itStoresArgumentAsCode(safeCode); + + it('does not self destruct', async () => { + const tx = { + to: deployedContract, + value: ethers.utils.parseEther('0.001'), + }; + + await expect(admin.sendTransaction(tx)).to.be.reverted; + + // Should still have the safeCode + expect(await ethers.provider.getCode(deployedContract)).to.equal(safeCode); + }); + }); + }); });