From c659408a0443236a58528b9fa427c512c2a84cae Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Mon, 22 Jul 2024 21:03:13 +0100 Subject: [PATCH 1/3] perf: write `Create2` revert data to the beginning of memory --- src/Create2.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Create2.sol b/src/Create2.sol index 5b8a0db..22bd6ab 100644 --- a/src/Create2.sol +++ b/src/Create2.sol @@ -23,15 +23,15 @@ library Create2 { } assembly ("memory-safe") { - let free := mload(0x40) - if iszero(returndatasize()) { - mstore(free, 0x33d2bae4) // Create2EmptyRevert() - revert(add(free, 28), 4) + mstore(0, 0x33d2bae4) // Create2EmptyRevert() + revert(28, 4) } - returndatacopy(free, 0, returndatasize()) - revert(free, returndatasize()) + // Even though this may extend beyond scratch space, we revert the current context immediately so it's still + // memory-safe. + returndatacopy(0, 0, returndatasize()) + revert(0, returndatasize()) } } } From a989deca0a7084347ddbee608000d2894a9d09cb Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Mon, 22 Jul 2024 21:22:14 +0100 Subject: [PATCH 2/3] perf: assembly for all of `ERC721TransferLib._transfer()` with reusable data --- src/ERC721TransferLib.sol | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/ERC721TransferLib.sol b/src/ERC721TransferLib.sol index df33450..144d624 100644 --- a/src/ERC721TransferLib.sol +++ b/src/ERC721TransferLib.sol @@ -65,24 +65,19 @@ library ERC721TransferLib { address addr = address(token.addr); uint256[] memory ids = token.ids; - uint256 idSrc; - uint256 idDst; assembly ("memory-safe") { - idSrc := add(ids, 0x20) - idDst := add(reusableCallData, 0x64) - } + let idSrc := add(ids, 0x20) + let idDst := add(reusableCallData, 0x64) + let dataPtr := add(reusableCallData, 0x20) - for (uint256 end = idSrc + ids.length * 0x20; idSrc < end; idSrc += 0x20) { - assembly ("memory-safe") { + for { let end := add(idSrc, mul(mload(ids), 0x20)) } lt(idSrc, end) { idSrc := add(idSrc, 0x20) } { mcopy(idDst, idSrc, 0x20) - } - (bool success,) = addr.call(reusableCallData); - if (!success) { - assembly ("memory-safe") { - let free := mload(0x40) - returndatacopy(free, 0, returndatasize()) - revert(free, returndatasize()) + if iszero(call(gas(), addr, 0, dataPtr, reusableCallData, 0, 0)) { + // Even though this may extend beyond scratch space, we revert the current context immediately so + // it's still memory-safe. + returndatacopy(0, 0, returndatasize()) + revert(0, returndatasize()) } } } From 6eaf0593f6a00b46c609139bf5a88f11801fc8cd Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Tue, 23 Jul 2024 21:09:46 +0100 Subject: [PATCH 3/3] feat: `ERC721TransferLib._safeTransfer()` variants --- src/ERC721TransferLib.sol | 42 +++++++++++++- test/ERC721TransferLib.t.sol | 103 ++++++++++++++++++++++++++++++++++- 2 files changed, 142 insertions(+), 3 deletions(-) diff --git a/src/ERC721TransferLib.sol b/src/ERC721TransferLib.sol index 144d624..afe0e67 100644 --- a/src/ERC721TransferLib.sol +++ b/src/ERC721TransferLib.sol @@ -23,6 +23,11 @@ library ERC721TransferLib { token.addr.transferFrom(parties.seller, parties.buyer, token.id); } + /// @dev Transfers the token from `parties.seller` to `parties.buyer` using `safeTransferFrom()`. + function _safeTransfer(ERC721Token memory token, Parties memory parties, bytes memory data) internal { + token.addr.safeTransferFrom(parties.seller, parties.buyer, token.id, data); + } + /// @dev Represents multiple ERC721 tokens within the same contract. struct MultiERC721Token { IERC721 addr; @@ -37,6 +42,14 @@ library ERC721TransferLib { _transfer(tokens, _reusableTransferCallData(parties)); } + /** + * @dev Transfers all tokens from `parties.seller` to `parties.buyer` using `safeTransferFrom()`. + * @param tokens Any number of ERC721 tokens from a single contract; ids MUST be distinct across the entire array. + */ + function _safeTransfer(MultiERC721Token memory tokens, Parties memory parties, bytes memory data) internal { + _transfer(tokens, _reusableSafeTransferCallData(parties, data)); + } + /** * @dev Transfers all tokens from `parties.seller` to `parties.buyer`. * @param tokens An _array_ of MultiERC721Token structs, representing any number of tokens. {addr,id} pairs MUST be @@ -49,6 +62,18 @@ library ERC721TransferLib { } } + /** + * @dev Transfers all tokens from `parties.seller` to `parties.buyer` using `safeTransferFrom()`. + * @param tokens An _array_ of MultiERC721Token structs, representing any number of tokens. {addr,id} pairs MUST be + * distinct across the entire array. + */ + function _safeTransfer(MultiERC721Token[] memory tokens, Parties memory parties, bytes memory data) internal { + bytes memory callData = _reusableSafeTransferCallData(parties, data); + for (uint256 i = 0; i < tokens.length; ++i) { + _transfer(tokens[i], callData); + } + } + /** * @dev Returns calldata for `ERC721.transferFrom(parties.seller, parties.buyer, 0)`, which can be reused to avoid * unnecessary memory expansion when transferring large numbers of tokens. @@ -57,9 +82,24 @@ library ERC721TransferLib { return abi.encodeCall(IERC721.transferFrom, (parties.seller, parties.buyer, 0)); } + /** + * @dev Returns calldata for `ERC721.safeTransferFrom(parties.seller, parties.buyer, 0, data)`. See + * `_reusableTransferCallData()` for rationale. + */ + function _reusableSafeTransferCallData(Parties memory parties, bytes memory data) + private + pure + returns (bytes memory) + { + // Since `safeTransferFrom()` has an overload, we can't use `abi.encodeCall()` as it's ambiguous. + return abi.encodeWithSignature( + "safeTransferFrom(address,address,uint256,bytes)", parties.seller, parties.buyer, 0, data + ); + } + /** * @param token Contract and IDs of tokens to be transferred. - * @param reusableCallData Output of `_reusableTransferCallData()`. + * @param reusableCallData Output of `_reusableTransferCallData()` or `_reusableSafeTransferCallData()`. */ function _transfer(MultiERC721Token memory token, bytes memory reusableCallData) private { address addr = address(token.addr); diff --git a/test/ERC721TransferLib.t.sol b/test/ERC721TransferLib.t.sol index 3ed69c3..7f52334 100644 --- a/test/ERC721TransferLib.t.sol +++ b/test/ERC721TransferLib.t.sol @@ -9,6 +9,16 @@ import {ERC721TransferLib, IERC721} from "../src/ERC721TransferLib.sol"; import {Parties} from "../src/TypesAndConstants.sol"; import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol"; +import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; + +contract ERC721Receiver is IERC721Receiver { + mapping(uint256 => bytes) public tokenData; + + function onERC721Received(address, address, uint256 tokenId, bytes calldata data) external returns (bytes4) { + tokenData[tokenId] = data; + return IERC721Receiver.onERC721Received.selector; + } +} contract ERC721TransferLibTest is Test, ITestEvents { using ERC721TransferLib for *; @@ -23,10 +33,18 @@ contract ERC721TransferLibTest is Test, ITestEvents { uint256[NUM_CONTRACTS][TOKENS_PER_CONTRACT] calldata ids, Parties memory parties ) public { + vm.assume(parties.buyer.code.length == 0); // otherwise safeTransferFrom() will fail + ERC721TransferLib.MultiERC721Token[] memory tokens = _testSetup(deploySalts, ids, parties); - function(ERC721TransferLib.MultiERC721Token[] memory, Parties memory)[3] memory funcs = - [_transferBatch, _transferPerContract, _transferIndividually]; + function(ERC721TransferLib.MultiERC721Token[] memory, Parties memory)[6] memory funcs = [ + _transferBatch, + _safeTransferBatch, + _transferPerContract, + _safeTransferPerContract, + _transferIndividually, + _safeTransferIndividually + ]; for (uint256 i = 0; i < funcs.length; ++i) { uint256 snap = vm.snapshot(); @@ -45,6 +63,19 @@ contract ERC721TransferLibTest is Test, ITestEvents { tokens._transfer(parties); } + function _safeTransferBatch(ERC721TransferLib.MultiERC721Token[] memory tokens, Parties memory parties) internal { + _safeTransferBatchWithData(tokens, parties, ""); + } + + /// @dev The `WithData` suffix avoids overloading so the functions can be placed in arrays for test suites. + function _safeTransferBatchWithData( + ERC721TransferLib.MultiERC721Token[] memory tokens, + Parties memory parties, + bytes memory data + ) internal { + tokens._safeTransfer(parties, data); + } + function _transferPerContract(ERC721TransferLib.MultiERC721Token[] memory tokens, Parties memory parties) internal { @@ -53,6 +84,22 @@ contract ERC721TransferLibTest is Test, ITestEvents { } } + function _safeTransferPerContract(ERC721TransferLib.MultiERC721Token[] memory tokens, Parties memory parties) + internal + { + _safeTransferPerContractWithData(tokens, parties, ""); + } + + function _safeTransferPerContractWithData( + ERC721TransferLib.MultiERC721Token[] memory tokens, + Parties memory parties, + bytes memory data + ) internal { + for (uint256 i = 0; i < tokens.length; ++i) { + tokens[i]._safeTransfer(parties, data); + } + } + function _transferIndividually(ERC721TransferLib.MultiERC721Token[] memory tokens, Parties memory parties) internal { @@ -63,6 +110,24 @@ contract ERC721TransferLibTest is Test, ITestEvents { } } + function _safeTransferIndividually(ERC721TransferLib.MultiERC721Token[] memory tokens, Parties memory parties) + internal + { + _safeTransferIndividuallyWithData(tokens, parties, ""); + } + + function _safeTransferIndividuallyWithData( + ERC721TransferLib.MultiERC721Token[] memory tokens, + Parties memory parties, + bytes memory data + ) internal { + for (uint256 i = 0; i < tokens.length; ++i) { + for (uint256 j = 0; j < tokens[i].ids.length; ++j) { + ERC721TransferLib.ERC721Token({addr: tokens[i].addr, id: tokens[i].ids[j]})._safeTransfer(parties, data); + } + } + } + function testGas( bytes32[NUM_CONTRACTS] calldata deploySalts, uint256[NUM_CONTRACTS][TOKENS_PER_CONTRACT] calldata ids, @@ -154,6 +219,40 @@ contract ERC721TransferLibTest is Test, ITestEvents { tokens._transfer(parties); } + function testSafeTransferDataPropagation(uint256 tokenId, address seller, bytes32 buyerSalt, bytes memory data) + external + { + vm.assume(seller != address(0)); + + ERC721Receiver buyer = new ERC721Receiver{salt: buyerSalt}(); + Parties memory parties = Parties({seller: seller, buyer: address(buyer)}); + + Token t = new Token(); + t.mint(seller, tokenId); + + ERC721TransferLib.MultiERC721Token[] memory tokens = new ERC721TransferLib.MultiERC721Token[](1); + tokens[0].addr = IERC721(t); + tokens[0].ids = new uint256[](1); + tokens[0].ids[0] = tokenId; + + function(ERC721TransferLib.MultiERC721Token[] memory, Parties memory, bytes memory)[3] memory funcs = + [_safeTransferBatchWithData, _safeTransferPerContractWithData, _safeTransferIndividuallyWithData]; + + for (uint256 i = 0; i < funcs.length; ++i) { + uint256 snap = vm.snapshot(); + _assertOwner(tokens, parties.seller); + assertEq(buyer.tokenData(tokenId), "", "safeTransfer() data recorder empty"); + + vm.startPrank(parties.seller); + funcs[i](tokens, parties, data); + vm.stopPrank(); + + _assertOwner(tokens, parties.buyer); + assertEq(buyer.tokenData(tokenId), data, "safeTransfer() data propagated"); + vm.revertTo(snap); + } + } + function testErrorPropagation(uint256 tokenId, Parties memory parties) public { // The implementation uses a direct call() and propagates failures with assembly so needs to be tested.