Skip to content

Commit 8ea438c

Browse files
jakeloojakeloo
authored
Merge pull request #143 from thirdweb-dev/shipyard-droperc721-audit-fixes
droperc721 fixes
2 parents 8d3d775 + 5eb1cb1 commit 8ea438c

File tree

8 files changed

+128
-46
lines changed

8 files changed

+128
-46
lines changed

‎contracts/drop/DropERC1155.sol‎

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -73,14 +73,14 @@ contract DropERC1155 is
7373
/// @dev The address that receives all platform fees from all sales.
7474
address private platformFeeRecipient;
7575

76+
/// @dev The % of primary sales collected as platform fees.
77+
uint16 private platformFeeBps;
78+
7679
/// @dev The recipient of who gets the royalty.
7780
address private royaltyRecipient;
7881

7982
/// @dev The (default) address that receives all royalty value.
80-
uint128 private royaltyBps;
81-
82-
/// @dev The % of primary sales collected as platform fees.
83-
uint128 private platformFeeBps;
83+
uint16 private royaltyBps;
8484

8585
/// @dev Contract level metadata.
8686
string public contractURI;
@@ -149,11 +149,11 @@ contract DropERC1155 is
149149
name = _name;
150150
symbol = _symbol;
151151
royaltyRecipient = _royaltyRecipient;
152-
royaltyBps = _royaltyBps;
152+
royaltyBps = uint16(_royaltyBps);
153153
platformFeeRecipient = _platformFeeRecipient;
154154
primarySaleRecipient = _saleRecipient;
155155
contractURI = _contractURI;
156-
platformFeeBps = _platformFeeBps;
156+
platformFeeBps = uint16(_platformFeeBps);
157157
_owner = _defaultAdmin;
158158

159159
_setupRole(DEFAULT_ADMIN_ROLE, _defaultAdmin);
@@ -257,10 +257,14 @@ contract DropERC1155 is
257257
// Get the active claim condition index.
258258
uint256 activeConditionId = getActiveClaimConditionId(_tokenId);
259259

260-
// Verify claim validity. If not valid, revert.
261-
verifyClaim(activeConditionId, _msgSender(), _tokenId, _quantity, _currency, _pricePerToken);
260+
/**
261+
* We make allowlist checks (i.e. verifyClaimMerkleProof) before verifying the claim's general
262+
* validity (i.e. verifyClaim) because we give precedence to the check of allow list quantity
263+
* restriction over the check of the general claim condition's quantityLimitPerTransaction
264+
* restriction.
265+
*/
262266

263-
// Verify inclusion in allowlist
267+
// Verify inclusion in allowlist.
264268
(bool validMerkleProof, uint256 merkleProofIndex) = verifyClaimMerkleProof(
265269
activeConditionId,
266270
_msgSender(),
@@ -270,8 +274,23 @@ contract DropERC1155 is
270274
_proofMaxQuantityPerTransaction
271275
);
272276

277+
// Verify claim validity. If not valid, revert.
278+
bool toVerifyMaxQuantityPerTransaction = _proofMaxQuantityPerTransaction == 0;
279+
verifyClaim(
280+
activeConditionId,
281+
_msgSender(),
282+
_tokenId,
283+
_quantity,
284+
_currency,
285+
_pricePerToken,
286+
toVerifyMaxQuantityPerTransaction
287+
);
288+
273289
if (validMerkleProof && _proofMaxQuantityPerTransaction > 0) {
274-
// Mark the claimer's use of their position in the allowlist.
290+
/**
291+
* Mark the claimer's use of their position in the allowlist. A spot in an allowlist
292+
* can be used only once.
293+
*/
275294
claimCondition[_tokenId].limitMerkleProofClaim[activeConditionId].set(merkleProofIndex);
276295
}
277296

@@ -317,8 +336,11 @@ contract DropERC1155 is
317336
"startTimestamp must be in ascending order."
318337
);
319338

339+
uint256 supplyClaimedAlready = condition.phases[newStartIndex + i].supplyClaimed;
340+
require(supplyClaimedAlready < _phases[i].maxClaimableSupply, "max supply claimed already");
341+
320342
condition.phases[newStartIndex + i] = _phases[i];
321-
condition.phases[newStartIndex + i].supplyClaimed = 0;
343+
condition.phases[newStartIndex + i].supplyClaimed = supplyClaimedAlready;
322344

