feat: prevent factories derived from BaseSplitCode from deploying exe…

…cutable bytecode halves (balancer#2549)

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

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

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);
     }
 }

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');

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);
+      });
+    });
+  });
 });