Skip to content

Commit 06bf918

Browse files
0age0age
authored
Merge pull request ProjectOpenSea#502 from ProjectOpenSea/test-null-spend
Add Foundry test for failures when spending pre-approved items from null address with invalid signature
2 parents 1a65f0e + 250dc70 commit 06bf918

File tree

5 files changed

+393
-4
lines changed

5 files changed

+393
-4
lines changed

‎test/foundry/FulfillOrderTest.t.sol

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,45 @@ contract FulfillOrderTest is BaseOrderTest {
109109
_;
110110
}
111111

112+
function testNullAddressSpendReverts() public {
113+
// mint token to null address
114+
preapproved721.mint(address(0), 1);
115+
// mint erc token to test address
116+
token1.mint(address(this), 1);
117+
// offer burnt erc721
118+
addErc721OfferItem(address(preapproved721), 1);
119+
// consider erc20 to null address
120+
addErc20ConsiderationItem(payable(0), 1);
121+
// configure baseOrderParameters with null address as offerer
122+
configureOrderParameters(address(0));
123+
test(
124+
this.nullAddressSpendReverts,
125+
Context(referenceConsideration, empty, 0, 0, 0)
126+
);
127+
test(
128+
this.nullAddressSpendReverts,
129+
Context(consideration, empty, 0, 0, 0)
130+
);
131+
}
132+
133+
function nullAddressSpendReverts(Context memory context)
134+
external
135+
stateless
136+
{
137+
// create a bad signature
138+
bytes memory signature = abi.encodePacked(
139+
bytes32(0),
140+
bytes32(0),
141+
bytes1(uint8(27))
142+
);
143+
// test that signature is recognized as invalid even though signer recovered is null address
144+
vm.expectRevert(abi.encodeWithSignature("InvalidSigner()"));
145+
context.consideration.fulfillOrder(
146+
Order(baseOrderParameters, signature),
147+
bytes32(0)
148+
);
149+
}
150+
112151
function testFulfillAscendingDescendingOffer(FuzzInputsCommon memory inputs)
113152
public
114153
validateInputs(inputs)

‎test/foundry/token/ERC721.sol

