fix: confirm that token address actually has code before calling `tra…

…nsfer()` (#39)

src/ERC721TransferLib.sol

@@ -12,6 +12,9 @@ import {Parties} from "./TypesAndConstants.sol";
  * exploited by the `TMPL/SwapperBase.sol.tmpl` template.
  */
 library ERC721TransferLib {
+    /// @dev Thrown if a token address doesn't contain any code.
+    error NoCodeAtAddress(address);
+
     /// @dev Represents a single ERC721 token.
     struct ERC721Token {
         IERC721 addr;
@@ -63,6 +66,9 @@ library ERC721TransferLib {
      */
     function _transfer(MultiERC721Token memory token, bytes memory reusableCallData) private {
         address addr = address(token.addr);
+        if (addr.code.length == 0) {
+            revert NoCodeAtAddress(addr);
+        }
         uint256[] memory ids = token.ids;
 
         uint256 idSrc;

test/ERC721TransferLib.t.sol

@@ -10,17 +10,23 @@ import {Parties} from "../src/TypesAndConstants.sol";
 
 import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
 
+contract ERC721Transferrer {
+    function transfer(ERC721TransferLib.MultiERC721Token[] memory tokens, Parties memory parties) external {
+        ERC721TransferLib._transfer(tokens, parties);
+    }
+}
+
 contract ERC721TransferLibTest is Test, ITestEvents {
     using ERC721TransferLib for *;
 
     address public tokenTemplate = address(new Token());
 
     uint256 constant NUM_CONTRACTS = 5;
-    uint256 constant TOKENS_PER_CONTRACT = 5;
+    uint256 constant TOKENS_PER_CONTRACT = 10;
 
     function testERC721TokenTransfers(
         bytes32[NUM_CONTRACTS] calldata deploySalts,
-        uint256[NUM_CONTRACTS][TOKENS_PER_CONTRACT] calldata ids,
+        uint256[TOKENS_PER_CONTRACT][NUM_CONTRACTS] calldata ids,
         Parties memory parties
     ) public {
         ERC721TransferLib.MultiERC721Token[] memory tokens = _testSetup(deploySalts, ids, parties);
@@ -65,7 +71,7 @@ contract ERC721TransferLibTest is Test, ITestEvents {
 
     function testGas(
         bytes32[NUM_CONTRACTS] calldata deploySalts,
-        uint256[NUM_CONTRACTS][TOKENS_PER_CONTRACT] calldata ids,
+        uint256[TOKENS_PER_CONTRACT][NUM_CONTRACTS] calldata ids,
         Parties memory parties
     ) public {
         ERC721TransferLib.MultiERC721Token[] memory tokens = _testSetup(deploySalts, ids, parties);
@@ -97,17 +103,17 @@ contract ERC721TransferLibTest is Test, ITestEvents {
             libGas -= gasleft();
         }
 
-        // 137 gas per token was found empirically; it has no special meaning other than to demonstrate the gas saving
-        // in this particular instance and may be a change-detector test.
-        assertLe(libGas + 137 * NUM_CONTRACTS * TOKENS_PER_CONTRACT, naiveGas);
+        // Gas saving per token was found empirically; it has no special meaning other than to demonstrate the saving
+        // in this specific instance and may be a change-detector test.
+        assertLe(libGas + 147 * NUM_CONTRACTS * TOKENS_PER_CONTRACT, naiveGas);
         console2.log(naiveGas - libGas);
     }
 
     mapping(bytes32 => bool) private _saltSeen;
 
     function _testSetup(
         bytes32[NUM_CONTRACTS] calldata deploySalts,
-        uint256[NUM_CONTRACTS][TOKENS_PER_CONTRACT] calldata ids,
+        uint256[TOKENS_PER_CONTRACT][NUM_CONTRACTS] calldata ids,
         Parties memory parties
     ) internal returns (ERC721TransferLib.MultiERC721Token[] memory) {
         vm.assume(parties.seller != parties.buyer);
@@ -126,7 +132,7 @@ contract ERC721TransferLibTest is Test, ITestEvents {
 
             Token t = new Token{salt: deploySalts[i]}();
             tokens[i].addr = t;
-            tokens[i].ids = new uint256[](ids[i].length);
+            tokens[i].ids = new uint256[](TOKENS_PER_CONTRACT);
 
             for (uint256 j = 0; j < TOKENS_PER_CONTRACT; ++j) {
                 uint256 id = ids[i][j];
@@ -148,12 +154,40 @@ contract ERC721TransferLibTest is Test, ITestEvents {
         }
     }
 
-    function testMultiERC721TokenTransferNothing(address tokenContract, Parties memory parties) public {
+    function testMultiERC721TokenTransferNothing(bytes32 tokenDeploySalt, Parties memory parties) public {
         ERC721TransferLib.MultiERC721Token[] memory tokens = new ERC721TransferLib.MultiERC721Token[](1);
-        tokens[0].addr = IERC721(tokenContract);
+        tokens[0].addr = new Token{salt: tokenDeploySalt}();
         tokens._transfer(parties);
     }
 
+    function testNoCodeAtTokenAddress(
+        bytes32[NUM_CONTRACTS] calldata deploySalts,
+        uint256[TOKENS_PER_CONTRACT][NUM_CONTRACTS] calldata ids,
+        address emptyTokenContract,
+        uint256 contractToEmpty,
+        Parties memory parties
+    ) public {
+        vm.assume(emptyTokenContract.code.length == 0);
+
+        ERC721TransferLib.MultiERC721Token[] memory tokens = _testSetup(deploySalts, ids, parties);
+
+        // When using vm.expectRevert(), it expects the very next external call to be the one that reverts, but that
+        // won't be the case when using an internal library function. We therefore have to have a proxy contract use the
+        // library as a means of wrapping all transfers into a single (reverting) call.
+        ERC721Transferrer proxy = new ERC721Transferrer();
+        for (uint256 i = 0; i < tokens.length; ++i) {
+            vm.prank(parties.seller);
+            tokens[i].addr.setApprovalForAll(address(proxy), true);
+        }
+
+        // By only clearing the contract now, the approval loop above is much cleaner.
+        tokens[bound(contractToEmpty, 0, tokens.length - 1)].addr = IERC721(emptyTokenContract);
+
+        vm.expectRevert(abi.encodeWithSelector(ERC721TransferLib.NoCodeAtAddress.selector, emptyTokenContract));
+        proxy.transfer(tokens, parties);
+        vm.stopPrank();
+    }
+
     function testErrorPropagation(uint256 tokenId, Parties memory parties) public {
         // The implementation uses a direct call() and propagates failures with assembly so needs to be tested.