Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ERC721TransferLib._safeTransfer() variants (Finding 16) #46

Merged
merged 5 commits into from
Aug 4, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion src/ERC721TransferLib.sol
Original file line number Diff line number Diff line change
@@ -26,6 +26,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;
@@ -40,6 +45,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
@@ -52,6 +65,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.
@@ -60,9 +85,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);
103 changes: 101 additions & 2 deletions test/ERC721TransferLib.t.sol
Original file line number Diff line number Diff line change
@@ -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 ERC721Transferrer {
function transfer(ERC721TransferLib.MultiERC721Token[] memory tokens, Parties memory parties) external {
@@ -29,10 +39,18 @@ contract ERC721TransferLibTest is Test, ITestEvents {
uint256[TOKENS_PER_CONTRACT][NUM_CONTRACTS] 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();
@@ -51,6 +69,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
{
@@ -59,6 +90,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
{
@@ -69,6 +116,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[TOKENS_PER_CONTRACT][NUM_CONTRACTS] calldata ids,
@@ -160,6 +225,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 testNoCodeAtTokenAddress(
bytes32[NUM_CONTRACTS] calldata deploySalts,
uint256[TOKENS_PER_CONTRACT][NUM_CONTRACTS] calldata ids,