From 58c3b999acbf3a200c4dd838d1ee3cecd0154f60 Mon Sep 17 00:00:00 2001 From: James Wenzel Date: Fri, 10 Jun 2022 14:17:55 -0700 Subject: [PATCH 1/3] add preapproved token --- lib/murky | 2 +- test/foundry/FulfillOrderTest.t.sol | 6 + test/foundry/token/ERC721.sol | 279 ++++++++++++++++++ .../utils/OfferConsiderationItemAdder.sol | 31 +- test/foundry/utils/TestTokenMinter.sol | 41 +++ 5 files changed, 356 insertions(+), 3 deletions(-) create mode 100644 test/foundry/token/ERC721.sol diff --git a/lib/murky b/lib/murky index fc669746..2caa3b01 160000 --- a/lib/murky +++ b/lib/murky @@ -1 +1 @@ -Subproject commit fc669746449ed6ae0a3dade6dff8bd8487b99354 +Subproject commit 2caa3b01b888a03152cfebfec8acb24eb8036c16 diff --git a/test/foundry/FulfillOrderTest.t.sol b/test/foundry/FulfillOrderTest.t.sol index eeed488e..b6188782 100644 --- a/test/foundry/FulfillOrderTest.t.sol +++ b/test/foundry/FulfillOrderTest.t.sol @@ -107,6 +107,12 @@ contract FulfillOrderTest is BaseOrderTest { _; } + function testNoSpendFromNullAddress() public { + preapproved721.mint(address(0), 1); + addErc721OfferItem(address(preapproved721), 1); + addEthConsiderationItem(alice, 1); + } + function testFulfillAscendingDescendingOffer(FuzzInputsCommon memory inputs) public validateInputs(inputs) diff --git a/test/foundry/token/ERC721.sol b/test/foundry/token/ERC721.sol new file mode 100644 index 00000000..751cebb1 --- /dev/null +++ b/test/foundry/token/ERC721.sol @@ -0,0 +1,279 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity >=0.8.0; + +/// @notice Modern, minimalist, and gas efficient ERC-721 implementation. +/// @author Solmate (https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC721.sol) +/// @notice modified for testing purposes +abstract contract ERC721 { + /*////////////////////////////////////////////////////////////// + EVENTS + //////////////////////////////////////////////////////////////*/ + + event Transfer( + address indexed from, + address indexed to, + uint256 indexed id + ); + + event Approval( + address indexed owner, + address indexed spender, + uint256 indexed id + ); + + event ApprovalForAll( + address indexed owner, + address indexed operator, + bool approved + ); + + /*////////////////////////////////////////////////////////////// + METADATA STORAGE/LOGIC + //////////////////////////////////////////////////////////////*/ + + string public name; + + string public symbol; + + function tokenURI(uint256 id) public view virtual returns (string memory); + + /*////////////////////////////////////////////////////////////// + ERC721 BALANCE/OWNER STORAGE + //////////////////////////////////////////////////////////////*/ + + mapping(uint256 => address) internal _ownerOf; + + mapping(address => uint256) internal _balanceOf; + + function ownerOf(uint256 id) public view virtual returns (address owner) { + require((owner = _ownerOf[id]) != address(0), "NOT_MINTED"); + } + + function balanceOf(address owner) public view virtual returns (uint256) { + require(owner != address(0), "ZERO_ADDRESS"); + + return _balanceOf[owner]; + } + + /*////////////////////////////////////////////////////////////// + ERC721 APPROVAL STORAGE + //////////////////////////////////////////////////////////////*/ + + mapping(uint256 => address) public getApproved; + + mapping(address => mapping(address => bool)) internal _isApprovedForAll; + + /*////////////////////////////////////////////////////////////// + CONSTRUCTOR + //////////////////////////////////////////////////////////////*/ + + constructor(string memory _name, string memory _symbol) { + name = _name; + symbol = _symbol; + } + + /*////////////////////////////////////////////////////////////// + ERC721 LOGIC + //////////////////////////////////////////////////////////////*/ + + function approve(address spender, uint256 id) public virtual { + address owner = _ownerOf[id]; + + require( + msg.sender == owner || isApprovedForAll(owner, msg.sender), + "NOT_AUTHORIZED" + ); + + getApproved[id] = spender; + + emit Approval(owner, spender, id); + } + + function setApprovalForAll(address operator, bool approved) public virtual { + _isApprovedForAll[msg.sender][operator] = approved; + + emit ApprovalForAll(msg.sender, operator, approved); + } + + function isApprovedForAll(address owner, address spender) + public + view + virtual + returns (bool) + { + return _isApprovedForAll[owner][spender]; + } + + function transferFrom( + address from, + address to, + uint256 id + ) public virtual { + require(from == _ownerOf[id], "WRONG_FROM"); + + require( + msg.sender == from || + isApprovedForAll(from, msg.sender) || + msg.sender == getApproved[id], + "NOT_AUTHORIZED" + ); + + // Underflow of the sender's balance is impossible because we check for + // ownership above and the recipient's balance can't realistically overflow. + unchecked { + _balanceOf[from]--; + + _balanceOf[to]++; + } + + _ownerOf[id] = to; + + delete getApproved[id]; + + emit Transfer(from, to, id); + } + + function safeTransferFrom( + address from, + address to, + uint256 id + ) public virtual { + transferFrom(from, to, id); + + require( + to.code.length == 0 || + ERC721TokenReceiver(to).onERC721Received( + msg.sender, + from, + id, + "" + ) == + ERC721TokenReceiver.onERC721Received.selector, + "UNSAFE_RECIPIENT" + ); + } + + function safeTransferFrom( + address from, + address to, + uint256 id, + bytes calldata data + ) public virtual { + transferFrom(from, to, id); + + require( + to.code.length == 0 || + ERC721TokenReceiver(to).onERC721Received( + msg.sender, + from, + id, + data + ) == + ERC721TokenReceiver.onERC721Received.selector, + "UNSAFE_RECIPIENT" + ); + } + + /*////////////////////////////////////////////////////////////// + ERC165 LOGIC + //////////////////////////////////////////////////////////////*/ + + function supportsInterface(bytes4 interfaceId) + public + view + virtual + returns (bool) + { + return + interfaceId == 0x01ffc9a7 || // ERC165 Interface ID for ERC165 + interfaceId == 0x80ac58cd || // ERC165 Interface ID for ERC721 + interfaceId == 0x5b5e139f; // ERC165 Interface ID for ERC721Metadata + } + + /*////////////////////////////////////////////////////////////// + INTERNAL MINT/BURN LOGIC + //////////////////////////////////////////////////////////////*/ + + function _mint(address to, uint256 id) internal virtual { + require(_ownerOf[id] == address(0), "ALREADY_MINTED"); + + // Counter overflow is incredibly unrealistic. + unchecked { + _balanceOf[to]++; + } + + _ownerOf[id] = to; + + emit Transfer(address(0), to, id); + } + + function _burn(uint256 id) internal virtual { + address owner = _ownerOf[id]; + + require(owner != address(0), "NOT_MINTED"); + + // Ownership check above ensures no underflow. + unchecked { + _balanceOf[owner]--; + } + + delete _ownerOf[id]; + + delete getApproved[id]; + + emit Transfer(owner, address(0), id); + } + + /*////////////////////////////////////////////////////////////// + INTERNAL SAFE MINT LOGIC + //////////////////////////////////////////////////////////////*/ + + function _safeMint(address to, uint256 id) internal virtual { + _mint(to, id); + + require( + to.code.length == 0 || + ERC721TokenReceiver(to).onERC721Received( + msg.sender, + address(0), + id, + "" + ) == + ERC721TokenReceiver.onERC721Received.selector, + "UNSAFE_RECIPIENT" + ); + } + + function _safeMint( + address to, + uint256 id, + bytes memory data + ) internal virtual { + _mint(to, id); + + require( + to.code.length == 0 || + ERC721TokenReceiver(to).onERC721Received( + msg.sender, + address(0), + id, + data + ) == + ERC721TokenReceiver.onERC721Received.selector, + "UNSAFE_RECIPIENT" + ); + } +} + +/// @notice A generic interface for a contract which properly accepts ERC721 tokens. +/// @author Solmate (https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC721.sol) +abstract contract ERC721TokenReceiver { + function onERC721Received( + address, + address, + uint256, + bytes calldata + ) external virtual returns (bytes4) { + return ERC721TokenReceiver.onERC721Received.selector; + } +} diff --git a/test/foundry/utils/OfferConsiderationItemAdder.sol b/test/foundry/utils/OfferConsiderationItemAdder.sol index 683b8e03..989378b9 100644 --- a/test/foundry/utils/OfferConsiderationItemAdder.sol +++ b/test/foundry/utils/OfferConsiderationItemAdder.sol @@ -52,8 +52,35 @@ contract OfferConsiderationItemAdder is TestTokenMinter { addOfferItem(itemType, identifier, amt, amt); } - function addErc721OfferItem(uint256 tokenId) internal { - addOfferItem(ItemType.ERC721, address(test721_1), tokenId, 1, 1); + function addErc721OfferItem(uint256 identifier) internal { + addErc721OfferItem(address(test721_1), identifier); + } + + function addErc721OfferItem(address token, uint256 identifier) internal { + addErc721OfferItem(token, identifier, 1, 1); + } + + function addErc721OfferItem( + address token, + uint256 identifier, + uint256 amount + ) internal { + addErc721OfferItem(token, identifier, amount, amount); + } + + function addErc721OfferItem( + address token, + uint256 identifier, + uint256 startAmount, + uint256 endAmount + ) internal { + addOfferItem( + ItemType.ERC721, + token, + identifier, + startAmount, + endAmount + ); } function addErc1155OfferItem(uint256 tokenId, uint256 amount) internal { diff --git a/test/foundry/utils/TestTokenMinter.sol b/test/foundry/utils/TestTokenMinter.sol index 6543bf40..367ab15f 100644 --- a/test/foundry/utils/TestTokenMinter.sol +++ b/test/foundry/utils/TestTokenMinter.sol @@ -8,6 +8,36 @@ import { ERC721Recipient } from "./ERC721Recipient.sol"; import { ERC1155Recipient } from "./ERC1155Recipient.sol"; import { ItemType } from "../../../contracts/lib/ConsiderationEnums.sol"; import { BaseConsiderationTest } from "./BaseConsiderationTest.sol"; +import { ERC721 } from "../token/ERC721.sol"; + +contract PreapprovedERC721 is ERC721 { + mapping(address => bool) preapprovals; + + constructor(address[] memory preapproved) ERC721("", "") { + for (uint256 i = 0; i < preapproved.length; i++) { + preapprovals[preapproved[i]] = true; + } + } + + function mint(address to, uint256 amount) external returns (bool) { + _mint(to, amount); + return true; + } + + function isApprovedForAll(address owner, address operator) + public + view + override + returns (bool) + { + return + preapprovals[operator] || super.isApprovedForAll(owner, operator); + } + + function tokenURI(uint256) public pure override returns (string memory) { + return ""; + } +} contract TestTokenMinter is BaseConsiderationTest, @@ -30,6 +60,7 @@ contract TestTokenMinter is TestERC721 internal test721_1; TestERC721 internal test721_2; TestERC721 internal test721_3; + PreapprovedERC721 internal preapproved721; TestERC1155 internal test1155_1; TestERC1155 internal test1155_2; @@ -39,6 +70,13 @@ contract TestTokenMinter is TestERC721[] erc721s; TestERC1155[] erc1155s; + address[] preapprovals = [ + address(consideration), + address(referenceConsideration), + address(conduit), + address(referenceConduit) + ]; + modifier only1155Receiver(address recipient) { vm.assume(recipient != address(0)); if (recipient.code.length > 0) { @@ -174,9 +212,12 @@ contract TestTokenMinter is test1155_1 = new TestERC1155(); test1155_2 = new TestERC1155(); test1155_3 = new TestERC1155(); + preapproved721 = new PreapprovedERC721(preapprovals); + vm.label(address(token1), "token1"); vm.label(address(test721_1), "test721_1"); vm.label(address(test1155_1), "test1155_1"); + vm.label(address(preapproved721), "preapproved721"); emit log("Deployed test token contracts"); } From ac351500e9b6bf8f887f02533f42f2e79b29d8b7 Mon Sep 17 00:00:00 2001 From: James Wenzel Date: Fri, 10 Jun 2022 15:12:40 -0700 Subject: [PATCH 2/3] include test for spend against null address with invalid signature; fix unused param in _performTestFulfillOrderRevertInvalidArrayLength --- test/foundry/FulfillOrderTest.t.sol | 32 +++++++++++++++++++++++++- test/foundry/utils/BaseOrderTest.sol | 4 ++-- test/foundry/utils/TestTokenMinter.sol | 17 ++++++++------ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/test/foundry/FulfillOrderTest.t.sol b/test/foundry/FulfillOrderTest.t.sol index b6188782..d998d41e 100644 --- a/test/foundry/FulfillOrderTest.t.sol +++ b/test/foundry/FulfillOrderTest.t.sol @@ -108,9 +108,39 @@ contract FulfillOrderTest is BaseOrderTest { } function testNoSpendFromNullAddress() public { + // mint token to null address preapproved721.mint(address(0), 1); + // mint erc token to test address + token1.mint(address(this), 1); + // offer burnt erc721 addErc721OfferItem(address(preapproved721), 1); - addEthConsiderationItem(alice, 1); + // consider erc20 to null address + addErc20ConsiderationItem(payable(0), 1); + // configure baseOrderParameters with null address as offerer + configureOrderParameters(address(0)); + test( + this.noSpendFromNullAddress, + Context(referenceConsideration, empty, 0, 0, 0) + ); + test( + this.noSpendFromNullAddress, + Context(consideration, empty, 0, 0, 0) + ); + } + + function noSpendFromNullAddress(Context memory context) external stateless { + // create a bad signature + bytes memory signature = abi.encodePacked( + bytes32(0), + bytes32(0), + bytes1(uint8(27)) + ); + // test that signature is recognized as invalid even though signer recovered is null address + vm.expectRevert(abi.encodeWithSignature("InvalidSigner()")); + context.consideration.fulfillOrder( + Order(baseOrderParameters, signature), + bytes32(0) + ); } function testFulfillAscendingDescendingOffer(FuzzInputsCommon memory inputs) diff --git a/test/foundry/utils/BaseOrderTest.sol b/test/foundry/utils/BaseOrderTest.sol index ab37bf82..084e43dc 100644 --- a/test/foundry/utils/BaseOrderTest.sol +++ b/test/foundry/utils/BaseOrderTest.sol @@ -219,7 +219,7 @@ contract BaseOrderTest is OfferConsiderationItemAdder, AmountDeriver { uint256 originalItemsLength, uint256 amtToSubtractFromItemsLength ) internal { - assertTrue(_validateOrder(order, consideration)); + assertTrue(_validateOrder(order, _consideration)); bool overwriteItemsLength = amtToSubtractFromItemsLength > 0; if (overwriteItemsLength) { @@ -251,7 +251,7 @@ contract BaseOrderTest is OfferConsiderationItemAdder, AmountDeriver { ); bool success = _callConsiderationFulfillOrderWithCalldata( - address(consideration), + address(_consideration), fulfillOrderCalldata ); diff --git a/test/foundry/utils/TestTokenMinter.sol b/test/foundry/utils/TestTokenMinter.sol index 367ab15f..623d63aa 100644 --- a/test/foundry/utils/TestTokenMinter.sol +++ b/test/foundry/utils/TestTokenMinter.sol @@ -11,7 +11,7 @@ import { BaseConsiderationTest } from "./BaseConsiderationTest.sol"; import { ERC721 } from "../token/ERC721.sol"; contract PreapprovedERC721 is ERC721 { - mapping(address => bool) preapprovals; + mapping(address => bool) public preapprovals; constructor(address[] memory preapproved) ERC721("", "") { for (uint256 i = 0; i < preapproved.length; i++) { @@ -70,12 +70,7 @@ contract TestTokenMinter is TestERC721[] erc721s; TestERC1155[] erc1155s; - address[] preapprovals = [ - address(consideration), - address(referenceConsideration), - address(conduit), - address(referenceConduit) - ]; + address[] preapprovals; modifier only1155Receiver(address recipient) { vm.assume(recipient != address(0)); @@ -99,6 +94,14 @@ contract TestTokenMinter is function setUp() public virtual override { super.setUp(); + + preapprovals = [ + address(consideration), + address(referenceConsideration), + address(conduit), + address(referenceConduit) + ]; + vm.label(alice, "alice"); vm.label(bob, "bob"); vm.label(cal, "cal"); From 250dc701771a8ae20e95b36440ef4443e725b718 Mon Sep 17 00:00:00 2001 From: James Wenzel Date: Fri, 10 Jun 2022 18:54:26 -0700 Subject: [PATCH 3/3] rename test for clarity --- test/foundry/FulfillOrderTest.t.sol | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/test/foundry/FulfillOrderTest.t.sol b/test/foundry/FulfillOrderTest.t.sol index d998d41e..98d949a9 100644 --- a/test/foundry/FulfillOrderTest.t.sol +++ b/test/foundry/FulfillOrderTest.t.sol @@ -107,7 +107,7 @@ contract FulfillOrderTest is BaseOrderTest { _; } - function testNoSpendFromNullAddress() public { + function testNullAddressSpendReverts() public { // mint token to null address preapproved721.mint(address(0), 1); // mint erc token to test address @@ -119,16 +119,19 @@ contract FulfillOrderTest is BaseOrderTest { // configure baseOrderParameters with null address as offerer configureOrderParameters(address(0)); test( - this.noSpendFromNullAddress, + this.nullAddressSpendReverts, Context(referenceConsideration, empty, 0, 0, 0) ); test( - this.noSpendFromNullAddress, + this.nullAddressSpendReverts, Context(consideration, empty, 0, 0, 0) ); } - function noSpendFromNullAddress(Context memory context) external stateless { + function nullAddressSpendReverts(Context memory context) + external + stateless + { // create a bad signature bytes memory signature = abi.encodePacked( bytes32(0),