Lines changed: 279 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,279 @@
1+
// SPDX-License-Identifier: AGPL-3.0-only
2+
pragma solidity >=0.8.0;
3+
4+
/// @notice Modern, minimalist, and gas efficient ERC-721 implementation.
5+
/// @author Solmate (https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC721.sol)
6+
/// @notice modified for testing purposes
7+
abstract contract ERC721 {
8+
/*//////////////////////////////////////////////////////////////
9+
EVENTS
10+
//////////////////////////////////////////////////////////////*/
11+
12+
event Transfer(
13+
address indexed from,
14+
address indexed to,
15+
uint256 indexed id
16+
);
17+
18+
event Approval(
19+
address indexed owner,
20+
address indexed spender,
21+
uint256 indexed id
22+
);
23+
24+
event ApprovalForAll(
25+
address indexed owner,
26+
address indexed operator,
27+
bool approved
28+
);
29+
30+
/*//////////////////////////////////////////////////////////////
31+
METADATA STORAGE/LOGIC
32+
//////////////////////////////////////////////////////////////*/
33+
34+
string public name;
35+
36+
string public symbol;
37+
38+
function tokenURI(uint256 id) public view virtual returns (string memory);
39+
40+
/*//////////////////////////////////////////////////////////////
41+
ERC721 BALANCE/OWNER STORAGE
42+
//////////////////////////////////////////////////////////////*/
43+
44+
mapping(uint256 => address) internal _ownerOf;
45+
46+
mapping(address => uint256) internal _balanceOf;
47+
48+
function ownerOf(uint256 id) public view virtual returns (address owner) {
49+
require((owner = _ownerOf[id]) != address(0), "NOT_MINTED");
50+
}
51+
52+
function balanceOf(address owner) public view virtual returns (uint256) {
53+
require(owner != address(0), "ZERO_ADDRESS");
54+
55+
return _balanceOf[owner];
56+
}
57+
58+
/*//////////////////////////////////////////////////////////////
59+
ERC721 APPROVAL STORAGE
60+
//////////////////////////////////////////////////////////////*/
61+
62+
mapping(uint256 => address) public getApproved;
63+
64+
mapping(address => mapping(address => bool)) internal _isApprovedForAll;
65+
66+
/*//////////////////////////////////////////////////////////////
67+
CONSTRUCTOR
68+
//////////////////////////////////////////////////////////////*/
69+
70+
constructor(string memory _name, string memory _symbol) {
71+
name = _name;
72+
symbol = _symbol;
73+
}
74+
75+
/*//////////////////////////////////////////////////////////////
76+
ERC721 LOGIC
77+
//////////////////////////////////////////////////////////////*/
78+
79+
function approve(address spender, uint256 id) public virtual {
80+
address owner = _ownerOf[id];
81+
82+
require(
83+
msg.sender == owner || isApprovedForAll(owner, msg.sender),
84+
"NOT_AUTHORIZED"
85+
);
86+
87+
getApproved[id] = spender;
88+
89+
emit Approval(owner, spender, id);
90+
}
91+
92+
function setApprovalForAll(address operator, bool approved) public virtual {
93+
_isApprovedForAll[msg.sender][operator] = approved;
94+
95+
emit ApprovalForAll(msg.sender, operator, approved);
96+
}
97+
98+
function isApprovedForAll(address owner, address spender)
99+
public
100+
view
101+
virtual
102+
returns (bool)
103+
{
104+
return _isApprovedForAll[owner][spender];
105+
}
106+
107+
function transferFrom(
108+
address from,
109+
address to,
110+
uint256 id
111+
) public virtual {
112+
require(from == _ownerOf[id], "WRONG_FROM");
113+
114+
require(
115+
msg.sender == from ||
116+
isApprovedForAll(from, msg.sender) ||
117+
msg.sender == getApproved[id],
118+
"NOT_AUTHORIZED"
119+
);
120+
121+
// Underflow of the sender's balance is impossible because we check for
122+
// ownership above and the recipient's balance can't realistically overflow.
123+
unchecked {
124+
_balanceOf[from]--;
125+
126+
_balanceOf[to]++;
127+
}
128+
129+
_ownerOf[id] = to;
130+
131+
delete getApproved[id];
132+
133+
emit Transfer(from, to, id);
134+
}
135+
136+
function safeTransferFrom(
137+
address from,
138+
address to,
139+
uint256 id
140+
) public virtual {
141+
transferFrom(from, to, id);
142+
143+
require(
144+
to.code.length == 0 ||
145+
ERC721TokenReceiver(to).onERC721Received(
146+
msg.sender,
147+
from,
148+
id,
149+
""
150+
) ==
151+
ERC721TokenReceiver.onERC721Received.selector,
152+
"UNSAFE_RECIPIENT"
153+
);
154+
}
155+
156+
function safeTransferFrom(
157+
address from,
158+
address to,
159+
uint256 id,
160+
bytes calldata data
161+
) public virtual {
162+
transferFrom(from, to, id);
163+
164+
require(
165+
to.code.length == 0 ||
166+
ERC721TokenReceiver(to).onERC721Received(
167+
msg.sender,
168+
from,
169+
id,
170+
data
171+
) ==
172+
ERC721TokenReceiver.onERC721Received.selector,
173+
"UNSAFE_RECIPIENT"
174+
);
175+
}
176+
177+
/*//////////////////////////////////////////////////////////////
178+
ERC165 LOGIC
179+
//////////////////////////////////////////////////////////////*/
180+
181+
function supportsInterface(bytes4 interfaceId)
182+
public
183+
view
184+
virtual
185+
returns (bool)
186+
{
187+
return
188+
interfaceId == 0x01ffc9a7 || // ERC165 Interface ID for ERC165
189+
interfaceId == 0x80ac58cd || // ERC165 Interface ID for ERC721
190+
interfaceId == 0x5b5e139f; // ERC165 Interface ID for ERC721Metadata
191+
}
192+
193+
/*//////////////////////////////////////////////////////////////
194+
INTERNAL MINT/BURN LOGIC
195+
//////////////////////////////////////////////////////////////*/
196+
197+
function _mint(address to, uint256 id) internal virtual {
198+
require(_ownerOf[id] == address(0), "ALREADY_MINTED");
199+
200+
// Counter overflow is incredibly unrealistic.
201+
unchecked {
202+
_balanceOf[to]++;
203+
}
204+
205+
_ownerOf[id] = to;
206+
207+
emit Transfer(address(0), to, id);
208+
}
209+
210+
function _burn(uint256 id) internal virtual {
211+
address owner = _ownerOf[id];
212+
213+
require(owner != address(0), "NOT_MINTED");
214+
215+
// Ownership check above ensures no underflow.
216+
unchecked {
217+
_balanceOf[owner]--;
218+
}
219+
220+
delete _ownerOf[id];
221+
222+
delete getApproved[id];
223+
224+
emit Transfer(owner, address(0), id);
225+
}
226+
227+
/*//////////////////////////////////////////////////////////////
228+
INTERNAL SAFE MINT LOGIC
229+
//////////////////////////////////////////////////////////////*/
230+
231+
function _safeMint(address to, uint256 id) internal virtual {
232+
_mint(to, id);
233+
234+
require(
235+
to.code.length == 0 ||
236+
ERC721TokenReceiver(to).onERC721Received(
237+
msg.sender,
238+
address(0),
239+
id,
240+
""
241+
) ==
242+
ERC721TokenReceiver.onERC721Received.selector,
243+
"UNSAFE_RECIPIENT"
244+
);
245+
}
246+
247+
function _safeMint(
248+
address to,
249+
uint256 id,
250+
bytes memory data
251+
) internal virtual {
252+
_mint(to, id);
253+
254+
require(
255+
to.code.length == 0 ||
256+
ERC721TokenReceiver(to).onERC721Received(
257+
msg.sender,
258+
address(0),
259+
id,
260+
data
261+
) ==
262+
ERC721TokenReceiver.onERC721Received.selector,
263+
"UNSAFE_RECIPIENT"
264+
);
265+
}
266+
}
267+
268+
/// @notice A generic interface for a contract which properly accepts ERC721 tokens.
269+
/// @author Solmate (https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC721.sol)
270+
abstract contract ERC721TokenReceiver {
271+
function onERC721Received(
272+
address,
273+
address,
274+
uint256,
275+
bytes calldata
276+
) external virtual returns (bytes4) {
277+
return ERC721TokenReceiver.onERC721Received.selector;
278+
}
279+
}

