SignatureDrop: using DropSinglePhase (#169)

* back to DropSinglePhase

* access control for setClaimConditions

* updated unit tests

* eliminate SigMint; IDrop to IDropSinglePhase

* delete prev commented code

* delete unused DropSinglePhase

* delete getters for size

* remove reentrancy upgradeable from SignatureDrop for sizing

* reentrancy tests outline

* reentrancy tests: claim and mintWithSignature

* use erc721a _burn; remove duplication

* run prettier

* delete unused SigMint

* silence forge build warnings

* merkle tree, allowlist for tests

* use _msgSender()

* code cleanup; refactored tests for mintWithSignature

* run prettier

* scenario based tests

* run prettier

* yaml edit forge tests

* yaml edit forge tests

* yaml edit forge tests

* package update

* getClaimTimestamp conditionId

* benchmark tests for claim and mintWithSignature

* update docs

* dev release

* Remove claimConditionIndex from TokensClaimed in IDropSinglePhase

* code comment about mintWithSignature frontrun attack

* use internal _setup instead of public setters in initialize

* yarn: use erc721a-upgradeable 3.3

* code comment for ERC721A's _burn

* run prettier

* run prettier

Co-authored-by: yash <kumaryashcse@gmail.com>
Co-authored-by: Krishang Nadgauda <nkrishang@Krishangs-MBP.lan>
Co-authored-by: Krishang Nadgauda <nkrishang@Krishangs-MacBook-Pro.local>
Co-authored-by: Joaquim Verges <joaquim.verges@gmail.com>

Author: 5 people

.github/workflows/tests.yml

@@ -32,4 +32,6 @@ jobs:
         with:
           version: nightly
       - name: Run tests
-        run: forge test
+        run: |
+          forge test --no-match-contract SignatureDrop
+          forge test --match-contract SignatureDrop --ffi

contracts/feature/ContractMetadata.sol

@@ -8,8 +8,13 @@ abstract contract ContractMetadata is IContractMetadata {
     string public override contractURI;
 
     /// @dev Lets a contract admin set the URI for contract-level metadata.
-    function setContractURI(string memory _uri) public override {
+    function setContractURI(string memory _uri) external override {
         require(_canSetContractURI(), "Not authorized");
+        _setupContractURI(_uri);
+    }
+
+    /// @dev Lets a contract admin set the URI for contract-level metadata.
+    function _setupContractURI(string memory _uri) internal {
         string memory prevURI = contractURI;
         contractURI = _uri;
 

contracts/feature/DropSinglePhase.sol

@@ -88,17 +88,15 @@ abstract contract DropSinglePhase is IDropSinglePhase {
         // Mint the relevant NFTs to claimer.
         uint256 startTokenId = transferTokensOnClaim(_receiver, _quantity);
 
-        emit TokensClaimed(claimCondition, _dropMsgSender(), _receiver, _quantity, startTokenId);
+        emit TokensClaimed(_dropMsgSender(), _receiver, startTokenId, _quantity);
 
         _afterClaim(_receiver, _quantity, _currency, _pricePerToken, _allowlistProof, _data);
     }
 
     /// @dev Lets a contract admin set claim conditions.
-    function setClaimConditions(
-        ClaimCondition calldata _condition,
-        bool _resetClaimEligibility,
-        bytes memory
-    ) external virtual override {
+    function setClaimConditions(ClaimCondition calldata _condition, bool _resetClaimEligibility) external override {
+        require(_canSetClaimConditions(), "Not authorized");
+
         bytes32 targetConditionId = conditionId;
         uint256 supplyClaimedAlready = claimCondition.supplyClaimed;
 
@@ -110,10 +108,10 @@ abstract contract DropSinglePhase is IDropSinglePhase {
         require(supplyClaimedAlready <= _condition.maxClaimableSupply, "max supply claimed already");
 
         claimCondition = ClaimCondition({
-            startTimestamp: block.timestamp,
+            startTimestamp: _condition.startTimestamp,
             maxClaimableSupply: _condition.maxClaimableSupply,
             supplyClaimed: supplyClaimedAlready,
-            quantityLimitPerTransaction: _condition.supplyClaimed,
+            quantityLimitPerTransaction: _condition.quantityLimitPerTransaction,
             waitTimeInSecondsBetweenClaims: _condition.waitTimeInSecondsBetweenClaims,
             merkleRoot: _condition.merkleRoot,
             pricePerToken: _condition.pricePerToken,
@@ -150,12 +148,8 @@ abstract contract DropSinglePhase is IDropSinglePhase {
             "exceed max claimable supply."
         );
 
-        uint256 timestampOfLastClaim = lastClaimTimestamp[conditionId][_claimer];
-        require(
-            timestampOfLastClaim == 0 ||
-                block.timestamp >= timestampOfLastClaim + currentClaimPhase.waitTimeInSecondsBetweenClaims,
-            "cannot claim."
-        );
+        (uint256 lastClaimedAt, uint256 nextValidClaimTimestamp) = getClaimTimestamp(_claimer);
+        require(lastClaimedAt == 0 || block.timestamp >= nextValidClaimTimestamp, "cannot claim.");
     }
 
     /// @dev Checks whether a claimer meets the claim condition's allowlist criteria.
@@ -181,6 +175,23 @@ abstract contract DropSinglePhase is IDropSinglePhase {
         }
     }
 
+    /// @dev Returns the timestamp for when a claimer is eligible for claiming NFTs again.
+    function getClaimTimestamp(address _claimer)
+        public
+        view
+        returns (uint256 lastClaimedAt, uint256 nextValidClaimTimestamp)
+    {
+        lastClaimedAt = lastClaimTimestamp[conditionId][_claimer];
+
+        unchecked {
+            nextValidClaimTimestamp = lastClaimedAt + claimCondition.waitTimeInSecondsBetweenClaims;
+
+            if (nextValidClaimTimestamp < lastClaimedAt) {
+                nextValidClaimTimestamp = type(uint256).max;
+            }
+        }
+    }
+
     /*////////////////////////////////////////////////////////////////////
         Optional hooks that can be implemented in the derived contract
     ///////////////////////////////////////////////////////////////////*/
@@ -210,10 +221,6 @@ abstract contract DropSinglePhase is IDropSinglePhase {
         bytes memory _data
     ) internal virtual {}
 
-    /*///////////////////////////////////////////////////////////////
-        Virtual functions: to be implemented in derived contract
-    //////////////////////////////////////////////////////////////*/
-
     /// @dev Collects and distributes the primary sale value of NFTs being claimed.
     function collectPriceOnClaim(
         uint256 _quantityToClaim,
@@ -226,4 +233,6 @@ abstract contract DropSinglePhase is IDropSinglePhase {
         internal
         virtual
         returns (uint256 startTokenId);
+
+    function _canSetClaimConditions() internal virtual returns (bool);
 }

contracts/feature/Ownable.sol

@@ -8,9 +8,13 @@ abstract contract Ownable is IOwnable {
     address public override owner;
 
     /// @dev Lets a contract admin set a new owner for the contract. The new owner must be a contract admin.
-    function setOwner(address _newOwner) public override {
+    function setOwner(address _newOwner) external override {
         require(_canSetOwner(), "Not authorized");
+        _setupOwner(_newOwner);
+    }
 
+    /// @dev Lets a contract admin set a new owner for the contract. The new owner must be a contract admin.
+    function _setupOwner(address _newOwner) internal {
         address _prevOwner = owner;
         owner = _newOwner;
 

contracts/feature/PlatformFee.sol

@@ -16,8 +16,13 @@ abstract contract PlatformFee is IPlatformFee {
     }
 
     /// @dev Lets a contract admin update the platform fee recipient and bps
-    function setPlatformFeeInfo(address _platformFeeRecipient, uint256 _platformFeeBps) public override {
+    function setPlatformFeeInfo(address _platformFeeRecipient, uint256 _platformFeeBps) external override {
         require(_canSetPlatformFeeInfo(), "Not authorized");
+        _setupPlatformFeeInfo(_platformFeeRecipient, _platformFeeBps);
+    }
+
+    /// @dev Lets a contract admin update the platform fee recipient and bps
+    function _setupPlatformFeeInfo(address _platformFeeRecipient, uint256 _platformFeeBps) internal {
         require(_platformFeeBps <= 10_000, "Exceeds max bps");
 
         platformFeeBps = uint16(_platformFeeBps);

contracts/feature/PrimarySale.sol

@@ -12,9 +12,13 @@ abstract contract PrimarySale is IPrimarySale {
     }
 
     /// @dev Lets a contract admin set the recipient for all primary sales.
-    function setPrimarySaleRecipient(address _saleRecipient) public override {
+    function setPrimarySaleRecipient(address _saleRecipient) external override {
         require(_canSetPrimarySaleRecipient(), "Not authorized");
+        _setupPrimarySaleRecipient(_saleRecipient);
+    }
 
+    /// @dev Lets a contract admin set the recipient for all primary sales.
+    function _setupPrimarySaleRecipient(address _saleRecipient) internal {
         recipient = _saleRecipient;
         emit PrimarySaleRecipientUpdated(_saleRecipient);
     }

contracts/feature/Royalty.sol

@@ -42,8 +42,13 @@ abstract contract Royalty is IRoyalty {
     }
 
     /// @dev Lets a contract admin update the default royalty recipient and bps.
-    function setDefaultRoyaltyInfo(address _royaltyRecipient, uint256 _royaltyBps) public override {
+    function setDefaultRoyaltyInfo(address _royaltyRecipient, uint256 _royaltyBps) external override {
         require(_canSetRoyaltyInfo(), "Not authorized");
+        _setupDefaultRoyaltyInfo(_royaltyRecipient, _royaltyBps);
+    }
+
+    /// @dev Lets a contract admin update the default royalty recipient and bps.
+    function _setupDefaultRoyaltyInfo(address _royaltyRecipient, uint256 _royaltyBps) internal {
         require(_royaltyBps <= 10_000, "Exceeds max bps");
 
         royaltyRecipient = _royaltyRecipient;
@@ -59,6 +64,15 @@ abstract contract Royalty is IRoyalty {
         uint256 _bps
     ) external override {
         require(_canSetRoyaltyInfo(), "Not authorized");
+        _setupRoyaltyInfoForToken(_tokenId, _recipient, _bps);
+    }
+
+    /// @dev Lets a contract admin set the royalty recipient and bps for a particular token Id.
+    function _setupRoyaltyInfoForToken(
+        uint256 _tokenId,
+        address _recipient,
+        uint256 _bps
+    ) internal {
         require(_bps <= 10_000, "Exceeds max bps");
 
         royaltyInfoForToken[_tokenId] = RoyaltyInfo({ recipient: _recipient, bps: _bps });

contracts/feature/interface/IDropSinglePhase.sol

@@ -10,11 +10,10 @@ interface IDropSinglePhase is IClaimCondition {
     }
 
     event TokensClaimed(
-        ClaimCondition condition,
         address indexed claimer,
         address indexed receiver,
-        uint256 quantityClaimed,
-        uint256 indexed aux
+        uint256 startTokenId,
+        uint256 quantityClaimed
     );
 
     event ClaimConditionUpdated(ClaimCondition condition, bool resetEligibility);
@@ -46,12 +45,6 @@ interface IDropSinglePhase is IClaimCondition {
      *
      *  @param resetClaimEligibility    Whether to reset `limitLastClaimTimestamp` and `limitMerkleProofClaim` values when setting new
      *                                  claim conditions.
-     *
-     *  @param data                     Arbitrary bytes data that can be leveraged in the implementation of this interface.
      */
-    function setClaimConditions(
-        ClaimCondition calldata phase,
-        bool resetClaimEligibility,
-        bytes memory data
-    ) external;
+    function setClaimConditions(ClaimCondition calldata phase, bool resetClaimEligibility) external;
 }