323345
lastConditionStartTimestamp = _phases[i].startTimestamp;
324346
}
@@ -402,16 +424,19 @@ contract DropERC1155 is
402424
uint256 _tokenId,
403425
uint256 _quantity,
404426
address _currency,
405-
uint256 _pricePerToken
427+
uint256 _pricePerToken,
428+
bool verifyMaxQuantityPerTransaction
406429
) public view {
407430
ClaimCondition memory currentClaimPhase = claimCondition[_tokenId].phases[_conditionId];
408431

409432
require(
410433
_currency == currentClaimPhase.currency && _pricePerToken == currentClaimPhase.pricePerToken,
411434
"invalid currency or price specified."
412435
);
436+
// If we're checking for an allowlist quantity restriction, ignore the general quantity restriction.
413437
require(
414-
_quantity > 0 && _quantity <= currentClaimPhase.quantityLimitPerTransaction,
438+
_quantity > 0 &&
439+
(!verifyMaxQuantityPerTransaction || _quantity <= currentClaimPhase.quantityLimitPerTransaction),
415440
"invalid quantity claimed."
416441
);
417442
require(
@@ -569,7 +594,7 @@ contract DropERC1155 is
569594
require(_royaltyBps <= MAX_BPS, "exceed royalty bps");
570595

571596
royaltyRecipient = _royaltyRecipient;
572-
royaltyBps = uint128(_royaltyBps);
597+
royaltyBps = uint16(_royaltyBps);
573598

574599
emit DefaultRoyalty(_royaltyRecipient, _royaltyBps);
575600
}
@@ -594,7 +619,7 @@ contract DropERC1155 is
594619
{
595620
require(_platformFeeBps <= MAX_BPS, "bps <= 10000.");
596621

597-
platformFeeBps = uint64(_platformFeeBps);
622+
platformFeeBps = uint16(_platformFeeBps);
598623
platformFeeRecipient = _platformFeeRecipient;
599624

600625
emit PlatformFeeInfoUpdated(_platformFeeRecipient, _platformFeeBps);

‎contracts/drop/DropERC20.sol‎

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -181,10 +181,14 @@ contract DropERC20 is
181181
// Get the claim conditions.
182182
uint256 activeConditionId = getActiveClaimConditionId();
183183

184-
// Verify claim validity. If not valid, revert.
185-
verifyClaim(activeConditionId, _msgSender(), _quantity, _currency, _pricePerToken);
184+
/**
185+
* We make allowlist checks (i.e. verifyClaimMerkleProof) before verifying the claim's general
186+
* validity (i.e. verifyClaim) because we give precedence to the check of allow list quantity
187+
* restriction over the check of the general claim condition's quantityLimitPerTransaction
188+
* restriction.
189+
*/
186190

187-
// Verify inclusion in allowlist
191+
// Verify inclusion in allowlist.
188192
(bool validMerkleProof, uint256 merkleProofIndex) = verifyClaimMerkleProof(
189193
activeConditionId,
190194
_msgSender(),
@@ -193,8 +197,22 @@ contract DropERC20 is
193197
_proofMaxQuantityPerTransaction
194198
);
195199

200+
// Verify claim validity. If not valid, revert.
201+
bool toVerifyMaxQuantityPerTransaction = _proofMaxQuantityPerTransaction == 0;
202+
verifyClaim(
203+
activeConditionId,
204+
_msgSender(),
205+
_quantity,
206+
_currency,
207+
_pricePerToken,
208+
toVerifyMaxQuantityPerTransaction
209+
);
210+
196211
if (validMerkleProof && _proofMaxQuantityPerTransaction > 0) {
197-
// Mark the claimer's use of their position in the allowlist.
212+
/**
213+
* Mark the claimer's use of their position in the allowlist. A spot in an allowlist
214+
* can be used only once.
215+
*/
198216
claimCondition.limitMerkleProofClaim[activeConditionId].set(merkleProofIndex);
199217
}
200218

@@ -238,8 +256,11 @@ contract DropERC20 is
238256
"startTimestamp must be in ascending order."
239257
);
240258

259+
uint256 supplyClaimedAlready = claimCondition.phases[newStartIndex + i].supplyClaimed;
260+
require(supplyClaimedAlready < _phases[i].maxClaimableSupply, "max supply claimed already");
261+
241262
claimCondition.phases[newStartIndex + i] = _phases[i];
242-
claimCondition.phases[newStartIndex + i].supplyClaimed = 0;
263+
claimCondition.phases[newStartIndex + i].supplyClaimed = supplyClaimedAlready;
243264

244265
lastConditionStartTimestamp = _phases[i].startTimestamp;
245266
}
@@ -324,16 +345,19 @@ contract DropERC20 is
324345
address _claimer,
325346
uint256 _quantity,
326347
address _currency,
327-
uint256 _pricePerToken
348+
uint256 _pricePerToken,
349+
bool verifyMaxQuantityPerTransaction
328350
) public view {
329351
ClaimCondition memory currentClaimPhase = claimCondition.phases[_conditionId];
330352

331353
require(
332354
_currency == currentClaimPhase.currency && _pricePerToken == currentClaimPhase.pricePerToken,
333355
"invalid currency or price specified."
334356
);
357+
// If we're checking for an allowlist quantity restriction, ignore the general quantity restriction.
335358
require(
336-
_quantity > 0 && _quantity <= currentClaimPhase.quantityLimitPerTransaction,
359+
_quantity > 0 &&
360+
(!verifyMaxQuantityPerTransaction || _quantity <= currentClaimPhase.quantityLimitPerTransaction),
337361
"invalid quantity claimed."
338362
);
339363
require(

‎contracts/drop/DropERC721.sol‎

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,14 @@ contract DropERC721 is
7575
/// @dev The address that receives all platform fees from all sales.
7676
address private platformFeeRecipient;
7777

78+
/// @dev The % of primary sales collected as platform fees.
79+
uint16 private platformFeeBps;
80+
7881
/// @dev The (default) address that receives all royalty value.
7982
address private royaltyRecipient;
8083

8184
/// @dev The (default) % of a sale to take as royalty (in basis points).
82-
uint128 private royaltyBps;
83-
84-
/// @dev The % of primary sales collected as platform fees.
85-
uint128 private platformFeeBps;
85+
uint16 private royaltyBps;
8686

8787
/// @dev Contract level metadata.
8888
string public contractURI;
@@ -143,9 +143,9 @@ contract DropERC721 is
143143

144144
// Initialize this contract's state.
145145
royaltyRecipient = _royaltyRecipient;
146-
royaltyBps = _royaltyBps;
146+
royaltyBps = uint16(_royaltyBps);
147147
platformFeeRecipient = _platformFeeRecipient;
148-
platformFeeBps = _platformFeeBps;
148+
platformFeeBps = uint16(_platformFeeBps);
149149
primarySaleRecipient = _saleRecipient;
150150
contractURI = _contractURI;
151151
_owner = _defaultAdmin;
@@ -322,10 +322,14 @@ contract DropERC721 is
322322
// Get the claim conditions.
323323
uint256 activeConditionId = getActiveClaimConditionId();
324324

325-
// Verify claim validity. If not valid, revert.
326-
verifyClaim(activeConditionId, _msgSender(), _quantity, _currency, _pricePerToken);
325+
/**
326+
* We make allowlist checks (i.e. verifyClaimMerkleProof) before verifying the claim's general
327+
* validity (i.e. verifyClaim) because we give precedence to the check of allow list quantity
328+
* restriction over the check of the general claim condition's quantityLimitPerTransaction
329+
* restriction.
330+
*/
327331

328-
// Verify inclusion in allowlist
332+
// Verify inclusion in allowlist.
329333
(bool validMerkleProof, uint256 merkleProofIndex) = verifyClaimMerkleProof(
330334
activeConditionId,
331335
_msgSender(),
@@ -334,8 +338,22 @@ contract DropERC721 is
334338
_proofMaxQuantityPerTransaction
335339
);
336340

341+
// Verify claim validity. If not valid, revert.
342+
bool toVerifyMaxQuantityPerTransaction = _proofMaxQuantityPerTransaction == 0;
343+
verifyClaim(
344+
activeConditionId,
345+
_msgSender(),
346+
_quantity,
347+
_currency,
348+
_pricePerToken,
349+
toVerifyMaxQuantityPerTransaction
350+
);
351+
337352
if (validMerkleProof && _proofMaxQuantityPerTransaction > 0) {
338-
// Mark the claimer's use of their position in the allowlist.
353+
/**
354+
* Mark the claimer's use of their position in the allowlist. A spot in an allowlist
355+
* can be used only once.
356+
*/
339357
claimCondition.limitMerkleProofClaim[activeConditionId].set(merkleProofIndex);
340358
}
341359

@@ -379,8 +397,11 @@ contract DropERC721 is
379397
"startTimestamp must be in ascending order."
380398
);
381399

400+
uint256 supplyClaimedAlready = claimCondition.phases[newStartIndex + i].supplyClaimed;
401+
require(supplyClaimedAlready < _phases[i].maxClaimableSupply, "max supply claimed already");
402+
382403
claimCondition.phases[newStartIndex + i] = _phases[i];
383-
claimCondition.phases[newStartIndex + i].supplyClaimed = 0;
404+
claimCondition.phases[newStartIndex + i].supplyClaimed = supplyClaimedAlready;
384405

385406
lastConditionStartTimestamp = _phases[i].startTimestamp;
386407
}
@@ -471,16 +492,20 @@ contract DropERC721 is
471492
address _claimer,
472493
uint256 _quantity,
473494
address _currency,
474-
uint256 _pricePerToken
495+
uint256 _pricePerToken,
496+
bool verifyMaxQuantityPerTransaction
475497
) public view {
476498
ClaimCondition memory currentClaimPhase = claimCondition.phases[_conditionId];
477499

478500
require(
479501
_currency == currentClaimPhase.currency && _pricePerToken == currentClaimPhase.pricePerToken,
480502
"invalid currency or price specified."
481503
);
504+
505+
// If we're checking for an allowlist quantity restriction, ignore the general quantity restriction.
482506
require(
483-
_quantity > 0 && _quantity <= currentClaimPhase.quantityLimitPerTransaction,
507+
_quantity > 0 &&
508+
(!verifyMaxQuantityPerTransaction || _quantity <= currentClaimPhase.quantityLimitPerTransaction),
484509
"invalid quantity claimed."
485510
);
486511
require(
@@ -624,7 +649,7 @@ contract DropERC721 is
624649
require(_royaltyBps <= MAX_BPS, "exceed royalty bps");
625650

626651
royaltyRecipient = _royaltyRecipient;
627-
royaltyBps = uint128(_royaltyBps);
652+
royaltyBps = uint16(_royaltyBps);
628653

629654
emit DefaultRoyalty(_royaltyRecipient, _royaltyBps);
630655
}
@@ -649,7 +674,7 @@ contract DropERC721 is
649674
{
650675
require(_platformFeeBps <= MAX_BPS, "bps <= 10000.");
651676

652-
platformFeeBps = uint64(_platformFeeBps);
677+
platformFeeBps = uint16(_platformFeeBps);
653678
platformFeeRecipient = _platformFeeRecipient;
654679

655680
emit PlatformFeeInfoUpdated(_platformFeeRecipient, _platformFeeBps);

‎contracts/lib/CurrencyTransferLib.sol‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@ pragma solidity ^0.8.11;
44
// Helper interfaces
55
import { IWETH } from "../interfaces/IWETH.sol";
66
import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
7+
import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
78

89
library CurrencyTransferLib {
10+
using SafeERC20Upgradeable for IERC20Upgradeable;
11+
912
/// @dev The address interpreted as native token of the chain.
1013
address public constant NATIVE_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
1114

@@ -67,11 +70,11 @@ library CurrencyTransferLib {
6770
return;
6871
}
6972

70-
bool success = _from == address(this)
71-
? IERC20Upgradeable(_currency).transfer(_to, _amount)
72-
: IERC20Upgradeable(_currency).transferFrom(_from, _to, _amount);
73-
74-
require(success, "currency transfer failed.");
73+
if (_from == address(this)) {
74+
IERC20Upgradeable(_currency).safeTransfer(_to, _amount);
75+
} else {
76+
IERC20Upgradeable(_currency).safeTransferFrom(_from, _to, _amount);
77+
}
7578
}
7679

7780
/// @dev Transfers `amount` of native token to `to`.
@@ -93,7 +96,7 @@ library CurrencyTransferLib {
9396
(bool success, ) = to.call{ value: value }("");
9497
if (!success) {
9598
IWETH(_nativeTokenWrapper).deposit{ value: value }();
96-
require(IERC20Upgradeable(_nativeTokenWrapper).transfer(to, value), "transfer failed");
99+
IERC20Upgradeable(_nativeTokenWrapper).safeTransfer(to, value);
97100
}
98101
}
99102
}

‎docs/DropERC1155.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1072,7 +1072,7 @@ function uri(uint256 _tokenId) external view returns (string _tokenURI)
10721072
### verifyClaim
10731073

10741074
```solidity
1075-
function verifyClaim(uint256 _conditionId, address _claimer, uint256 _tokenId, uint256 _quantity, address _currency, uint256 _pricePerToken) external view
1075+
function verifyClaim(uint256 _conditionId, address _claimer, uint256 _tokenId, uint256 _quantity, address _currency, uint256 _pricePerToken, bool verifyMaxQuantityPerTransaction) external view
10761076
```
10771077

10781078

@@ -1089,6 +1089,7 @@ function verifyClaim(uint256 _conditionId, address _claimer, uint256 _tokenId, u
10891089
| _quantity | uint256 | undefined
10901090
| _currency | address | undefined
10911091
| _pricePerToken | uint256 | undefined
1092+
| verifyMaxQuantityPerTransaction | bool | undefined
10921093

10931094
### verifyClaimMerkleProof
10941095

0 commit comments

Comments
 (0)