From 0431ef7e2924df040468b600b4a9e0751de2427a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 Jun 2024 19:08:06 +0200 Subject: [PATCH 1/7] Revert 8b2f29ceb0db64957a49deb63af7792772bd7a5d that introduced vulnerability to dirty upper bits in the implementation address --- contracts/proxy/Clones.sol | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/contracts/proxy/Clones.sol b/contracts/proxy/Clones.sol index 7001572b6e6..d243d67f34b 100644 --- a/contracts/proxy/Clones.sol +++ b/contracts/proxy/Clones.sol @@ -39,12 +39,11 @@ library Clones { } /// @solidity memory-safe-assembly assembly { - // Stores the bytecode after address - mstore(0x20, 0x5af43d82803e903d91602b57fd5bf3) - // implementation address - mstore(0x11, implementation) - // Packs the first 3 bytes of the `implementation` address with the bytecode before the address. - mstore(0x00, or(shr(0x88, implementation), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) + // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes + // of the `implementation` address with the bytecode before the address. + mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) + // Packs the remaining 17 bytes of `implementation` with the bytecode after the address. + mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3)) instance := create(value, 0x09, 0x37) } if (instance == address(0)) { @@ -80,12 +79,11 @@ library Clones { } /// @solidity memory-safe-assembly assembly { - // Stores the bytecode after address - mstore(0x20, 0x5af43d82803e903d91602b57fd5bf3) - // implementation address - mstore(0x11, implementation) - // Packs the first 3 bytes of the `implementation` address with the bytecode before the address. - mstore(0x00, or(shr(0x88, implementation), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) + // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes + // of the `implementation` address with the bytecode before the address. + mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000)) + // Packs the remaining 17 bytes of `implementation` with the bytecode after the address. + mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3)) instance := create2(value, 0x09, 0x37, salt) } if (instance == address(0)) { From eea2d94aa62c65e21943c4f09dc8eac15e455974 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 6 Jun 2024 11:43:13 -0600 Subject: [PATCH 2/7] Add tests --- test/proxy/Clones.t.sol | 88 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index 9015978fcaf..0d6a0967d90 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -2,10 +2,52 @@ pragma solidity ^0.8.20; -import {Test} from "forge-std/Test.sol"; +import {Test, console2} from "forge-std/Test.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; +contract DummyContract { + function getNumber() external pure returns (uint256) { + return 42; + } +} + +contract ClonesMock { + function cloneContract(address target) external returns (address) { + return Clones.clone(target); + } + + function cloneDirtyTarget(address target) external returns (address) { + // Add dirty bits; will start with (0xff..) + assembly { + // Get type(uint256).max and shift to the left by 20 bytes => `or` with target + target := or(shl(160, not(0)), target) + } + return Clones.clone(target); + } + + function cloneContractDeterministic(address target) external returns (address) { + return Clones.clone(target); + } + + function cloneDirtyTargetDeterministic(address target) external returns (address) { + // Add dirty bits; will start with (0xff..) + assembly { + // Get type(uint256).max and shift to the left by 20 bytes => `or` with target + target := or(shl(160, not(0)), target) + } + return Clones.clone(target); + } +} + contract ClonesTest is Test { + DummyContract private dummy; + ClonesMock private clonesWrapper; + + function setUp() public { + dummy = new DummyContract(); + clonesWrapper = new ClonesMock(); + } + function testSymbolicPredictDeterministicAddressSpillage(address implementation, bytes32 salt) public { address predicted = Clones.predictDeterministicAddress(implementation, salt); bytes32 spillage; @@ -15,4 +57,48 @@ contract ClonesTest is Test { } assertEq(spillage, bytes32(0)); } + + function test_DummyContractReturnsCorrectNumber() external { + assertEq(dummy.getNumber(), 42); + } + + /// Clone + + function test_ValidCloneOfDummyContractReturnsCorrectNumber() external { + DummyContract dummyClone = DummyContract(clonesWrapper.cloneContract(address(dummy))); + assertEq(dummyClone.getNumber(), dummy.getNumber()); + } + + function test_ClonesOfSameAddressHaveSameCodeDirty() external { + address clone = clonesWrapper.cloneContract(address(dummy)); + address invalidClone = clonesWrapper.cloneDirtyTarget(address(dummy)); + + assertEq(keccak256(clone.code), keccak256(invalidClone.code)); + } + + function testFail_CallToInvalidCloneWillRevertBecauseItHasNoCode() external { + DummyContract invalidDummyClone = DummyContract(clonesWrapper.cloneDirtyTarget(address(dummy))); + vm.expectRevert(); + invalidDummyClone.getNumber(); + } + + /// Clone Deterministic + + function test_ValidCloneOfDummyContractReturnsCorrectNumber_Deterministic() external { + DummyContract dummyClone = DummyContract(clonesWrapper.cloneContractDeterministic(address(dummy))); + assertEq(dummyClone.getNumber(), dummy.getNumber()); + } + + function test_ClonesOfSameAddressHaveSameCodeDirty_Deterministic() external { + address clone = clonesWrapper.cloneContractDeterministic(address(dummy)); + address invalidClone = clonesWrapper.cloneDirtyTargetDeterministic(address(dummy)); + + assertEq(keccak256(clone.code), keccak256(invalidClone.code)); + } + + function testFail_CallToInvalidCloneWillRevertBecauseItHasNoCode_Deterministic() external { + DummyContract invalidDummyClone = DummyContract(clonesWrapper.cloneDirtyTargetDeterministic(address(dummy))); + vm.expectRevert(); + invalidDummyClone.getNumber(); + } } From 101331b801a29daf7960c616b7eebb50356eb1c4 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 6 Jun 2024 11:44:01 -0600 Subject: [PATCH 3/7] Disable func-name-mixedcase --- test/proxy/Clones.t.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index 0d6a0967d90..13ab50f9274 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -2,6 +2,8 @@ pragma solidity ^0.8.20; +// solhint-disable func-name-mixedcase + import {Test, console2} from "forge-std/Test.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; From d2ce59af043babe018292c56f94263c2636ed7eb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 6 Jun 2024 22:44:30 +0200 Subject: [PATCH 4/7] Update Clones.t.sol --- test/proxy/Clones.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index 13ab50f9274..ba1624c90ee 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.20; // solhint-disable func-name-mixedcase -import {Test, console2} from "forge-std/Test.sol"; +import {Test} from "forge-std/Test.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; contract DummyContract { From b246576f71bee2a4dad27f2a30cd8460aeea07d8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 7 Jun 2024 21:59:26 +0200 Subject: [PATCH 5/7] cleanup tests --- test/proxy/Clones.t.sol | 98 +++++++++++------------------------------ 1 file changed, 26 insertions(+), 72 deletions(-) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index ba1624c90ee..d814ad4e6e9 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -2,53 +2,13 @@ pragma solidity ^0.8.20; -// solhint-disable func-name-mixedcase - import {Test} from "forge-std/Test.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; -contract DummyContract { +contract ClonesTest is Test { function getNumber() external pure returns (uint256) { return 42; } -} - -contract ClonesMock { - function cloneContract(address target) external returns (address) { - return Clones.clone(target); - } - - function cloneDirtyTarget(address target) external returns (address) { - // Add dirty bits; will start with (0xff..) - assembly { - // Get type(uint256).max and shift to the left by 20 bytes => `or` with target - target := or(shl(160, not(0)), target) - } - return Clones.clone(target); - } - - function cloneContractDeterministic(address target) external returns (address) { - return Clones.clone(target); - } - - function cloneDirtyTargetDeterministic(address target) external returns (address) { - // Add dirty bits; will start with (0xff..) - assembly { - // Get type(uint256).max and shift to the left by 20 bytes => `or` with target - target := or(shl(160, not(0)), target) - } - return Clones.clone(target); - } -} - -contract ClonesTest is Test { - DummyContract private dummy; - ClonesMock private clonesWrapper; - - function setUp() public { - dummy = new DummyContract(); - clonesWrapper = new ClonesMock(); - } function testSymbolicPredictDeterministicAddressSpillage(address implementation, bytes32 salt) public { address predicted = Clones.predictDeterministicAddress(implementation, salt); @@ -60,47 +20,41 @@ contract ClonesTest is Test { assertEq(spillage, bytes32(0)); } - function test_DummyContractReturnsCorrectNumber() external { - assertEq(dummy.getNumber(), 42); - } + function testCloneDirty() external { + address cloneClean = Clones.clone(address(this)); + address cloneDirty = Clones.clone(_dirty(address(this))); - /// Clone + // both clones have the same code + assertEq(keccak256(cloneClean.code), keccak256(cloneDirty.code)); - function test_ValidCloneOfDummyContractReturnsCorrectNumber() external { - DummyContract dummyClone = DummyContract(clonesWrapper.cloneContract(address(dummy))); - assertEq(dummyClone.getNumber(), dummy.getNumber()); + // both clones behave as expected + assertEq(ClonesTest(cloneClean).getNumber(), this.getNumber()); + assertEq(ClonesTest(cloneDirty).getNumber(), this.getNumber()); } - function test_ClonesOfSameAddressHaveSameCodeDirty() external { - address clone = clonesWrapper.cloneContract(address(dummy)); - address invalidClone = clonesWrapper.cloneDirtyTarget(address(dummy)); + function testCloneDeterministicDirty() external { + address cloneClean = Clones.cloneDeterministic(address(this), bytes32(block.prevrandao)); + address cloneDirty = Clones.cloneDeterministic(_dirty(address(this)), bytes32(~block.prevrandao)); - assertEq(keccak256(clone.code), keccak256(invalidClone.code)); - } - - function testFail_CallToInvalidCloneWillRevertBecauseItHasNoCode() external { - DummyContract invalidDummyClone = DummyContract(clonesWrapper.cloneDirtyTarget(address(dummy))); - vm.expectRevert(); - invalidDummyClone.getNumber(); - } + // both clones have the same code + assertEq(keccak256(cloneClean.code), keccak256(cloneDirty.code)); - /// Clone Deterministic - - function test_ValidCloneOfDummyContractReturnsCorrectNumber_Deterministic() external { - DummyContract dummyClone = DummyContract(clonesWrapper.cloneContractDeterministic(address(dummy))); - assertEq(dummyClone.getNumber(), dummy.getNumber()); + // both clones behave as expected + assertEq(ClonesTest(cloneClean).getNumber(), this.getNumber()); + assertEq(ClonesTest(cloneDirty).getNumber(), this.getNumber()); } - function test_ClonesOfSameAddressHaveSameCodeDirty_Deterministic() external { - address clone = clonesWrapper.cloneContractDeterministic(address(dummy)); - address invalidClone = clonesWrapper.cloneDirtyTargetDeterministic(address(dummy)); + function testPredictDeterministicAddressDirty(bytes32 salt) external { + address predictClean = Clones.predictDeterministicAddress(address(this), salt); + address predictDirty = Clones.predictDeterministicAddress(_dirty(address(this)), salt); - assertEq(keccak256(clone.code), keccak256(invalidClone.code)); + //prediction should be similar + assertEq(predictClean, predictDirty); } - function testFail_CallToInvalidCloneWillRevertBecauseItHasNoCode_Deterministic() external { - DummyContract invalidDummyClone = DummyContract(clonesWrapper.cloneDirtyTargetDeterministic(address(dummy))); - vm.expectRevert(); - invalidDummyClone.getNumber(); + function _dirty(address input) private pure returns (address output) { + assembly { + output := or(input, shl(160, not(0))) + } } } From 1d9d2f39f174736f2ac9e265874958e66640b67f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 10 Jun 2024 21:19:44 -0600 Subject: [PATCH 6/7] Implement PR suggestions --- test/proxy/Clones.t.sol | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index d814ad4e6e9..4ffed99cfdb 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -20,9 +20,11 @@ contract ClonesTest is Test { assertEq(spillage, bytes32(0)); } - function testCloneDirty() external { + function testCloneDirty(address caller) external { + vm.startPrank(caller); address cloneClean = Clones.clone(address(this)); address cloneDirty = Clones.clone(_dirty(address(this))); + vm.stopPrank(); // both clones have the same code assertEq(keccak256(cloneClean.code), keccak256(cloneDirty.code)); @@ -32,9 +34,9 @@ contract ClonesTest is Test { assertEq(ClonesTest(cloneDirty).getNumber(), this.getNumber()); } - function testCloneDeterministicDirty() external { - address cloneClean = Clones.cloneDeterministic(address(this), bytes32(block.prevrandao)); - address cloneDirty = Clones.cloneDeterministic(_dirty(address(this)), bytes32(~block.prevrandao)); + function testCloneDeterministicDirty(bytes32 salt) external { + address cloneClean = Clones.cloneDeterministic(address(this), salt); + address cloneDirty = Clones.cloneDeterministic(_dirty(address(this)), ~salt); // both clones have the same code assertEq(keccak256(cloneClean.code), keccak256(cloneDirty.code)); @@ -53,7 +55,7 @@ contract ClonesTest is Test { } function _dirty(address input) private pure returns (address output) { - assembly { + assembly ("memory-safe") { output := or(input, shl(160, not(0))) } } From 1d49361632b49699d3bfbcaf6519e9438e77f7a8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jun 2024 11:11:47 +0200 Subject: [PATCH 7/7] Update test/proxy/Clones.t.sol --- test/proxy/Clones.t.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/proxy/Clones.t.sol b/test/proxy/Clones.t.sol index 4ffed99cfdb..4301e103c26 100644 --- a/test/proxy/Clones.t.sol +++ b/test/proxy/Clones.t.sol @@ -20,11 +20,9 @@ contract ClonesTest is Test { assertEq(spillage, bytes32(0)); } - function testCloneDirty(address caller) external { - vm.startPrank(caller); + function testCloneDirty() external { address cloneClean = Clones.clone(address(this)); address cloneDirty = Clones.clone(_dirty(address(this))); - vm.stopPrank(); // both clones have the same code assertEq(keccak256(cloneClean.code), keccak256(cloneDirty.code));