‎test/foundry/utils/BaseOrderTest.sol

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ contract BaseOrderTest is OfferConsiderationItemAdder, AmountDeriver {
219219
uint256 originalItemsLength,
220220
uint256 amtToSubtractFromItemsLength
221221
) internal {
222-
assertTrue(_validateOrder(order, consideration));
222+
assertTrue(_validateOrder(order, _consideration));
223223

224224
bool overwriteItemsLength = amtToSubtractFromItemsLength > 0;
225225
if (overwriteItemsLength) {
@@ -251,7 +251,7 @@ contract BaseOrderTest is OfferConsiderationItemAdder, AmountDeriver {
251251
);
252252

253253
bool success = _callConsiderationFulfillOrderWithCalldata(
254-
address(consideration),
254+
address(_consideration),
255255
fulfillOrderCalldata
256256
);
257257

‎test/foundry/utils/OfferConsiderationItemAdder.sol

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,35 @@ contract OfferConsiderationItemAdder is TestTokenMinter {
5252
addOfferItem(itemType, identifier, amt, amt);
5353
}
5454

55-
function addErc721OfferItem(uint256 tokenId) internal {
56-
addOfferItem(ItemType.ERC721, address(test721_1), tokenId, 1, 1);
55+
function addErc721OfferItem(uint256 identifier) internal {
56+
addErc721OfferItem(address(test721_1), identifier);
57+
}
58+
59+
function addErc721OfferItem(address token, uint256 identifier) internal {
60+
addErc721OfferItem(token, identifier, 1, 1);
61+
}
62+
63+
function addErc721OfferItem(
64+
address token,
65+
uint256 identifier,
66+
uint256 amount
67+
) internal {
68+
addErc721OfferItem(token, identifier, amount, amount);
69+
}
70+
71+
function addErc721OfferItem(
72+
address token,
73+
uint256 identifier,
74+
uint256 startAmount,
75+
uint256 endAmount
76+
) internal {
77+
addOfferItem(
78+
ItemType.ERC721,
79+
token,
80+
identifier,
81+
startAmount,
82+
endAmount
83+
);
5784
}
5885

5986
function addErc1155OfferItem(uint256 tokenId, uint256 amount) internal {

0 commit comments

Comments
 (0)