From 08c1ad09d9f892bc38735053478021e7e7017882 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Tue, 21 May 2024 13:07:42 +0100 Subject: [PATCH] test: demonstrate non-reentrancy guaranteed by CREATE2 address collision --- test/ERC721ForERC20.t.sol | 6 ++++++ test/ERC721ForNative.t.sol | 6 ++++++ test/ERC721ForXTest.t.sol | 19 +++++++++++++++++++ test/MultiERC721ForERC20.t.sol | 6 ++++++ test/MultiERC721ForNative.t.sol | 7 +++++++ test/SwapperTestBase.t.sol | 23 +++++++++++++++++++++++ 6 files changed, 67 insertions(+) diff --git a/test/ERC721ForERC20.t.sol b/test/ERC721ForERC20.t.sol index 58c93d7..c91229a 100644 --- a/test/ERC721ForERC20.t.sol +++ b/test/ERC721ForERC20.t.sol @@ -9,6 +9,7 @@ import {ERC20Test} from "./ERC20Test.t.sol"; import {ERC721Token} from "../src/ERC721SwapperLib.sol"; import {ERC721ForERC20Swap, IERC20} from "../src/ERC721ForERC20/ERC721ForERC20Swap.sol"; +import {ERC721ForERC20SwapperDeployer} from "../src/ERC721ForERC20/ERC721ForERC20SwapperDeployer.gen.sol"; import {InsufficientBalance, Disbursement, Parties} from "../src/TypesAndConstants.sol"; import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol"; @@ -50,6 +51,11 @@ contract ERC721ForERC20Test is ERC721ForXTest, ERC20Test { return abi.encode(_asSwap(t), salt); } + /// @inheritdoc ERC721ForXTest + function _fillSelector() internal pure override returns (bytes4) { + return ERC721ForERC20SwapperDeployer.fill.selector; + } + /// @inheritdoc ERC721ForXTest function _fill(ERC721TestCase memory t) internal override { factory.fill(_asSwap(t), t.base.salt); diff --git a/test/ERC721ForNative.t.sol b/test/ERC721ForNative.t.sol index b50a4a2..aaa0c38 100644 --- a/test/ERC721ForNative.t.sol +++ b/test/ERC721ForNative.t.sol @@ -9,6 +9,7 @@ import {NativeTokenTest} from "./NativeTokenTest.t.sol"; import {ERC721Token} from "../src/ERC721SwapperLib.sol"; import {ERC721ForNativeSwap} from "../src/ERC721ForNative/ERC721ForNativeSwap.sol"; +import {ERC721ForNativeSwapperDeployer} from "../src/ERC721ForNative/ERC721ForNativeSwapperDeployer.gen.sol"; import {InsufficientBalance, Disbursement, Parties} from "../src/TypesAndConstants.sol"; import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol"; @@ -44,6 +45,11 @@ contract ERC721ForNativeTest is ERC721ForXTest, NativeTokenTest { return abi.encode(_asSwap(t), salt); } + /// @inheritdoc ERC721ForXTest + function _fillSelector() internal pure override returns (bytes4) { + return ERC721ForNativeSwapperDeployer.fill.selector; + } + /// @inheritdoc ERC721ForXTest function _fill(ERC721TestCase memory t) internal override { _fill(t, t.base.native.callValue); diff --git a/test/ERC721ForXTest.t.sol b/test/ERC721ForXTest.t.sol index 1c20423..d67abcf 100644 --- a/test/ERC721ForXTest.t.sol +++ b/test/ERC721ForXTest.t.sol @@ -39,6 +39,8 @@ abstract contract ERC721ForXTest is SwapperTestBase { function _encodedSwapAndSalt(ERC721TestCase memory, bytes32) internal view virtual returns (bytes memory); + function _fillSelector() internal pure virtual returns (bytes4); + /// @dev Fills the swap defined by the test case. function _fill(ERC721TestCase memory) internal virtual; @@ -278,6 +280,23 @@ abstract contract ERC721ForXTest is SwapperTestBase { vm.stopPrank(); } + function testNonReentrant(ERC721TestCase memory t) + external + assumeValidTest(t.base) + assumePaymentsValid(t.base) + assumeSufficientPayment(t.base) + assumeValidPlatformFee(t.base) + assumeApproving(t.base) + { + token.setPostTransferCall( + address(factory), abi.encodePacked(_fillSelector(), _encodedSwapAndSalt(t, t.base.salt)) + ); + + // The most precise way to detect a redeployment is to see that CREATE2 reverts without any return data. + // Inspection of the trace with `forge test -vvvv` is necessary to see the [CreationCollision] error. + _testFill(t, abi.encodeWithSelector(ETDeployer.Create2EmptyRevert.selector)); + } + function testGas() external { Disbursement[5] memory thirdParty; uint128 total = 1 ether; diff --git a/test/MultiERC721ForERC20.t.sol b/test/MultiERC721ForERC20.t.sol index 62ee0b8..2a84e94 100644 --- a/test/MultiERC721ForERC20.t.sol +++ b/test/MultiERC721ForERC20.t.sol @@ -9,6 +9,7 @@ import {ERC20Test} from "./ERC20Test.t.sol"; import {MultiERC721Token} from "../src/ERC721SwapperLib.sol"; import {MultiERC721ForERC20Swap, IERC20} from "../src/MultiERC721ForERC20/MultiERC721ForERC20Swap.sol"; +import {MultiERC721ForERC20SwapperDeployer} from "../src/MultiERC721ForERC20/MultiERC721ForERC20SwapperDeployer.gen.sol"; import {InsufficientBalance, Disbursement, Parties} from "../src/TypesAndConstants.sol"; import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol"; @@ -58,6 +59,11 @@ contract MultiERC721ForERC20Test is ERC721ForXTest, ERC20Test { return abi.encode(_asSwap(t), salt); } + /// @inheritdoc ERC721ForXTest + function _fillSelector() internal pure override returns (bytes4) { + return MultiERC721ForERC20SwapperDeployer.fill.selector; + } + /// @inheritdoc ERC721ForXTest function _fill(ERC721TestCase memory t) internal override { factory.fill(_asSwap(t), t.base.salt); diff --git a/test/MultiERC721ForNative.t.sol b/test/MultiERC721ForNative.t.sol index b0f1e2e..fb82b2a 100644 --- a/test/MultiERC721ForNative.t.sol +++ b/test/MultiERC721ForNative.t.sol @@ -9,6 +9,8 @@ import {NativeTokenTest} from "./NativeTokenTest.t.sol"; import {MultiERC721Token} from "../src/ERC721SwapperLib.sol"; import {MultiERC721ForNativeSwap} from "../src/MultiERC721ForNative/MultiERC721ForNativeSwap.sol"; +import {MultiERC721ForNativeSwapperDeployer} from + "../src/MultiERC721ForNative/MultiERC721ForNativeSwapperDeployer.gen.sol"; import {InsufficientBalance, Disbursement, Parties} from "../src/TypesAndConstants.sol"; import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol"; @@ -52,6 +54,11 @@ contract MultiERC721ForNativeTest is ERC721ForXTest, NativeTokenTest { return abi.encode(_asSwap(t), salt); } + /// @inheritdoc ERC721ForXTest + function _fillSelector() internal pure override returns (bytes4) { + return MultiERC721ForNativeSwapperDeployer.fill.selector; + } + /// @inheritdoc ERC721ForXTest function _fill(ERC721TestCase memory t) internal override { _fill(t, t.base.native.callValue); diff --git a/test/SwapperTestBase.t.sol b/test/SwapperTestBase.t.sol index 82fb3a7..c5926d0 100644 --- a/test/SwapperTestBase.t.sol +++ b/test/SwapperTestBase.t.sol @@ -16,6 +16,7 @@ import { import {ERC721, IERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol"; +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; contract Token is ERC721 { @@ -28,6 +29,28 @@ contract Token is ERC721 { function exists(uint256 tokenId) external view returns (bool) { return _ownerOf(tokenId) != address(0); } + + /** + * @dev If non-zero, this address is called after transferFrom() to enable testing of reentrancy. Although an attack + * would typically be performed by a Party, the token transfer is the only common function call in all tests so is + * the cleanest way to insert a reentrancy hook. + */ + address private _callPostTransfer; + + bytes private _postTransferCallData; + + function setPostTransferCall(address a, bytes calldata data) public { + _callPostTransfer = a; + _postTransferCallData = data; + } + + function transferFrom(address from, address to, uint256 tokenId) public override { + super.transferFrom(from, to, tokenId); + + if (_callPostTransfer != address(0)) { + Address.functionCall(_callPostTransfer, _postTransferCallData); + } + } } interface ITestEvents is ISwapperEvents {