Skip to content

droperc721 fixes #143

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Apr 1, 2022
Merged

droperc721 fixes #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fd82e10
(M-1) Unsafe ERC-20 transfer
Mar 24, 2022
c6986b2
(G-1) Variable packing improvement
Mar 24, 2022
ef37414
UX suggestion: allowlist qty restriction takes precedence over genera…
Mar 24, 2022
3fac7af
update verifyClaim interface
Mar 29, 2022
221a4a3
(M-2) Claim condition race condition
Mar 29, 2022
810fa81
DropERC1155: (G-1) Variable packing improvement
Mar 31, 2022
956f482
DropERC1155: UX suggestion: allowlist qty restriction takes precedenc…
Mar 31, 2022
bae4be7
DropERC1155: (M-2) Claim condition race condition
Mar 31, 2022
025145b
DropERC20: UX suggestion: allowlist qty restriction takes precedence …
Mar 31, 2022
fd82346
DropERC20: (M-2) Claim condition race condition
Mar 31, 2022
640a92c
run prettier + docs gen
Mar 31, 2022
5462745
fix/update DropERC721 tests
Mar 31, 2022
5eb1cb1
Merge branch 'main' into shipyard-droperc721-audit-fixes
jakeloo Apr 1, 2022
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
55 changes: 40 additions & 15 deletions contracts/drop/DropERC1155.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,14 @@ contract DropERC1155 is
/// @dev The address that receives all platform fees from all sales.
address private platformFeeRecipient;

/// @dev The % of primary sales collected as platform fees.
uint16 private platformFeeBps;

/// @dev The recipient of who gets the royalty.
address private royaltyRecipient;

/// @dev The (default) address that receives all royalty value.
uint128 private royaltyBps;

/// @dev The % of primary sales collected as platform fees.
uint128 private platformFeeBps;
uint16 private royaltyBps;

/// @dev Contract level metadata.
string public contractURI;
Expand Down Expand Up @@ -149,11 +149,11 @@ contract DropERC1155 is
name = _name;
symbol = _symbol;
royaltyRecipient = _royaltyRecipient;
royaltyBps = _royaltyBps;
royaltyBps = uint16(_royaltyBps);
platformFeeRecipient = _platformFeeRecipient;
primarySaleRecipient = _saleRecipient;
contractURI = _contractURI;
platformFeeBps = _platformFeeBps;
platformFeeBps = uint16(_platformFeeBps);
_owner = _defaultAdmin;

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

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

// Verify inclusion in allowlist
// Verify inclusion in allowlist.
(bool validMerkleProof, uint256 merkleProofIndex) = verifyClaimMerkleProof(
activeConditionId,
_msgSender(),
Expand All @@ -270,8 +274,23 @@ contract DropERC1155 is
_proofMaxQuantityPerTransaction
);

// Verify claim validity. If not valid, revert.
bool toVerifyMaxQuantityPerTransaction = _proofMaxQuantityPerTransaction == 0;
verifyClaim(
activeConditionId,
_msgSender(),
_tokenId,
_quantity,
_currency,
_pricePerToken,
toVerifyMaxQuantityPerTransaction
);

if (validMerkleProof && _proofMaxQuantityPerTransaction > 0) {
// Mark the claimer's use of their position in the allowlist.
/**
* Mark the claimer's use of their position in the allowlist. A spot in an allowlist
* can be used only once.
*/
claimCondition[_tokenId].limitMerkleProofClaim[activeConditionId].set(merkleProofIndex);
}

Expand Down Expand Up @@ -317,8 +336,11 @@ contract DropERC1155 is
"startTimestamp must be in ascending order."
);

uint256 supplyClaimedAlready = condition.phases[newStartIndex + i].supplyClaimed;
require(supplyClaimedAlready < _phases[i].maxClaimableSupply, "max supply claimed already");

condition.phases[newStartIndex + i] = _phases[i];
condition.phases[newStartIndex + i].supplyClaimed = 0;
condition.phases[newStartIndex + i].supplyClaimed = supplyClaimedAlready;

