From 9769286a78e8cae721795d4f83a1ab6a527db695 Mon Sep 17 00:00:00 2001 From: byshape Date: Tue, 5 Mar 2024 18:05:37 +0000 Subject: [PATCH 1/6] Masked computed address in Create2.computeAddress --- .../mocks/Create2ComputedAddressMock.sol | 20 +++++++++++++++++++ contracts/utils/Create2.sol | 2 +- test/utils/Create2.test.js | 12 ++++++++++- 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 contracts/mocks/Create2ComputedAddressMock.sol diff --git a/contracts/mocks/Create2ComputedAddressMock.sol b/contracts/mocks/Create2ComputedAddressMock.sol new file mode 100644 index 00000000000..9766a46a997 --- /dev/null +++ b/contracts/mocks/Create2ComputedAddressMock.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Create2} from "../utils/Create2.sol"; + +contract Create2ComputedAddressMock { + function getComputedRemainder( + bytes32 salt, + bytes32 bytecodeHash, + address deployer + ) external pure returns (uint256 remainder) { + uint256 remainderMask = ~(uint256(type(uint160).max)); + address predicted = Create2.computeAddress(salt, bytecodeHash, deployer); + + assembly ("memory-safe") { + remainder := and(predicted, remainderMask) + } + } +} diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index ad1cd5f4d54..d16611ee17f 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -90,7 +90,7 @@ library Create2 { mstore(ptr, deployer) // Right-aligned with 12 preceding garbage bytes let start := add(ptr, 0x0b) // The hashed data starts at the final garbage byte which we will set to 0xff mstore8(start, 0xff) - addr := keccak256(start, 85) + addr := and(keccak256(start, 85), 0xffffffffffffffffffffffffffffffffffffffff) } } } diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 86d00e13197..57801859f1e 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -6,6 +6,7 @@ async function fixture() { const [deployer, other] = await ethers.getSigners(); const factory = await ethers.deployContract('$Create2'); + const computedAddressMock = await ethers.deployContract('Create2ComputedAddressMock'); // Bytecode for deploying a contract that includes a constructor. // We use a vesting wallet, with 3 constructor arguments. @@ -19,7 +20,7 @@ async function fixture() { .getContractFactory('$Create2') .then(({ bytecode, interface }) => ethers.concat([bytecode, interface.encodeDeploy([])])); - return { deployer, other, factory, constructorByteCode, constructorLessBytecode }; + return { deployer, other, factory, computedAddressMock, constructorByteCode, constructorLessBytecode }; } describe('Create2', function () { @@ -54,6 +55,15 @@ describe('Create2', function () { ); expect(onChainComputed).to.equal(offChainComputed); }); + + it('computes the clean contract address', async function () { + const computedRemainder = await this.computedAddressMock.getComputedRemainder( + saltHex, + ethers.keccak256(this.constructorByteCode), + ethers.Typed.address(this.deployer), + ); + expect(computedRemainder).to.equal(0); + }); }); describe('deploy', function () { From c5c7cf5ba9a53c7359184cc8a6cc153d20b13252 Mon Sep 17 00:00:00 2001 From: byshape Date: Tue, 5 Mar 2024 19:26:55 +0000 Subject: [PATCH 2/6] Masked computed address in Clones.predictDeterministicAddress --- .../mocks/ComputedAddressRemainderMock.sol | 36 +++++++++++++++++++ .../mocks/Create2ComputedAddressMock.sol | 20 ----------- contracts/proxy/Clones.sol | 2 +- test/proxy/Clones.test.js | 13 ++++++- test/utils/Create2.test.js | 4 +-- 5 files changed, 51 insertions(+), 24 deletions(-) create mode 100644 contracts/mocks/ComputedAddressRemainderMock.sol delete mode 100644 contracts/mocks/Create2ComputedAddressMock.sol diff --git a/contracts/mocks/ComputedAddressRemainderMock.sol b/contracts/mocks/ComputedAddressRemainderMock.sol new file mode 100644 index 00000000000..93c1228fbde --- /dev/null +++ b/contracts/mocks/ComputedAddressRemainderMock.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Clones} from "../proxy/Clones.sol"; +import {Create2} from "../utils/Create2.sol"; + +contract ComputedAddressRemainderMock { + uint256 private constant _REMAINDER_MASK = ~(uint256(type(uint160).max)); + + function getCreate2ComputedRemainder( + bytes32 salt, + bytes32 bytecodeHash, + address deployer + ) external pure returns (uint256 remainder) { + address predicted = Create2.computeAddress(salt, bytecodeHash, deployer); + remainder = _getRemainder(predicted); + } + + function getClonesPredictedRemainder( + address implementation, + bytes32 salt, + address deployer + ) external pure returns (uint256 remainder) { + address predicted = Clones.predictDeterministicAddress(implementation, salt, deployer); + remainder = _getRemainder(predicted); + } + + function _getRemainder(address addr) internal pure returns (uint256 remainder) { + uint256 remainderMask = _REMAINDER_MASK; + + assembly ("memory-safe") { + remainder := and(addr, remainderMask) + } + } +} diff --git a/contracts/mocks/Create2ComputedAddressMock.sol b/contracts/mocks/Create2ComputedAddressMock.sol deleted file mode 100644 index 9766a46a997..00000000000 --- a/contracts/mocks/Create2ComputedAddressMock.sol +++ /dev/null @@ -1,20 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {Create2} from "../utils/Create2.sol"; - -contract Create2ComputedAddressMock { - function getComputedRemainder( - bytes32 salt, - bytes32 bytecodeHash, - address deployer - ) external pure returns (uint256 remainder) { - uint256 remainderMask = ~(uint256(type(uint160).max)); - address predicted = Create2.computeAddress(salt, bytecodeHash, deployer); - - assembly ("memory-safe") { - remainder := and(predicted, remainderMask) - } - } -} diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 92ed339a756..46e29b03b30 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -79,7 +79,7 @@ library Clones { mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73) mstore(add(ptr, 0x58), salt) mstore(add(ptr, 0x78), keccak256(add(ptr, 0x0c), 0x37)) - predicted := keccak256(add(ptr, 0x43), 0x55) + predicted := and(keccak256(add(ptr, 0x43), 0x55), 0xffffffffffffffffffffffffffffffffffffffff) } } diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 626b1e56467..37dda9d9dd7 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -9,6 +9,7 @@ async function fixture() { const factory = await ethers.deployContract('$Clones'); const implementation = await ethers.deployContract('DummyImplementation'); + const computedAddressMock = await ethers.deployContract('ComputedAddressRemainderMock'); const newClone = async (initData, opts = {}) => { const clone = await factory.$clone.staticCall(implementation).then(address => implementation.attach(address)); @@ -27,7 +28,7 @@ async function fixture() { return clone; }; - return { deployer, factory, implementation, newClone, newCloneDeterministic }; + return { deployer, factory, implementation, computedAddressMock, newClone, newCloneDeterministic }; } describe('Clones', function () { @@ -83,5 +84,15 @@ describe('Clones', function () { .to.emit(this.factory, 'return$cloneDeterministic') .withArgs(predicted); }); + + it('clean address prediction', async function () { + const salt = ethers.randomBytes(32); + const predictedRemainder = await this.computedAddressMock.getClonesPredictedRemainder( + this.implementation, + salt, + ethers.Typed.address(this.deployer), + ); + expect(predictedRemainder).to.equal(0); + }); }); }); diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 57801859f1e..75472ded2f1 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -6,7 +6,7 @@ async function fixture() { const [deployer, other] = await ethers.getSigners(); const factory = await ethers.deployContract('$Create2'); - const computedAddressMock = await ethers.deployContract('Create2ComputedAddressMock'); + const computedAddressMock = await ethers.deployContract('ComputedAddressRemainderMock'); // Bytecode for deploying a contract that includes a constructor. // We use a vesting wallet, with 3 constructor arguments. @@ -57,7 +57,7 @@ describe('Create2', function () { }); it('computes the clean contract address', async function () { - const computedRemainder = await this.computedAddressMock.getComputedRemainder( + const computedRemainder = await this.computedAddressMock.getCreate2ComputedRemainder( saltHex, ethers.keccak256(this.constructorByteCode), ethers.Typed.address(this.deployer), From 85d0b75f99bd7933aee96271d952817d82233a30 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 14 Mar 2024 23:58:52 +0000 Subject: [PATCH 3/6] Add changeset --- .changeset/fluffy-steaks-exist.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fluffy-steaks-exist.md diff --git a/.changeset/fluffy-steaks-exist.md b/.changeset/fluffy-steaks-exist.md new file mode 100644 index 00000000000..b625e243481 --- /dev/null +++ b/.changeset/fluffy-steaks-exist.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`Create2`, `Clones`: Mask `computeAddress` and `cloneDeterministic` outputs to produce a clean value for an `address` type (i.e. only use 20 bytes) From a52481ab31599fa6f03ec21ff3b11090c70aed5b Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 15 Mar 2024 00:14:13 +0000 Subject: [PATCH 4/6] Change memory-safe to an annotation --- contracts/mocks/ComputedAddressRemainderMock.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/mocks/ComputedAddressRemainderMock.sol b/contracts/mocks/ComputedAddressRemainderMock.sol index 93c1228fbde..a265cbf43f2 100644 --- a/contracts/mocks/ComputedAddressRemainderMock.sol +++ b/contracts/mocks/ComputedAddressRemainderMock.sol @@ -29,7 +29,8 @@ contract ComputedAddressRemainderMock { function _getRemainder(address addr) internal pure returns (uint256 remainder) { uint256 remainderMask = _REMAINDER_MASK; - assembly ("memory-safe") { + /// @solidity memory-safe-assembly + assembly { remainder := and(addr, remainderMask) } } From 66df9c608c0f801ad08c02077f131d4195f275b9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 17 Mar 2024 13:37:20 +0100 Subject: [PATCH 5/6] check using fuzzing --- .../mocks/ComputedAddressRemainderMock.sol | 37 ------------------- test/proxy/Clones.t.sol | 18 +++++++++ test/proxy/Clones.test.js | 13 +------ test/utils/Create2.t.sol | 18 +++++++++ test/utils/Create2.test.js | 12 +----- 5 files changed, 38 insertions(+), 60 deletions(-) delete mode 100644 contracts/mocks/ComputedAddressRemainderMock.sol create mode 100644 test/proxy/Clones.t.sol create mode 100644 test/utils/Create2.t.sol diff --git a/contracts/mocks/ComputedAddressRemainderMock.sol b/contracts/mocks/ComputedAddressRemainderMock.sol deleted file mode 100644 index a265cbf43f2..00000000000 --- a/contracts/mocks/ComputedAddressRemainderMock.sol +++ /dev/null @@ -1,37 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {Clones} from "../proxy/Clones.sol"; -import {Create2} from "../utils/Create2.sol"; - -contract ComputedAddressRemainderMock { - uint256 private constant _REMAINDER_MASK = ~(uint256(type(uint160).max)); - - function getCreate2ComputedRemainder( - bytes32 salt, - bytes32 bytecodeHash, - address deployer - ) external pure returns (uint256 remainder) { - address predicted = Create2.computeAddress(salt, bytecodeHash, deployer); - remainder = _getRemainder(predicted); - } - - function getClonesPredictedRemainder( - address implementation, - bytes32 salt, - address deployer - ) external pure returns (uint256 remainder) { - address predicted = Clones.predictDeterministicAddress(implementation, salt, deployer); - remainder = _getRemainder(predicted); - } - - function _getRemainder(address addr) internal pure returns (uint256 remainder) { - uint256 remainderMask = _REMAINDER_MASK; - - /// @solidity memory-safe-assembly - assembly { - remainder := and(addr, remainderMask) - } - } -} diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol new file mode 100644 index 00000000000..cfe025b5c23 --- /dev/null +++ b/test/proxy/Clones.t.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; + +contract ClonesTest is Test { + function testClonesPredicted(address implementation, bytes32 salt) public { + address predicted = Clones.predictDeterministicAddress(implementation, salt); + bytes32 spill; + /// @solidity memory-safe-assembly + assembly { + spill := and(predicted, 0xffffffffffffffffffffffff0000000000000000000000000000000000000000) + } + assertEq(spill, bytes32(0)); + } +} diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 37dda9d9dd7..626b1e56467 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -9,7 +9,6 @@ async function fixture() { const factory = await ethers.deployContract('$Clones'); const implementation = await ethers.deployContract('DummyImplementation'); - const computedAddressMock = await ethers.deployContract('ComputedAddressRemainderMock'); const newClone = async (initData, opts = {}) => { const clone = await factory.$clone.staticCall(implementation).then(address => implementation.attach(address)); @@ -28,7 +27,7 @@ async function fixture() { return clone; }; - return { deployer, factory, implementation, computedAddressMock, newClone, newCloneDeterministic }; + return { deployer, factory, implementation, newClone, newCloneDeterministic }; } describe('Clones', function () { @@ -84,15 +83,5 @@ describe('Clones', function () { .to.emit(this.factory, 'return$cloneDeterministic') .withArgs(predicted); }); - - it('clean address prediction', async function () { - const salt = ethers.randomBytes(32); - const predictedRemainder = await this.computedAddressMock.getClonesPredictedRemainder( - this.implementation, - salt, - ethers.Typed.address(this.deployer), - ); - expect(predictedRemainder).to.equal(0); - }); }); }); diff --git a/test/utils/Create2.t.sol b/test/utils/Create2.t.sol new file mode 100644 index 00000000000..18f920fd0ef --- /dev/null +++ b/test/utils/Create2.t.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; + +contract Create2Test is Test { + function testClonesPredicted(bytes32 salt, bytes32 bytecodeHash, address deployer) public { + address predicted = Create2.computeAddress(salt, bytecodeHash, deployer); + bytes32 spill; + /// @solidity memory-safe-assembly + assembly { + spill := and(predicted, 0xffffffffffffffffffffffff0000000000000000000000000000000000000000) + } + assertEq(spill, bytes32(0)); + } +} diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 75472ded2f1..86d00e13197 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -6,7 +6,6 @@ async function fixture() { const [deployer, other] = await ethers.getSigners(); const factory = await ethers.deployContract('$Create2'); - const computedAddressMock = await ethers.deployContract('ComputedAddressRemainderMock'); // Bytecode for deploying a contract that includes a constructor. // We use a vesting wallet, with 3 constructor arguments. @@ -20,7 +19,7 @@ async function fixture() { .getContractFactory('$Create2') .then(({ bytecode, interface }) => ethers.concat([bytecode, interface.encodeDeploy([])])); - return { deployer, other, factory, computedAddressMock, constructorByteCode, constructorLessBytecode }; + return { deployer, other, factory, constructorByteCode, constructorLessBytecode }; } describe('Create2', function () { @@ -55,15 +54,6 @@ describe('Create2', function () { ); expect(onChainComputed).to.equal(offChainComputed); }); - - it('computes the clean contract address', async function () { - const computedRemainder = await this.computedAddressMock.getCreate2ComputedRemainder( - saltHex, - ethers.keccak256(this.constructorByteCode), - ethers.Typed.address(this.deployer), - ); - expect(computedRemainder).to.equal(0); - }); }); describe('deploy', function () { From 379f055e6954332ed7bebed7ab503344b961ebf5 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 17 Mar 2024 13:40:38 +0100 Subject: [PATCH 6/6] naming --- test/proxy/Clones.t.sol | 8 ++++---- test/utils/Create2.t.sol | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index cfe025b5c23..214de2d5a51 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -6,13 +6,13 @@ import {Test} from "forge-std/Test.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; contract ClonesTest is Test { - function testClonesPredicted(address implementation, bytes32 salt) public { + function testPredictDeterministicAddressSpillage(address implementation, bytes32 salt) public { address predicted = Clones.predictDeterministicAddress(implementation, salt); - bytes32 spill; + bytes32 spillage; /// @solidity memory-safe-assembly assembly { - spill := and(predicted, 0xffffffffffffffffffffffff0000000000000000000000000000000000000000) + spillage := and(predicted, 0xffffffffffffffffffffffff0000000000000000000000000000000000000000) } - assertEq(spill, bytes32(0)); + assertEq(spillage, bytes32(0)); } } diff --git a/test/utils/Create2.t.sol b/test/utils/Create2.t.sol index 18f920fd0ef..d602eded0a5 100644 --- a/test/utils/Create2.t.sol +++ b/test/utils/Create2.t.sol @@ -6,13 +6,13 @@ import {Test} from "forge-std/Test.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; contract Create2Test is Test { - function testClonesPredicted(bytes32 salt, bytes32 bytecodeHash, address deployer) public { + function testComputeAddressSpillage(bytes32 salt, bytes32 bytecodeHash, address deployer) public { address predicted = Create2.computeAddress(salt, bytecodeHash, deployer); - bytes32 spill; + bytes32 spillage; /// @solidity memory-safe-assembly assembly { - spill := and(predicted, 0xffffffffffffffffffffffff0000000000000000000000000000000000000000) + spillage := and(predicted, 0xffffffffffffffffffffffff0000000000000000000000000000000000000000) } - assertEq(spill, bytes32(0)); + assertEq(spillage, bytes32(0)); } }