From 3696b7d598f592b233d3d3a86c9899c5ac66dd21 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 11 Jul 2024 00:40:26 +0200 Subject: [PATCH 01/18] Add clone variant with instance parameters stored in 'immutable storage' --- contracts/proxy/Clones.sol | 139 +++++++++++++++++++++++++ test/proxy/Clones.test.js | 202 ++++++++++++++++++++++++------------- 2 files changed, 269 insertions(+), 72 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index d243d67f34b..c19618420a9 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -17,6 +17,8 @@ import {Errors} from "../utils/Errors.sol"; * deterministic method. */ library Clones { + error ImmutableArgsTooLarge(); + /** * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. * @@ -121,4 +123,141 @@ library Clones { ) internal view returns (address predicted) { return predictDeterministicAddress(implementation, salt, address(this)); } + + function cloneWithImmutableArgs(address implementation, bytes memory args) internal returns (address instance) { + return cloneWithImmutableArgs(implementation, args, 0); + } + + function cloneWithImmutableArgs( + address implementation, + bytes memory args, + uint256 value + ) internal returns (address instance) { + if (address(this).balance < value) { + revert Errors.InsufficientBalance(address(this).balance, value); + } + + uint256 extraLength = args.length; + uint256 codeLength = 0x2d + extraLength; + uint256 initLength = 0x38 + extraLength; + if (codeLength > 0xffff) revert ImmutableArgsTooLarge(); + + /// @solidity memory-safe-assembly + assembly { + // [ptr + 0x43] ...................................................................................................................................... // args + // [ptr + 0x23] ......................................................................00000000000000000000000000000000005af43d82803e903d91602b57fd5bf3...... // suffix + // [ptr + 0x14] ........................................000000000000000000000000eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee.................................... // implementation + // [ptr + 0x00] 00000000000000000000003d61000080600a3d3981f3363d3d373d3d3d363d73............................................................................ // prefix + // [ptr + 0x0d] ..........................XX................................................................................................................ // length (part 1) + // [ptr + 0x0e] ............................XX.............................................................................................................. // length (part 2) + let ptr := mload(0x40) + mcopy(add(ptr, 0x43), add(args, 0x20), extraLength) + mstore(add(ptr, 0x23), 0x5af43d82803e903d91602b57fd5bf3) + mstore(add(ptr, 0x14), implementation) + mstore(add(ptr, 0x00), 0x3d61000080600b3d3981f3363d3d373d3d3d363d73) + mstore8(add(ptr, 0x0d), shr(8, codeLength)) + mstore8(add(ptr, 0x0e), shr(0, codeLength)) + instance := create(value, add(ptr, 0x0b), initLength) + } + + if (instance == address(0)) { + revert Errors.FailedDeployment(); + } + } + + function cloneWithImmutableArgsDeterministic( + address implementation, + bytes memory args, + bytes32 salt + ) internal returns (address instance) { + return cloneWithImmutableArgsDeterministic(implementation, args, salt, 0); + } + + function cloneWithImmutableArgsDeterministic( + address implementation, + bytes memory args, + bytes32 salt, + uint256 value + ) internal returns (address instance) { + if (address(this).balance < value) { + revert Errors.InsufficientBalance(address(this).balance, value); + } + + uint256 extraLength = args.length; + uint256 codeLength = 0x2d + extraLength; + uint256 initLength = 0x38 + extraLength; + if (codeLength > 0xffff) revert ImmutableArgsTooLarge(); + + /// @solidity memory-safe-assembly + assembly { + // [ptr + 0x43] ...................................................................................................................................... // args + // [ptr + 0x23] ......................................................................00000000000000000000000000000000005af43d82803e903d91602b57fd5bf3...... // suffix + // [ptr + 0x14] ........................................000000000000000000000000eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee.................................... // implementation + // [ptr + 0x00] 00000000000000000000003d61000080600a3d3981f3363d3d373d3d3d363d73............................................................................ // prefix + // [ptr + 0x0d] ..........................XX................................................................................................................ // length (part 1) + // [ptr + 0x0e] ............................XX.............................................................................................................. // length (part 2) + let ptr := mload(0x40) + mcopy(add(ptr, 0x43), add(args, 0x20), extraLength) + mstore(add(ptr, 0x23), 0x5af43d82803e903d91602b57fd5bf3) + mstore(add(ptr, 0x14), implementation) + mstore(add(ptr, 0x00), 0x3d61000080600b3d3981f3363d3d373d3d3d363d73) + mstore8(add(ptr, 0x0d), shr(8, codeLength)) + mstore8(add(ptr, 0x0e), shr(0, codeLength)) + instance := create2(value, add(ptr, 0x0b), initLength, salt) + } + if (instance == address(0)) { + revert Errors.FailedDeployment(); + } + } + + function predictWithImmutableArgsDeterministicAddress( + address implementation, + bytes memory args, + bytes32 salt, + address deployer + ) internal pure returns (address predicted) { + uint256 extraLength = args.length; + uint256 codeLength = 0x2d + extraLength; + uint256 initLength = 0x38 + extraLength; + if (codeLength > 0xffff) revert ImmutableArgsTooLarge(); + + /// @solidity memory-safe-assembly + assembly { + let ptr := mload(0x40) + mstore(add(ptr, add(0x58, extraLength)), salt) + mstore(add(ptr, add(0x38, extraLength)), deployer) + mstore8(add(ptr, add(0x43, extraLength)), 0xff) + mcopy(add(ptr, 0x43), add(args, 0x20), extraLength) + mstore(add(ptr, 0x23), 0x5af43d82803e903d91602b57fd5bf3) + mstore(add(ptr, 0x14), implementation) + mstore(add(ptr, 0x00), 0x3d61000080600b3d3981f3363d3d373d3d3d363d73) + mstore8(add(ptr, 0x0d), shr(8, codeLength)) + mstore8(add(ptr, 0x0e), shr(0, codeLength)) + mstore(add(ptr, add(0x78, extraLength)), keccak256(add(ptr, 0x0b), initLength)) + predicted := and( + keccak256(add(ptr, add(0x43, extraLength)), 0x55), + 0xffffffffffffffffffffffffffffffffffffffff + ) + } + } + + function predictWithImmutableArgsDeterministicAddress( + address implementation, + bytes memory args, + bytes32 salt + ) internal view returns (address predicted) { + return predictWithImmutableArgsDeterministicAddress(implementation, args, salt, address(this)); + } + + function fetchCloneArgs(address instance) internal view returns (bytes memory result) { + uint256 argsLength = instance.code.length - 0x2d; // revert if length is too short + assembly { + // reserve space + result := mload(0x40) + mstore(0x40, add(result, add(0x20, argsLength))) + // load + mstore(result, argsLength) + extcodecopy(instance, add(result, 0x20), 0x2d, argsLength) + } + } } diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 70220fbf7a0..6ad7d55ccae 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -10,30 +10,47 @@ async function fixture() { const factory = await ethers.deployContract('$Clones'); const implementation = await ethers.deployContract('DummyImplementation'); - const newClone = async (opts = {}) => { - const clone = await factory.$clone.staticCall(implementation).then(address => implementation.attach(address)); - const tx = await (opts.deployValue - ? factory.$clone(implementation, ethers.Typed.uint256(opts.deployValue)) - : factory.$clone(implementation)); - if (opts.initData || opts.initValue) { - await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); - } - return Object.assign(clone, { deploymentTransaction: () => tx }); - }; - - const newCloneDeterministic = async (opts = {}) => { - const salt = opts.salt ?? ethers.randomBytes(32); - const clone = await factory.$cloneDeterministic - .staticCall(implementation, salt) - .then(address => implementation.attach(address)); - const tx = await (opts.deployValue - ? factory.$cloneDeterministic(implementation, salt, ethers.Typed.uint256(opts.deployValue)) - : factory.$cloneDeterministic(implementation, salt)); - if (opts.initData || opts.initValue) { - await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); - } - return Object.assign(clone, { deploymentTransaction: () => tx }); - }; + const newClone = + args => + async (opts = {}) => { + const clone = await factory.$clone.staticCall(implementation).then(address => implementation.attach(address)); + const tx = await (opts.deployValue + ? args + ? factory.$cloneWithImmutableArgs(implementation, args, ethers.Typed.uint256(opts.deployValue)) + : factory.$clone(implementation, ethers.Typed.uint256(opts.deployValue)) + : args + ? factory.$cloneWithImmutableArgs(implementation, args) + : factory.$clone(implementation)); + if (opts.initData || opts.initValue) { + await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); + } + return Object.assign(clone, { deploymentTransaction: () => tx }); + }; + + const newCloneDeterministic = + args => + async (opts = {}) => { + const salt = opts.salt ?? ethers.randomBytes(32); + const clone = await factory.$cloneDeterministic + .staticCall(implementation, salt) + .then(address => implementation.attach(address)); + const tx = await (opts.deployValue + ? args + ? factory.$cloneWithImmutableArgsDeterministic( + implementation, + args, + salt, + ethers.Typed.uint256(opts.deployValue), + ) + : factory.$cloneDeterministic(implementation, salt, ethers.Typed.uint256(opts.deployValue)) + : args + ? factory.$cloneWithImmutableArgsDeterministic(implementation, args, salt) + : factory.$cloneDeterministic(implementation, salt)); + if (opts.initData || opts.initValue) { + await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); + } + return Object.assign(clone, { deploymentTransaction: () => tx }); + }; return { deployer, factory, implementation, newClone, newCloneDeterministic }; } @@ -43,53 +60,94 @@ describe('Clones', function () { Object.assign(this, await loadFixture(fixture)); }); - describe('clone', function () { - beforeEach(async function () { - this.createClone = this.newClone; + for (const args of [undefined, '0x', '0x11223344']) { + describe(args ? `with immutable args: ${args}` : 'without immutable args', function () { + describe('clone', function () { + beforeEach(async function () { + this.createClone = this.newClone(args); + }); + + shouldBehaveLikeClone(); + + it('get immutable arguments', async function () { + const instance = await this.createClone(); + expect(await this.factory.$fetchCloneArgs(instance)).to.equal(args ?? '0x'); + }); + }); + + describe('cloneDeterministic', function () { + beforeEach(async function () { + this.createClone = this.newCloneDeterministic(undefined); + }); + + shouldBehaveLikeClone(); + + it('revert if address already used', async function () { + const salt = ethers.randomBytes(32); + + const deployClone = () => + args + ? this.factory.$cloneWithImmutableArgsDeterministic(this.implementation, args, salt) + : this.factory.$cloneDeterministic(this.implementation, salt); + + // deploy once + await expect(deployClone()).to.not.be.reverted; + + // deploy twice + await expect(deployClone()).to.be.revertedWithCustomError(this.factory, 'FailedDeployment'); + }); + + it('address prediction', async function () { + const salt = ethers.randomBytes(32); + + if (args) { + const expected = ethers.getCreate2Address( + this.factory.target, + salt, + ethers.keccak256( + ethers.concat([ + '0x3d61', + ethers.toBeHex(0x2d + ethers.getBytes(args).length, 2), + '0x80600b3d3981f3363d3d373d3d3d363d73', + this.implementation.target, + '0x5af43d82803e903d91602b57fd5bf3', + args, + ]), + ), + ); + + const predicted = await this.factory.$predictWithImmutableArgsDeterministicAddress( + this.implementation, + args, + salt, + ); + expect(predicted).to.equal(expected); + + await expect(this.factory.$cloneWithImmutableArgsDeterministic(this.implementation, args, salt)) + .to.emit(this.factory, 'return$cloneWithImmutableArgsDeterministic_address_bytes_bytes32') + .withArgs(predicted); + } else { + const expected = ethers.getCreate2Address( + this.factory.target, + salt, + ethers.keccak256( + ethers.concat([ + '0x3d602d80600a3d3981f3363d3d373d3d3d363d73', + this.implementation.target, + '0x5af43d82803e903d91602b57fd5bf3', + ]), + ), + ); + + const predicted = await this.factory.$predictDeterministicAddress(this.implementation, salt); + expect(predicted).to.equal(expected); + + await expect(this.factory.$cloneDeterministic(this.implementation, salt)) + .to.emit(this.factory, 'return$cloneDeterministic_address_bytes32') + .withArgs(predicted); + } + }); + }); }); - - shouldBehaveLikeClone(); - }); - - describe('cloneDeterministic', function () { - beforeEach(async function () { - this.createClone = this.newCloneDeterministic; - }); - - shouldBehaveLikeClone(); - - it('revert if address already used', async function () { - const salt = ethers.randomBytes(32); - - // deploy once - await expect(this.factory.$cloneDeterministic(this.implementation, salt)).to.emit( - this.factory, - 'return$cloneDeterministic_address_bytes32', - ); - - // deploy twice - await expect(this.factory.$cloneDeterministic(this.implementation, salt)).to.be.revertedWithCustomError( - this.factory, - 'FailedDeployment', - ); - }); - - it('address prediction', async function () { - const salt = ethers.randomBytes(32); - - const creationCode = ethers.concat([ - '0x3d602d80600a3d3981f3363d3d373d3d3d363d73', - this.implementation.target, - '0x5af43d82803e903d91602b57fd5bf3', - ]); - - const predicted = await this.factory.$predictDeterministicAddress(this.implementation, salt); - const expected = ethers.getCreate2Address(this.factory.target, salt, ethers.keccak256(creationCode)); - expect(predicted).to.equal(expected); - - await expect(this.factory.$cloneDeterministic(this.implementation, salt)) - .to.emit(this.factory, 'return$cloneDeterministic_address_bytes32') - .withArgs(predicted); - }); - }); + } }); From 05a30429404ab5a558d6161e62936fa99de0e910 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 15 Jul 2024 11:15:51 +0200 Subject: [PATCH 02/18] update from 4337 branch --- contracts/proxy/Clones.sol | 152 +++++++++++++++++++------------------ 1 file changed, 77 insertions(+), 75 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index c19618420a9..661811d3fc0 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.20; +import {Create2} from "../utils/Create2.sol"; import {Errors} from "../utils/Errors.sol"; /** @@ -124,10 +125,23 @@ library Clones { return predictDeterministicAddress(implementation, salt, address(this)); } + /** + * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`, with `args` + * attached to it as immutable arguments (that can be fetched using {fetchCloneArgs}). + * + * This function uses the create opcode, which should never revert. + */ function cloneWithImmutableArgs(address implementation, bytes memory args) internal returns (address instance) { return cloneWithImmutableArgs(implementation, args, 0); } + /** + * @dev Same as {xref-Clones-cloneWithImmutableArgs-address-bytes-}[cloneWithImmutableArgs], but with a `value` + * parameter to send native currency to the new contract. + * + * NOTE: Using a non-zero value at creation will require the contract using this function (e.g. a factory) + * to always have enough balance for new deployments. Consider exposing this function under a payable method. + */ function cloneWithImmutableArgs( address implementation, bytes memory args, @@ -136,35 +150,23 @@ library Clones { if (address(this).balance < value) { revert Errors.InsufficientBalance(address(this).balance, value); } - - uint256 extraLength = args.length; - uint256 codeLength = 0x2d + extraLength; - uint256 initLength = 0x38 + extraLength; - if (codeLength > 0xffff) revert ImmutableArgsTooLarge(); - - /// @solidity memory-safe-assembly - assembly { - // [ptr + 0x43] ...................................................................................................................................... // args - // [ptr + 0x23] ......................................................................00000000000000000000000000000000005af43d82803e903d91602b57fd5bf3...... // suffix - // [ptr + 0x14] ........................................000000000000000000000000eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee.................................... // implementation - // [ptr + 0x00] 00000000000000000000003d61000080600a3d3981f3363d3d373d3d3d363d73............................................................................ // prefix - // [ptr + 0x0d] ..........................XX................................................................................................................ // length (part 1) - // [ptr + 0x0e] ............................XX.............................................................................................................. // length (part 2) - let ptr := mload(0x40) - mcopy(add(ptr, 0x43), add(args, 0x20), extraLength) - mstore(add(ptr, 0x23), 0x5af43d82803e903d91602b57fd5bf3) - mstore(add(ptr, 0x14), implementation) - mstore(add(ptr, 0x00), 0x3d61000080600b3d3981f3363d3d373d3d3d363d73) - mstore8(add(ptr, 0x0d), shr(8, codeLength)) - mstore8(add(ptr, 0x0e), shr(0, codeLength)) - instance := create(value, add(ptr, 0x0b), initLength) + bytes memory bytecode = _cloneWithImmutableArgsCode(implementation, args); + assembly ("memory-safe") { + instance := create(value, add(bytecode, 0x20), mload(bytecode)) } - if (instance == address(0)) { revert Errors.FailedDeployment(); } } + /** + * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`, with `args` + * attached to it as immutable arguments (that can be fetched using {fetchCloneArgs}). + * + * This function uses the create2 opcode and a `salt` to deterministically deploy the clone. Using the same + * `implementation` and `salt` multiple time will revert, since the clones cannot be deployed twice at the same + * address. + */ function cloneWithImmutableArgsDeterministic( address implementation, bytes memory args, @@ -173,74 +175,39 @@ library Clones { return cloneWithImmutableArgsDeterministic(implementation, args, salt, 0); } + /** + * @dev Same as {xref-Clones-cloneWithImmutableArgsDeterministic-address-bytes-bytes32-}[cloneWithImmutableArgsDeterministic], + * but with a `value` parameter to send native currency to the new contract. + * + * NOTE: Using a non-zero value at creation will require the contract using this function (e.g. a factory) + * to always have enough balance for new deployments. Consider exposing this function under a payable method. + */ function cloneWithImmutableArgsDeterministic( address implementation, bytes memory args, bytes32 salt, uint256 value ) internal returns (address instance) { - if (address(this).balance < value) { - revert Errors.InsufficientBalance(address(this).balance, value); - } - - uint256 extraLength = args.length; - uint256 codeLength = 0x2d + extraLength; - uint256 initLength = 0x38 + extraLength; - if (codeLength > 0xffff) revert ImmutableArgsTooLarge(); - - /// @solidity memory-safe-assembly - assembly { - // [ptr + 0x43] ...................................................................................................................................... // args - // [ptr + 0x23] ......................................................................00000000000000000000000000000000005af43d82803e903d91602b57fd5bf3...... // suffix - // [ptr + 0x14] ........................................000000000000000000000000eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee.................................... // implementation - // [ptr + 0x00] 00000000000000000000003d61000080600a3d3981f3363d3d373d3d3d363d73............................................................................ // prefix - // [ptr + 0x0d] ..........................XX................................................................................................................ // length (part 1) - // [ptr + 0x0e] ............................XX.............................................................................................................. // length (part 2) - let ptr := mload(0x40) - mcopy(add(ptr, 0x43), add(args, 0x20), extraLength) - mstore(add(ptr, 0x23), 0x5af43d82803e903d91602b57fd5bf3) - mstore(add(ptr, 0x14), implementation) - mstore(add(ptr, 0x00), 0x3d61000080600b3d3981f3363d3d373d3d3d363d73) - mstore8(add(ptr, 0x0d), shr(8, codeLength)) - mstore8(add(ptr, 0x0e), shr(0, codeLength)) - instance := create2(value, add(ptr, 0x0b), initLength, salt) - } - if (instance == address(0)) { - revert Errors.FailedDeployment(); - } + bytes memory bytecode = _cloneWithImmutableArgsCode(implementation, args); + return Create2.deploy(value, salt, bytecode); } + /** + * @dev Computes the address of a clone deployed using {Clones-cloneWithImmutableArgsDeterministic}. + */ function predictWithImmutableArgsDeterministicAddress( address implementation, bytes memory args, bytes32 salt, address deployer ) internal pure returns (address predicted) { - uint256 extraLength = args.length; - uint256 codeLength = 0x2d + extraLength; - uint256 initLength = 0x38 + extraLength; - if (codeLength > 0xffff) revert ImmutableArgsTooLarge(); - - /// @solidity memory-safe-assembly - assembly { - let ptr := mload(0x40) - mstore(add(ptr, add(0x58, extraLength)), salt) - mstore(add(ptr, add(0x38, extraLength)), deployer) - mstore8(add(ptr, add(0x43, extraLength)), 0xff) - mcopy(add(ptr, 0x43), add(args, 0x20), extraLength) - mstore(add(ptr, 0x23), 0x5af43d82803e903d91602b57fd5bf3) - mstore(add(ptr, 0x14), implementation) - mstore(add(ptr, 0x00), 0x3d61000080600b3d3981f3363d3d373d3d3d363d73) - mstore8(add(ptr, 0x0d), shr(8, codeLength)) - mstore8(add(ptr, 0x0e), shr(0, codeLength)) - mstore(add(ptr, add(0x78, extraLength)), keccak256(add(ptr, 0x0b), initLength)) - predicted := and( - keccak256(add(ptr, add(0x43, extraLength)), 0x55), - 0xffffffffffffffffffffffffffffffffffffffff - ) - } + bytes memory bytecode = _cloneWithImmutableArgsCode(implementation, args); + return Create2.computeAddress(salt, keccak256(bytecode), deployer); } + /** + * @dev Computes the address of a clone deployed using {Clones-cloneWithImmutableArgsDeterministic}. + */ function predictWithImmutableArgsDeterministicAddress( address implementation, bytes memory args, @@ -249,6 +216,17 @@ library Clones { return predictWithImmutableArgsDeterministicAddress(implementation, args, salt, address(this)); } + /** + * @dev Get the immutable args attached to a clone. + * + * - If `instance` is a clone that was deployed using `clone` or `cloneDeterministic`, this + * function will return an empty array. + * - If `instance` is a clone that was deployed using `cloneWithImmutableArgs` or + * `cloneWithImmutableArgsDeterministic`, this function will return the args array used at + * creation. + * - If `instance` is NOT a clone deployed using this library, the behavior is undefined. This + * function should only be used to check addresses that are known to be clones. + */ function fetchCloneArgs(address instance) internal view returns (bytes memory result) { uint256 argsLength = instance.code.length - 0x2d; // revert if length is too short assembly { @@ -260,4 +238,28 @@ library Clones { extcodecopy(instance, add(result, 0x20), 0x2d, argsLength) } } + + /** + * @dev Helper that prepares the initcode of the proxy with immutable args. + * + * An assembly variant of this function requires copying the `args` array, which can be efficiently done using + * `mcopy`. Unfortunatelly, that opcode is not available before cancun. A pure solidity implemenation using + * abi.encodePacked is more expensive but also more portable and easier to review. + */ + function _cloneWithImmutableArgsCode( + address implementation, + bytes memory args + ) private pure returns (bytes memory) { + uint256 initCodeLength = args.length + 0x2d; + if (initCodeLength > type(uint16).max) revert ImmutableArgsTooLarge(); + return + abi.encodePacked( + hex"3d61", + uint16(initCodeLength), + hex"80600b3d3981f3363d3d373d3d3d363d73", + implementation, + hex"5af43d82803e903d91602b57fd5bf3", + args + ); + } } From 51f98596ec8c54fe047c13a16d126b5ffeddd1bb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 16 Jul 2024 16:46:24 +0200 Subject: [PATCH 03/18] optimisation & fuzzing --- contracts/proxy/Clones.sol | 4 ++-- test/proxy/Clones.t.sol | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 661811d3fc0..de222a2ae98 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -254,9 +254,9 @@ library Clones { if (initCodeLength > type(uint16).max) revert ImmutableArgsTooLarge(); return abi.encodePacked( - hex"3d61", + hex"61", uint16(initCodeLength), - hex"80600b3d3981f3363d3d373d3d3d363d73", + hex"3d81600a3d39f3363d3d373d3d3d363d73", implementation, hex"5af43d82803e903d91602b57fd5bf3", args diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index 4301e103c26..9fd97981c5a 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -20,12 +20,28 @@ contract ClonesTest is Test { assertEq(spillage, bytes32(0)); } + function testSymbolicPredictWithImmutableArgsDeterministicAddressSpillage( + address implementation, + bytes32 salt, + bytes memory args + ) public { + vm.assume(args.length < 0xffd3); + + address predicted = Clones.predictWithImmutableArgsDeterministicAddress(implementation, args, salt); + bytes32 spillage; + /// @solidity memory-safe-assembly + assembly { + spillage := and(predicted, 0xffffffffffffffffffffffff0000000000000000000000000000000000000000) + } + assertEq(spillage, bytes32(0)); + } + function testCloneDirty() external { address cloneClean = Clones.clone(address(this)); address cloneDirty = Clones.clone(_dirty(address(this))); // both clones have the same code - assertEq(keccak256(cloneClean.code), keccak256(cloneDirty.code)); + assertEq(cloneClean.code, cloneDirty.code); // both clones behave as expected assertEq(ClonesTest(cloneClean).getNumber(), this.getNumber()); @@ -37,7 +53,7 @@ contract ClonesTest is Test { address cloneDirty = Clones.cloneDeterministic(_dirty(address(this)), ~salt); // both clones have the same code - assertEq(keccak256(cloneClean.code), keccak256(cloneDirty.code)); + assertEq(cloneClean.code, cloneDirty.code); // both clones behave as expected assertEq(ClonesTest(cloneClean).getNumber(), this.getNumber()); @@ -52,6 +68,22 @@ contract ClonesTest is Test { assertEq(predictClean, predictDirty); } + function testFetchCloneArgs(bytes memory args, bytes32 salt) external { + vm.assume(args.length < 0xffd3); + + address instance1 = Clones.cloneWithImmutableArgs(address(this), args); + address instance2 = Clones.cloneWithImmutableArgsDeterministic(address(this), args, salt); + + // both clones have the same code + assertEq(instance1.code, instance2.code); + + // both clones behave as expected and args can be fetched + assertEq(ClonesTest(instance1).getNumber(), this.getNumber()); + assertEq(ClonesTest(instance2).getNumber(), this.getNumber()); + assertEq(Clones.fetchCloneArgs(instance1), args); + assertEq(Clones.fetchCloneArgs(instance2), args); + } + function _dirty(address input) private pure returns (address output) { assembly ("memory-safe") { output := or(input, shl(160, not(0))) From 95c5a3772b971c375694dde83435b9d0c1c46a09 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 16 Jul 2024 17:10:58 +0200 Subject: [PATCH 04/18] fix tests --- test/proxy/Clones.test.js | 88 ++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 6ad7d55ccae..f6c2a71ff08 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -4,6 +4,22 @@ const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const shouldBehaveLikeClone = require('./Clones.behaviour'); +const cloneInitCode = (instance, args = undefined) => + args + ? ethers.concat([ + '0x61', + ethers.toBeHex(0x2d + ethers.getBytes(args).length, 2), + '0x3d81600a3d39f3363d3d373d3d3d363d73', + instance.target ?? instance.address ?? instance, + '0x5af43d82803e903d91602b57fd5bf3', + args, + ]) + : ethers.concat([ + '0x3d602d80600a3d3981f3363d3d373d3d3d363d73', + instance.target ?? instance.address ?? instance, + '0x5af43d82803e903d91602b57fd5bf3', + ]); + async function fixture() { const [deployer] = await ethers.getSigners(); @@ -13,13 +29,16 @@ async function fixture() { const newClone = args => async (opts = {}) => { - const clone = await factory.$clone.staticCall(implementation).then(address => implementation.attach(address)); - const tx = await (opts.deployValue - ? args + const clone = await (args + ? factory.$cloneWithImmutableArgs.staticCall(implementation, args) + : factory.$clone.staticCall(implementation) + ).then(address => implementation.attach(address)); + const tx = await (args + ? opts.deployValue ? factory.$cloneWithImmutableArgs(implementation, args, ethers.Typed.uint256(opts.deployValue)) - : factory.$clone(implementation, ethers.Typed.uint256(opts.deployValue)) - : args - ? factory.$cloneWithImmutableArgs(implementation, args) + : factory.$cloneWithImmutableArgs(implementation, args) + : opts.deployValue + ? factory.$clone(implementation, ethers.Typed.uint256(opts.deployValue)) : factory.$clone(implementation)); if (opts.initData || opts.initValue) { await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); @@ -31,20 +50,21 @@ async function fixture() { args => async (opts = {}) => { const salt = opts.salt ?? ethers.randomBytes(32); - const clone = await factory.$cloneDeterministic - .staticCall(implementation, salt) - .then(address => implementation.attach(address)); - const tx = await (opts.deployValue - ? args + const clone = await (args + ? factory.$cloneWithImmutableArgsDeterministic.staticCall(implementation, args, salt) + : factory.$cloneDeterministic.staticCall(implementation, salt) + ).then(address => implementation.attach(address)); + const tx = await (args + ? opts.deployValue ? factory.$cloneWithImmutableArgsDeterministic( implementation, args, salt, ethers.Typed.uint256(opts.deployValue), ) - : factory.$cloneDeterministic(implementation, salt, ethers.Typed.uint256(opts.deployValue)) - : args - ? factory.$cloneWithImmutableArgsDeterministic(implementation, args, salt) + : factory.$cloneWithImmutableArgsDeterministic(implementation, args, salt) + : opts.deployValue + ? factory.$cloneDeterministic(implementation, salt, ethers.Typed.uint256(opts.deployValue)) : factory.$cloneDeterministic(implementation, salt)); if (opts.initData || opts.initValue) { await deployer.sendTransaction({ to: clone, value: opts.initValue ?? 0n, data: opts.initData ?? '0x' }); @@ -77,11 +97,16 @@ describe('Clones', function () { describe('cloneDeterministic', function () { beforeEach(async function () { - this.createClone = this.newCloneDeterministic(undefined); + this.createClone = this.newCloneDeterministic(args); }); shouldBehaveLikeClone(); + it('get immutable arguments', async function () { + const instance = await this.createClone(); + expect(await this.factory.$fetchCloneArgs(instance)).to.equal(args ?? '0x'); + }); + it('revert if address already used', async function () { const salt = ethers.randomBytes(32); @@ -100,22 +125,13 @@ describe('Clones', function () { it('address prediction', async function () { const salt = ethers.randomBytes(32); - if (args) { - const expected = ethers.getCreate2Address( - this.factory.target, - salt, - ethers.keccak256( - ethers.concat([ - '0x3d61', - ethers.toBeHex(0x2d + ethers.getBytes(args).length, 2), - '0x80600b3d3981f3363d3d373d3d3d363d73', - this.implementation.target, - '0x5af43d82803e903d91602b57fd5bf3', - args, - ]), - ), - ); + const expected = ethers.getCreate2Address( + this.factory.target, + salt, + ethers.keccak256(cloneInitCode(this.implementation, args)), + ); + if (args) { const predicted = await this.factory.$predictWithImmutableArgsDeterministicAddress( this.implementation, args, @@ -127,18 +143,6 @@ describe('Clones', function () { .to.emit(this.factory, 'return$cloneWithImmutableArgsDeterministic_address_bytes_bytes32') .withArgs(predicted); } else { - const expected = ethers.getCreate2Address( - this.factory.target, - salt, - ethers.keccak256( - ethers.concat([ - '0x3d602d80600a3d3981f3363d3d373d3d3d363d73', - this.implementation.target, - '0x5af43d82803e903d91602b57fd5bf3', - ]), - ), - ); - const predicted = await this.factory.$predictDeterministicAddress(this.implementation, salt); expect(predicted).to.equal(expected); From 0cc071fef2066bcd176f622446c566062d38db51 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jul 2024 13:03:01 +0200 Subject: [PATCH 05/18] Apply suggestions from code review --- contracts/proxy/Clones.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index de222a2ae98..3d64f5a69df 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -243,7 +243,7 @@ library Clones { * @dev Helper that prepares the initcode of the proxy with immutable args. * * An assembly variant of this function requires copying the `args` array, which can be efficiently done using - * `mcopy`. Unfortunatelly, that opcode is not available before cancun. A pure solidity implemenation using + * `mcopy`. Unfortunately, that opcode is not available before cancun. A pure solidity implementation using * abi.encodePacked is more expensive but also more portable and easier to review. */ function _cloneWithImmutableArgsCode( From 548669aa98f0c97d2e1714b284ad8a182276a8ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Mon, 26 Aug 2024 18:08:39 -0600 Subject: [PATCH 06/18] Apply review suggestions --- contracts/proxy/Clones.sol | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 3d64f5a69df..32660c88576 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -126,8 +126,9 @@ library Clones { } /** - * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`, with `args` - * attached to it as immutable arguments (that can be fetched using {fetchCloneArgs}). + * @dev Deploys and returns the address of a clone that mimics the behavior of `implementation` with custom + * immutable arguments. These are provided through `args` and cannot be changed after deployment. To + * access the arguments within the implementation, use {fetchCloneArgs}. * * This function uses the create opcode, which should never revert. */ @@ -160,8 +161,9 @@ library Clones { } /** - * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`, with `args` - * attached to it as immutable arguments (that can be fetched using {fetchCloneArgs}). + * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation` with custom + * immutable arguments. These are provided through `args` and cannot be changed after deployment. To + * access the arguments within the implementation, use {fetchCloneArgs}. * * This function uses the create2 opcode and a `salt` to deterministically deploy the clone. Using the same * `implementation` and `salt` multiple time will revert, since the clones cannot be deployed twice at the same @@ -229,12 +231,8 @@ library Clones { */ function fetchCloneArgs(address instance) internal view returns (bytes memory result) { uint256 argsLength = instance.code.length - 0x2d; // revert if length is too short - assembly { - // reserve space - result := mload(0x40) - mstore(0x40, add(result, add(0x20, argsLength))) - // load - mstore(result, argsLength) + result = new bytes(argsLength); + assembly ("memory-safe") { extcodecopy(instance, add(result, 0x20), 0x2d, argsLength) } } From fdb585bec45229e22dcd3330ce659275bdf372c2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 27 Aug 2024 15:35:57 +0200 Subject: [PATCH 07/18] check EIP-3860 limitation --- contracts/proxy/Clones.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 32660c88576..69fcd75af89 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -249,7 +249,8 @@ library Clones { bytes memory args ) private pure returns (bytes memory) { uint256 initCodeLength = args.length + 0x2d; - if (initCodeLength > type(uint16).max) revert ImmutableArgsTooLarge(); + // initcode is limited to 49152 bytes as per https://eips.ethereum.org/EIPS/eip-3860[EIP-3860] + if (initCodeLength > 0xc000) revert ImmutableArgsTooLarge(); return abi.encodePacked( hex"61", From 2dd6cd0a1f3181b117769f400a7490dea24020b0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 27 Aug 2024 15:41:28 +0200 Subject: [PATCH 08/18] up --- contracts/proxy/Clones.sol | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 69fcd75af89..bda7b48af4f 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -229,12 +229,12 @@ library Clones { * - If `instance` is NOT a clone deployed using this library, the behavior is undefined. This * function should only be used to check addresses that are known to be clones. */ - function fetchCloneArgs(address instance) internal view returns (bytes memory result) { - uint256 argsLength = instance.code.length - 0x2d; // revert if length is too short - result = new bytes(argsLength); + function fetchCloneArgs(address instance) internal view returns (bytes memory) { + bytes memory result = new bytes(instance.code.length - 0x2d); // revert if length is too short assembly ("memory-safe") { - extcodecopy(instance, add(result, 0x20), 0x2d, argsLength) + extcodecopy(instance, add(result, 0x20), 0x2d, mload(result)) } + return result; } /** @@ -243,18 +243,19 @@ library Clones { * An assembly variant of this function requires copying the `args` array, which can be efficiently done using * `mcopy`. Unfortunately, that opcode is not available before cancun. A pure solidity implementation using * abi.encodePacked is more expensive but also more portable and easier to review. + * + * NOTE: https://eips.ethereum.org/EIPS/eip-3860[EIP-3860] limits the length of the `initcode` to 49152 bytes. + * With the proxy code taking 45 bytes, that limits the length of the immutable args to 49107 bytes. */ function _cloneWithImmutableArgsCode( address implementation, bytes memory args ) private pure returns (bytes memory) { - uint256 initCodeLength = args.length + 0x2d; - // initcode is limited to 49152 bytes as per https://eips.ethereum.org/EIPS/eip-3860[EIP-3860] - if (initCodeLength > 0xc000) revert ImmutableArgsTooLarge(); + if (args.length > 49107) revert ImmutableArgsTooLarge(); return abi.encodePacked( hex"61", - uint16(initCodeLength), + uint16(args.length + 0x2d), hex"3d81600a3d39f3363d3d373d3d3d363d73", implementation, hex"5af43d82803e903d91602b57fd5bf3", From 45cdd11b08ed149ddaffb9e30524c7d34539ae7a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Aug 2024 13:11:11 +0200 Subject: [PATCH 09/18] rename --- contracts/proxy/Clones.sol | 28 ++++++++++++++-------------- test/proxy/Clones.t.sol | 6 +++--- test/proxy/Clones.test.js | 14 +++++++------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index bda7b48af4f..bd71f1dd05c 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -151,7 +151,7 @@ library Clones { if (address(this).balance < value) { revert Errors.InsufficientBalance(address(this).balance, value); } - bytes memory bytecode = _cloneWithImmutableArgsCode(implementation, args); + bytes memory bytecode = _cloneCodeWithImmutableArgs(implementation, args); assembly ("memory-safe") { instance := create(value, add(bytecode, 0x20), mload(bytecode)) } @@ -169,53 +169,53 @@ library Clones { * `implementation` and `salt` multiple time will revert, since the clones cannot be deployed twice at the same * address. */ - function cloneWithImmutableArgsDeterministic( + function cloneDeterministicWithImmutableArgs( address implementation, bytes memory args, bytes32 salt ) internal returns (address instance) { - return cloneWithImmutableArgsDeterministic(implementation, args, salt, 0); + return cloneDeterministicWithImmutableArgs(implementation, args, salt, 0); } /** - * @dev Same as {xref-Clones-cloneWithImmutableArgsDeterministic-address-bytes-bytes32-}[cloneWithImmutableArgsDeterministic], + * @dev Same as {xref-Clones-cloneDeterministicWithImmutableArgs-address-bytes-bytes32-}[cloneDeterministicWithImmutableArgs], * but with a `value` parameter to send native currency to the new contract. * * NOTE: Using a non-zero value at creation will require the contract using this function (e.g. a factory) * to always have enough balance for new deployments. Consider exposing this function under a payable method. */ - function cloneWithImmutableArgsDeterministic( + function cloneDeterministicWithImmutableArgs( address implementation, bytes memory args, bytes32 salt, uint256 value ) internal returns (address instance) { - bytes memory bytecode = _cloneWithImmutableArgsCode(implementation, args); + bytes memory bytecode = _cloneCodeWithImmutableArgs(implementation, args); return Create2.deploy(value, salt, bytecode); } /** - * @dev Computes the address of a clone deployed using {Clones-cloneWithImmutableArgsDeterministic}. + * @dev Computes the address of a clone deployed using {Clones-cloneDeterministicWithImmutableArgs}. */ - function predictWithImmutableArgsDeterministicAddress( + function predictDeterministicAddressWithImmutableArgs( address implementation, bytes memory args, bytes32 salt, address deployer ) internal pure returns (address predicted) { - bytes memory bytecode = _cloneWithImmutableArgsCode(implementation, args); + bytes memory bytecode = _cloneCodeWithImmutableArgs(implementation, args); return Create2.computeAddress(salt, keccak256(bytecode), deployer); } /** - * @dev Computes the address of a clone deployed using {Clones-cloneWithImmutableArgsDeterministic}. + * @dev Computes the address of a clone deployed using {Clones-cloneDeterministicWithImmutableArgs}. */ - function predictWithImmutableArgsDeterministicAddress( + function predictDeterministicAddressWithImmutableArgs( address implementation, bytes memory args, bytes32 salt ) internal view returns (address predicted) { - return predictWithImmutableArgsDeterministicAddress(implementation, args, salt, address(this)); + return predictDeterministicAddressWithImmutableArgs(implementation, args, salt, address(this)); } /** @@ -224,7 +224,7 @@ library Clones { * - If `instance` is a clone that was deployed using `clone` or `cloneDeterministic`, this * function will return an empty array. * - If `instance` is a clone that was deployed using `cloneWithImmutableArgs` or - * `cloneWithImmutableArgsDeterministic`, this function will return the args array used at + * `cloneDeterministicWithImmutableArgs`, this function will return the args array used at * creation. * - If `instance` is NOT a clone deployed using this library, the behavior is undefined. This * function should only be used to check addresses that are known to be clones. @@ -247,7 +247,7 @@ library Clones { * NOTE: https://eips.ethereum.org/EIPS/eip-3860[EIP-3860] limits the length of the `initcode` to 49152 bytes. * With the proxy code taking 45 bytes, that limits the length of the immutable args to 49107 bytes. */ - function _cloneWithImmutableArgsCode( + function _cloneCodeWithImmutableArgs( address implementation, bytes memory args ) private pure returns (bytes memory) { diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index 9fd97981c5a..20257ee946f 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -20,14 +20,14 @@ contract ClonesTest is Test { assertEq(spillage, bytes32(0)); } - function testSymbolicPredictWithImmutableArgsDeterministicAddressSpillage( + function testSymbolicPredictDeterministicAddressWithImmutableArgsSpillage( address implementation, bytes32 salt, bytes memory args ) public { vm.assume(args.length < 0xffd3); - address predicted = Clones.predictWithImmutableArgsDeterministicAddress(implementation, args, salt); + address predicted = Clones.predictDeterministicAddressWithImmutableArgs(implementation, args, salt); bytes32 spillage; /// @solidity memory-safe-assembly assembly { @@ -72,7 +72,7 @@ contract ClonesTest is Test { vm.assume(args.length < 0xffd3); address instance1 = Clones.cloneWithImmutableArgs(address(this), args); - address instance2 = Clones.cloneWithImmutableArgsDeterministic(address(this), args, salt); + address instance2 = Clones.cloneDeterministicWithImmutableArgs(address(this), args, salt); // both clones have the same code assertEq(instance1.code, instance2.code); diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index f6c2a71ff08..1ef81724be4 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -51,18 +51,18 @@ async function fixture() { async (opts = {}) => { const salt = opts.salt ?? ethers.randomBytes(32); const clone = await (args - ? factory.$cloneWithImmutableArgsDeterministic.staticCall(implementation, args, salt) + ? factory.$cloneDeterministicWithImmutableArgs.staticCall(implementation, args, salt) : factory.$cloneDeterministic.staticCall(implementation, salt) ).then(address => implementation.attach(address)); const tx = await (args ? opts.deployValue - ? factory.$cloneWithImmutableArgsDeterministic( + ? factory.$cloneDeterministicWithImmutableArgs( implementation, args, salt, ethers.Typed.uint256(opts.deployValue), ) - : factory.$cloneWithImmutableArgsDeterministic(implementation, args, salt) + : factory.$cloneDeterministicWithImmutableArgs(implementation, args, salt) : opts.deployValue ? factory.$cloneDeterministic(implementation, salt, ethers.Typed.uint256(opts.deployValue)) : factory.$cloneDeterministic(implementation, salt)); @@ -112,7 +112,7 @@ describe('Clones', function () { const deployClone = () => args - ? this.factory.$cloneWithImmutableArgsDeterministic(this.implementation, args, salt) + ? this.factory.$cloneDeterministicWithImmutableArgs(this.implementation, args, salt) : this.factory.$cloneDeterministic(this.implementation, salt); // deploy once @@ -132,15 +132,15 @@ describe('Clones', function () { ); if (args) { - const predicted = await this.factory.$predictWithImmutableArgsDeterministicAddress( + const predicted = await this.factory.$predictDeterministicAddressWithImmutableArgs( this.implementation, args, salt, ); expect(predicted).to.equal(expected); - await expect(this.factory.$cloneWithImmutableArgsDeterministic(this.implementation, args, salt)) - .to.emit(this.factory, 'return$cloneWithImmutableArgsDeterministic_address_bytes_bytes32') + await expect(this.factory.$cloneDeterministicWithImmutableArgs(this.implementation, args, salt)) + .to.emit(this.factory, 'return$cloneDeterministicWithImmutableArgs_address_bytes_bytes32') .withArgs(predicted); } else { const predicted = await this.factory.$predictDeterministicAddress(this.implementation, salt); From 054aa6a5dadca9b167cbb1b580aabe18554b3212 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Aug 2024 13:13:29 +0200 Subject: [PATCH 10/18] up --- test/proxy/Clones.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index 20257ee946f..cc67b0b67da 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -25,7 +25,7 @@ contract ClonesTest is Test { bytes32 salt, bytes memory args ) public { - vm.assume(args.length < 0xffd3); + vm.assume(args.length < 0xbfd3); address predicted = Clones.predictDeterministicAddressWithImmutableArgs(implementation, args, salt); bytes32 spillage; @@ -69,7 +69,7 @@ contract ClonesTest is Test { } function testFetchCloneArgs(bytes memory args, bytes32 salt) external { - vm.assume(args.length < 0xffd3); + vm.assume(args.length < 0xbfd3); address instance1 = Clones.cloneWithImmutableArgs(address(this), args); address instance2 = Clones.cloneDeterministicWithImmutableArgs(address(this), args, salt); From 959b36ab8ee2bb1e5e713a6f119cc43e9ded3fd9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 28 Aug 2024 18:24:15 +0200 Subject: [PATCH 11/18] test EIP-170 limits --- contracts/proxy/Clones.sol | 6 +++--- test/proxy/Clones.test.js | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index bd71f1dd05c..b539bb64761 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -244,14 +244,14 @@ library Clones { * `mcopy`. Unfortunately, that opcode is not available before cancun. A pure solidity implementation using * abi.encodePacked is more expensive but also more portable and easier to review. * - * NOTE: https://eips.ethereum.org/EIPS/eip-3860[EIP-3860] limits the length of the `initcode` to 49152 bytes. - * With the proxy code taking 45 bytes, that limits the length of the immutable args to 49107 bytes. + * NOTE: https://eips.ethereum.org/EIPS/eip-170[EIP-170] limits the length of the contract code to 24576 bytes. + * With the proxy code taking 45 bytes, that limits the length of the immutable args to 24531 bytes. */ function _cloneCodeWithImmutableArgs( address implementation, bytes memory args ) private pure returns (bytes memory) { - if (args.length > 49107) revert ImmutableArgsTooLarge(); + if (args.length > 0x5fd3) revert ImmutableArgsTooLarge(); return abi.encodePacked( hex"61", diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 1ef81724be4..c22810d6172 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -2,6 +2,8 @@ const { ethers } = require('hardhat'); const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { generators } = require('../helpers/random'); + const shouldBehaveLikeClone = require('./Clones.behaviour'); const cloneInitCode = (instance, args = undefined) => @@ -154,4 +156,20 @@ describe('Clones', function () { }); }); } + + it('EIP-170 limit on immutable args', async function () { + // EIP-170 limits the contract code size to 0x60000 + // This limits the length of immutable args to 0x5fd3 + const args = generators.hexBytes(0x5fd4); + const salt = ethers.randomBytes(32); + + await expect( + this.factory.$predictDeterministicAddressWithImmutableArgs(this.implementation, args, salt), + ).to.be.revertedWithCustomError(this.factory, 'ImmutableArgsTooLarge'); + + await expect(this.factory.$cloneWithImmutableArgs(this.implementation, args)).to.be.revertedWithCustomError( + this.factory, + 'ImmutableArgsTooLarge', + ); + }); }); From 2ff292a12e778a63d43bcc5ce77cf60563876d42 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 29 Aug 2024 10:04:57 +0200 Subject: [PATCH 12/18] Update test/proxy/Clones.test.js --- test/proxy/Clones.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index c22810d6172..f4ae21ca2b1 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -158,7 +158,7 @@ describe('Clones', function () { } it('EIP-170 limit on immutable args', async function () { - // EIP-170 limits the contract code size to 0x60000 + // EIP-170 limits the contract code size to 0x6000 // This limits the length of immutable args to 0x5fd3 const args = generators.hexBytes(0x5fd4); const salt = ethers.randomBytes(32); From 740ca1cfd86c4acc48e449cf323e40bba208dfa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Thu, 29 Aug 2024 21:24:24 -0600 Subject: [PATCH 13/18] Create four-chairs-help.md --- .changeset/four-chairs-help.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/four-chairs-help.md diff --git a/.changeset/four-chairs-help.md b/.changeset/four-chairs-help.md new file mode 100644 index 00000000000..6b8f118cc00 --- /dev/null +++ b/.changeset/four-chairs-help.md @@ -0,0 +1,5 @@ +--- +"openzeppelin-solidity": minor +--- + +`Clones`: Add `cloneWithImmutableArgs` and `cloneDeterministicWithImmutableArgs` variants that create clones per-instance immutable arguments. The corresponding `predictDeterministicWithImmutableArgs` function is included. From 029469a7b276745c51918c1c81fc9117e68f6686 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 30 Aug 2024 17:20:50 +0200 Subject: [PATCH 14/18] Update four-chairs-help.md --- .changeset/four-chairs-help.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/four-chairs-help.md b/.changeset/four-chairs-help.md index 6b8f118cc00..cbd0076075e 100644 --- a/.changeset/four-chairs-help.md +++ b/.changeset/four-chairs-help.md @@ -2,4 +2,4 @@ "openzeppelin-solidity": minor --- -`Clones`: Add `cloneWithImmutableArgs` and `cloneDeterministicWithImmutableArgs` variants that create clones per-instance immutable arguments. The corresponding `predictDeterministicWithImmutableArgs` function is included. +`Clones`: Add `cloneWithImmutableArgs` and `cloneDeterministicWithImmutableArgs` variants that create clones with per-instance immutable arguments. The immutable arguments can be retrieved using `fetchCloneArgs`. The corresponding `predictDeterministicWithImmutableArgs` function is also included. From f941c9d519bf6e82b9800c2aedbc3926a1f834c2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 2 Sep 2024 17:04:20 +0200 Subject: [PATCH 15/18] Rename custom error --- contracts/proxy/Clones.sol | 4 ++-- test/proxy/Clones.test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index b539bb64761..46c630cf6c6 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -18,7 +18,7 @@ import {Errors} from "../utils/Errors.sol"; * deterministic method. */ library Clones { - error ImmutableArgsTooLarge(); + error EIP170ContractCodeSizeLimit(); /** * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. @@ -251,7 +251,7 @@ library Clones { address implementation, bytes memory args ) private pure returns (bytes memory) { - if (args.length > 0x5fd3) revert ImmutableArgsTooLarge(); + if (args.length > 0x5fd3) revert EIP170ContractCodeSizeLimit(); return abi.encodePacked( hex"61", diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index f4ae21ca2b1..b98612adfb6 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -165,11 +165,11 @@ describe('Clones', function () { await expect( this.factory.$predictDeterministicAddressWithImmutableArgs(this.implementation, args, salt), - ).to.be.revertedWithCustomError(this.factory, 'ImmutableArgsTooLarge'); + ).to.be.revertedWithCustomError(this.factory, 'EIP170ContractCodeSizeLimit'); await expect(this.factory.$cloneWithImmutableArgs(this.implementation, args)).to.be.revertedWithCustomError( this.factory, - 'ImmutableArgsTooLarge', + 'EIP170ContractCodeSizeLimit', ); }); }); From c85adcc53f7e56fc2a963f93a80551d9d17b8fe8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 2 Sep 2024 17:08:37 +0200 Subject: [PATCH 16/18] Rename custom error --- contracts/proxy/Clones.sol | 4 ++-- test/proxy/Clones.test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 46c630cf6c6..1535fae77d0 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -18,7 +18,7 @@ import {Errors} from "../utils/Errors.sol"; * deterministic method. */ library Clones { - error EIP170ContractCodeSizeLimit(); + error ContractSizeLimitExcedeed(); /** * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. @@ -251,7 +251,7 @@ library Clones { address implementation, bytes memory args ) private pure returns (bytes memory) { - if (args.length > 0x5fd3) revert EIP170ContractCodeSizeLimit(); + if (args.length > 0x5fd3) revert ContractSizeLimitExcedeed(); return abi.encodePacked( hex"61", diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index b98612adfb6..5fccebae3e6 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -165,11 +165,11 @@ describe('Clones', function () { await expect( this.factory.$predictDeterministicAddressWithImmutableArgs(this.implementation, args, salt), - ).to.be.revertedWithCustomError(this.factory, 'EIP170ContractCodeSizeLimit'); + ).to.be.revertedWithCustomError(this.factory, 'ContractSizeLimitExcedeed'); await expect(this.factory.$cloneWithImmutableArgs(this.implementation, args)).to.be.revertedWithCustomError( this.factory, - 'EIP170ContractCodeSizeLimit', + 'ContractSizeLimitExcedeed', ); }); }); From 4364f58c8d72e834c0369b60aaaafeb8b501ba26 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 2 Sep 2024 17:19:56 +0200 Subject: [PATCH 17/18] Revert "Rename custom error" This reverts commit c85adcc53f7e56fc2a963f93a80551d9d17b8fe8. --- contracts/proxy/Clones.sol | 4 ++-- test/proxy/Clones.test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 1535fae77d0..46c630cf6c6 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -18,7 +18,7 @@ import {Errors} from "../utils/Errors.sol"; * deterministic method. */ library Clones { - error ContractSizeLimitExcedeed(); + error EIP170ContractCodeSizeLimit(); /** * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. @@ -251,7 +251,7 @@ library Clones { address implementation, bytes memory args ) private pure returns (bytes memory) { - if (args.length > 0x5fd3) revert ContractSizeLimitExcedeed(); + if (args.length > 0x5fd3) revert EIP170ContractCodeSizeLimit(); return abi.encodePacked( hex"61", diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 5fccebae3e6..b98612adfb6 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -165,11 +165,11 @@ describe('Clones', function () { await expect( this.factory.$predictDeterministicAddressWithImmutableArgs(this.implementation, args, salt), - ).to.be.revertedWithCustomError(this.factory, 'ContractSizeLimitExcedeed'); + ).to.be.revertedWithCustomError(this.factory, 'EIP170ContractCodeSizeLimit'); await expect(this.factory.$cloneWithImmutableArgs(this.implementation, args)).to.be.revertedWithCustomError( this.factory, - 'ContractSizeLimitExcedeed', + 'EIP170ContractCodeSizeLimit', ); }); }); From c9aa715d135e6267972475f0eb990284605de941 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 3 Sep 2024 21:43:52 +0200 Subject: [PATCH 18/18] Rename custom error --- contracts/proxy/Clones.sol | 4 ++-- test/proxy/Clones.test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 46c630cf6c6..c72408b37a4 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -18,7 +18,7 @@ import {Errors} from "../utils/Errors.sol"; * deterministic method. */ library Clones { - error EIP170ContractCodeSizeLimit(); + error CloneArgumentsTooLong(); /** * @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`. @@ -251,7 +251,7 @@ library Clones { address implementation, bytes memory args ) private pure returns (bytes memory) { - if (args.length > 0x5fd3) revert EIP170ContractCodeSizeLimit(); + if (args.length > 0x5fd3) revert CloneArgumentsTooLong(); return abi.encodePacked( hex"61", diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index b98612adfb6..0706c6f834c 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -165,11 +165,11 @@ describe('Clones', function () { await expect( this.factory.$predictDeterministicAddressWithImmutableArgs(this.implementation, args, salt), - ).to.be.revertedWithCustomError(this.factory, 'EIP170ContractCodeSizeLimit'); + ).to.be.revertedWithCustomError(this.factory, 'CloneArgumentsTooLong'); await expect(this.factory.$cloneWithImmutableArgs(this.implementation, args)).to.be.revertedWithCustomError( this.factory, - 'EIP170ContractCodeSizeLimit', + 'CloneArgumentsTooLong', ); }); });