lastConditionStartTimestamp = _phases[i].startTimestamp;
}
Expand Down Expand Up @@ -402,16 +424,19 @@ contract DropERC1155 is
uint256 _tokenId,
uint256 _quantity,
address _currency,
uint256 _pricePerToken
uint256 _pricePerToken,
bool verifyMaxQuantityPerTransaction
) public view {
ClaimCondition memory currentClaimPhase = claimCondition[_tokenId].phases[_conditionId];

require(
_currency == currentClaimPhase.currency && _pricePerToken == currentClaimPhase.pricePerToken,
"invalid currency or price specified."
);
// If we're checking for an allowlist quantity restriction, ignore the general quantity restriction.
require(
_quantity > 0 && _quantity <= currentClaimPhase.quantityLimitPerTransaction,
_quantity > 0 &&
(!verifyMaxQuantityPerTransaction || _quantity <= currentClaimPhase.quantityLimitPerTransaction),
"invalid quantity claimed."
);
require(
Expand Down Expand Up @@ -569,7 +594,7 @@ contract DropERC1155 is
require(_royaltyBps <= MAX_BPS, "exceed royalty bps");

royaltyRecipient = _royaltyRecipient;
royaltyBps = uint128(_royaltyBps);
royaltyBps = uint16(_royaltyBps);

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

platformFeeBps = uint64(_platformFeeBps);
platformFeeBps = uint16(_platformFeeBps);
platformFeeRecipient = _platformFeeRecipient;

emit PlatformFeeInfoUpdated(_platformFeeRecipient, _platformFeeBps);
Expand Down
38 changes: 31 additions & 7 deletions contracts/drop/DropERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,14 @@ contract DropERC20 is
// Get the claim conditions.
uint256 activeConditionId = getActiveClaimConditionId();

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

// Verify inclusion in allowlist
// Verify inclusion in allowlist.
(bool validMerkleProof, uint256 merkleProofIndex) = verifyClaimMerkleProof(
activeConditionId,
_msgSender(),
Expand All @@ -193,8 +197,22 @@ contract DropERC20 is
_proofMaxQuantityPerTransaction
);

// Verify claim validity. If not valid, revert.
bool toVerifyMaxQuantityPerTransaction = _proofMaxQuantityPerTransaction == 0;
verifyClaim(
activeConditionId,
_msgSender(),
_quantity,
_currency,
_pricePerToken,
toVerifyMaxQuantityPerTransaction
);

if (validMerkleProof && _proofMaxQuantityPerTransaction > 0) {
// Mark the claimer's use of their position in the allowlist.
/**
* Mark the claimer's use of their position in the allowlist. A spot in an allowlist
* can be used only once.
*/
claimCondition.limitMerkleProofClaim[activeConditionId].set(merkleProofIndex);
}

Expand Down Expand Up @@ -238,8 +256,11 @@ contract DropERC20 is
"startTimestamp must be in ascending order."
);

uint256 supplyClaimedAlready = claimCondition.phases[newStartIndex + i].supplyClaimed;
require(supplyClaimedAlready < _phases[i].maxClaimableSupply, "max supply claimed already");

claimCondition.phases[newStartIndex + i] = _phases[i];
claimCondition.phases[newStartIndex + i].supplyClaimed = 0;
claimCondition.phases[newStartIndex + i].supplyClaimed = supplyClaimedAlready;

lastConditionStartTimestamp = _phases[i].startTimestamp;
}
Expand Down Expand Up @@ -324,16 +345,19 @@ contract DropERC20 is
address _claimer,
uint256 _quantity,
address _currency,
uint256 _pricePerToken
uint256 _pricePerToken,
bool verifyMaxQuantityPerTransaction
) public view {
ClaimCondition memory currentClaimPhase = claimCondition.phases[_conditionId];

require(
_currency == currentClaimPhase.currency && _pricePerToken == currentClaimPhase.pricePerToken,
"invalid currency or price specified."
);
// If we're checking for an allowlist quantity restriction, ignore the general quantity restriction.
require(
_quantity > 0 && _quantity <= currentClaimPhase.quantityLimitPerTransaction,
_quantity > 0 &&
(!verifyMaxQuantityPerTransaction || _quantity <= currentClaimPhase.quantityLimitPerTransaction),
"invalid quantity claimed."
);
require(
Expand Down
55 changes: 40 additions & 15 deletions contracts/drop/DropERC721.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ contract DropERC721 is
/// @dev The address that receives all platform fees from all sales.
address private platformFeeRecipient;

/// @dev The % of primary sales collected as platform fees.
uint16 private platformFeeBps;

/// @dev The (default) address that receives all royalty value.
address private royaltyRecipient;

/// @dev The (default) % of a sale to take as royalty (in basis points).
uint128 private royaltyBps;

/// @dev The % of primary sales collected as platform fees.
uint128 private platformFeeBps;
uint16 private royaltyBps;

/// @dev Contract level metadata.
string public contractURI;
Expand Down Expand Up @@ -143,9 +143,9 @@ contract DropERC721 is

// Initialize this contract's state.
royaltyRecipient = _royaltyRecipient;
royaltyBps = _royaltyBps;
royaltyBps = uint16(_royaltyBps);
platformFeeRecipient = _platformFeeRecipient;
platformFeeBps = _platformFeeBps;
platformFeeBps = uint16(_platformFeeBps);
primarySaleRecipient = _saleRecipient;
contractURI = _contractURI;
_owner = _defaultAdmin;
Expand Down Expand Up @@ -322,10 +322,14 @@ contract DropERC721 is
// Get the claim conditions.
uint256 activeConditionId = getActiveClaimConditionId();

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

// Verify inclusion in allowlist
// Verify inclusion in allowlist.
(bool validMerkleProof, uint256 merkleProofIndex) = verifyClaimMerkleProof(
activeConditionId,
_msgSender(),
Expand All @@ -334,8 +338,22 @@ contract DropERC721 is
_proofMaxQuantityPerTransaction
);

// Verify claim validity. If not valid, revert.
bool toVerifyMaxQuantityPerTransaction = _proofMaxQuantityPerTransaction == 0;
verifyClaim(
activeConditionId,
_msgSender(),
_quantity,
_currency,
_pricePerToken,
toVerifyMaxQuantityPerTransaction
);

if (validMerkleProof && _proofMaxQuantityPerTransaction > 0) {
// Mark the claimer's use of their position in the allowlist.
/**
* Mark the claimer's use of their position in the allowlist. A spot in an allowlist
* can be used only once.
*/
claimCondition.limitMerkleProofClaim[activeConditionId].set(merkleProofIndex);
}

Expand Down Expand Up @@ -379,8 +397,11 @@ contract DropERC721 is
"startTimestamp must be in ascending order."
);

uint256 supplyClaimedAlready = claimCondition.phases[newStartIndex + i].supplyClaimed;
require(supplyClaimedAlready < _phases[i].maxClaimableSupply, "max supply claimed already");

claimCondition.phases[newStartIndex + i] = _phases[i];
claimCondition.phases[newStartIndex + i].supplyClaimed = 0;
claimCondition.phases[newStartIndex + i].supplyClaimed = supplyClaimedAlready;

lastConditionStartTimestamp = _phases[i].startTimestamp;
}
Expand Down Expand Up @@ -471,16 +492,20 @@ contract DropERC721 is
address _claimer,
uint256 _quantity,
address _currency,
uint256 _pricePerToken
uint256 _pricePerToken,
bool verifyMaxQuantityPerTransaction
) public view {
ClaimCondition memory currentClaimPhase = claimCondition.phases[_conditionId];

require(
_currency == currentClaimPhase.currency && _pricePerToken == currentClaimPhase.pricePerToken,
"invalid currency or price specified."
);

// If we're checking for an allowlist quantity restriction, ignore the general quantity restriction.
require(
_quantity > 0 && _quantity <= currentClaimPhase.quantityLimitPerTransaction,
_quantity > 0 &&
(!verifyMaxQuantityPerTransaction || _quantity <= currentClaimPhase.quantityLimitPerTransaction),
"invalid quantity claimed."
);
require(
Expand Down Expand Up @@ -624,7 +649,7 @@ contract DropERC721 is
require(_royaltyBps <= MAX_BPS, "exceed royalty bps");

royaltyRecipient = _royaltyRecipient;
royaltyBps = uint128(_royaltyBps);
royaltyBps = uint16(_royaltyBps);

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

platformFeeBps = uint64(_platformFeeBps);
platformFeeBps = uint16(_platformFeeBps);
platformFeeRecipient = _platformFeeRecipient;

emit PlatformFeeInfoUpdated(_platformFeeRecipient, _platformFeeBps);
Expand Down
15 changes: 9 additions & 6 deletions contracts/lib/CurrencyTransferLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ pragma solidity ^0.8.11;
// Helper interfaces
import { IWETH } from "../interfaces/IWETH.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";

library CurrencyTransferLib {
using SafeERC20Upgradeable for IERC20Upgradeable;

/// @dev The address interpreted as native token of the chain.
address public constant NATIVE_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

Expand Down Expand Up @@ -67,11 +70,11 @@ library CurrencyTransferLib {
return;
}

bool success = _from == address(this)
? IERC20Upgradeable(_currency).transfer(_to, _amount)
: IERC20Upgradeable(_currency).transferFrom(_from, _to, _amount);

require(success, "currency transfer failed.");
if (_from == address(this)) {
IERC20Upgradeable(_currency).safeTransfer(_to, _amount);
} else {
IERC20Upgradeable(_currency).safeTransferFrom(_from, _to, _amount);
}
}

/// @dev Transfers `amount` of native token to `to`.
Expand All @@ -93,7 +96,7 @@ library CurrencyTransferLib {
(bool success, ) = to.call{ value: value }("");
if (!success) {
IWETH(_nativeTokenWrapper).deposit{ value: value }();
require(IERC20Upgradeable(_nativeTokenWrapper).transfer(to, value), "transfer failed");
IERC20Upgradeable(_nativeTokenWrapper).safeTransfer(to, value);
}
}
}
3 changes: 2 additions & 1 deletion docs/DropERC1155.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ function uri(uint256 _tokenId) external view returns (string _tokenURI)
### verifyClaim

```solidity
function verifyClaim(uint256 _conditionId, address _claimer, uint256 _tokenId, uint256 _quantity, address _currency, uint256 _pricePerToken) external view
function verifyClaim(uint256 _conditionId, address _claimer, uint256 _tokenId, uint256 _quantity, address _currency, uint256 _pricePerToken, bool verifyMaxQuantityPerTransaction) external view
```


Expand All @@ -1089,6 +1089,7 @@ function verifyClaim(uint256 _conditionId, address _claimer, uint256 _tokenId, u
| _quantity | uint256 | undefined
| _currency | address | undefined
| _pricePerToken | uint256 | undefined
| verifyMaxQuantityPerTransaction | bool | undefined

### verifyClaimMerkleProof

Expand Down
Loading