Audit fixes: DropERC20.sol (#215)

* [M-3] Users can claim small amounts of tokens for free

* [Q-2] Missing Events: DropERC20

* [Q-4] Unused import

* [G-4] Reduce SLOADs in verifyClaim()

* [Q-5] Inaccurate getClaimTimestamp() return value

* forge update

* forge update

* run prettier

Co-authored-by: Yash <kumaryashcse@gmail.com>
Co-authored-by: Krishang Nadgauda <nkrishang@Krishangs-MacBook-Pro.local>

Author: 3 people

contracts/drop/DropERC20.sol

@@ -4,7 +4,6 @@ pragma solidity ^0.8.11;
 //  ==========  External imports    ==========
 
 import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";
-import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PausableUpgradeable.sol";
 import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20VotesUpgradeable.sol";
 
 import "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol";
@@ -314,6 +313,8 @@ contract DropERC20 is
 
         // `_pricePerToken` is interpreted as price per 1 ether unit of the ERC20 tokens.
         uint256 totalPrice = (_quantityToClaim * _pricePerToken) / 1 ether;
+        require(totalPrice > 0, "quantity too low");
+
         uint256 platformFees = (totalPrice * platformFeeBps) / MAX_BPS;
 
         if (_currency == CurrencyTransferLib.NATIVE_TOKEN) {
@@ -366,14 +367,17 @@ contract DropERC20 is
             currentClaimPhase.supplyClaimed + _quantity <= currentClaimPhase.maxClaimableSupply,
             "exceed max mint supply."
         );
-        require(maxTotalSupply == 0 || totalSupply() + _quantity <= maxTotalSupply, "exceed max total supply.");
+
+        uint256 _maxTotalSupply = maxTotalSupply;
+        uint256 _maxWalletClaimCount = maxWalletClaimCount;
+        require(_maxTotalSupply == 0 || totalSupply() + _quantity <= _maxTotalSupply, "exceed max total supply.");
         require(
-            maxWalletClaimCount == 0 || walletClaimCount[_claimer] + _quantity <= maxWalletClaimCount,
+            _maxWalletClaimCount == 0 || walletClaimCount[_claimer] + _quantity <= _maxWalletClaimCount,
             "exceed claim limit for wallet"
         );
 
-        (uint256 lastClaimTimestamp, uint256 nextValidClaimTimestamp) = getClaimTimestamp(_conditionId, _claimer);
-        require(lastClaimTimestamp == 0 || block.timestamp >= nextValidClaimTimestamp, "cannot claim yet.");
+        (, uint256 nextValidClaimTimestamp) = getClaimTimestamp(_conditionId, _claimer);
+        require(block.timestamp >= nextValidClaimTimestamp, "cannot claim yet.");
     }
 
     /// @dev Checks whether a claimer meets the claim condition's allowlist criteria.
@@ -424,13 +428,15 @@ contract DropERC20 is
     {
         lastClaimTimestamp = claimCondition.limitLastClaimTimestamp[_conditionId][_claimer];
 
-        unchecked {
-            nextValidClaimTimestamp =
-                lastClaimTimestamp +
-                claimCondition.phases[_conditionId].waitTimeInSecondsBetweenClaims;
+        if (lastClaimTimestamp != 0) {
+            unchecked {
+                nextValidClaimTimestamp =
+                    lastClaimTimestamp +
+                    claimCondition.phases[_conditionId].waitTimeInSecondsBetweenClaims;
 
-            if (nextValidClaimTimestamp < lastClaimTimestamp) {
-                nextValidClaimTimestamp = type(uint256).max;
+                if (nextValidClaimTimestamp < lastClaimTimestamp) {
+                    nextValidClaimTimestamp = type(uint256).max;
+                }
             }
         }
     }
@@ -488,7 +494,10 @@ contract DropERC20 is
 
     /// @dev Lets a contract admin set the URI for contract-level metadata.
     function setContractURI(string calldata _uri) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        string memory prevURI = contractURI;
         contractURI = _uri;
+
+        emit ContractURIUpdated(prevURI, _uri);
     }
 
     /*///////////////////////////////////////////////////////////////

contracts/interfaces/drop/IDropERC20.sol

@@ -34,6 +34,9 @@ interface IDropERC20 is IERC20Upgradeable, IDropClaimCondition {
     /// @dev Emitted when the global max wallet claim count is updated.
     event MaxWalletClaimCountUpdated(uint256 count);
 
+    /// @dev Emitted when the contract URI is updated.
+    event ContractURIUpdated(string prevURI, string newURI);
+
     /**
      *  @notice Lets an account claim a given quantity of tokens.
      *

lib/forge-std

@@ -24,7 +24,7 @@ contract DropERC20Test is BaseTest {
         address receiver = getActor(0);
         bytes32[] memory proofs = new bytes32[](0);
 
-        DropERC20.ClaimCondition[] memory conditions = new DropERC1155.ClaimCondition[](1);
+        DropERC20.ClaimCondition[] memory conditions = new DropERC20.ClaimCondition[](1);
         conditions[0].maxClaimableSupply = 500;
         conditions[0].quantityLimitPerTransaction = 100;
         conditions[0].waitTimeInSecondsBetweenClaims = type(uint256).max;
@@ -46,4 +46,52 @@ contract DropERC20Test is BaseTest {
         vm.expectRevert("invalid quantity claimed.");
         drop.claim(receiver, 200, address(0), 0, proofs, 1);
     }
+
+    function test_state_claim_timestamps() public {
+        vm.warp(100);
+
+        address receiver = getActor(0);
+        bytes32[] memory proofs = new bytes32[](0);
+
+        DropERC20.ClaimCondition[] memory conditions = new DropERC20.ClaimCondition[](1);
+        conditions[0].maxClaimableSupply = 500;
+        conditions[0].quantityLimitPerTransaction = 100;
+        conditions[0].waitTimeInSecondsBetweenClaims = type(uint256).max;
+
+        vm.prank(deployer);
+        drop.setClaimConditions(conditions, false);
+
+        (uint256 lastClaimTimestamp, uint256 nextValidClaimTimestamp) = drop.getClaimTimestamp(0, getActor(5));
+        assertEq(lastClaimTimestamp, 0);
+        assertEq(nextValidClaimTimestamp, 0);
+
+        vm.prank(getActor(5), getActor(5));
+        drop.claim(receiver, 50, address(0), 0, proofs, 1);
+
+        (lastClaimTimestamp, nextValidClaimTimestamp) = drop.getClaimTimestamp(0, getActor(5));
+        assertEq(lastClaimTimestamp, 100);
+        assertEq(nextValidClaimTimestamp, type(uint256).max);
+    }
+
+    function test_revert_claim_timestamps() public {
+        vm.warp(1);
+
+        address receiver = getActor(0);
+        bytes32[] memory proofs = new bytes32[](0);
+
+        DropERC20.ClaimCondition[] memory conditions = new DropERC20.ClaimCondition[](1);
+        conditions[0].maxClaimableSupply = 500;
+        conditions[0].quantityLimitPerTransaction = 100;
+        conditions[0].waitTimeInSecondsBetweenClaims = type(uint256).max;
+
+        vm.prank(deployer);
+        drop.setClaimConditions(conditions, false);
+
+        vm.prank(getActor(5), getActor(5));
+        drop.claim(receiver, 50, address(0), 0, proofs, 1);
+
+        vm.prank(getActor(5), getActor(5));
+        vm.expectRevert("cannot claim yet.");
+        drop.claim(receiver, 50, address(0), 0, proofs, 1);
+    }
 }

src/test/drop/DropERC20.t.sol

@@ -650,10 +650,10 @@
   resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.7.0.tgz#3092d70ea60e3d1835466266b1d68ad47035a2d5"
   integrity sha512-52Qb+A1DdOss8QvJrijYYPSf32GUg2pGaG/yCxtaA3cu4jduouTdg4XZSMLW9op54m1jH7J8hoajhHKOPsoJFw==
 
-"@primitivefi/hardhat-dodoc@^0.1.3":
-  version "0.1.3"
-  resolved "https://registry.npmjs.org/@primitivefi/hardhat-dodoc/-/hardhat-dodoc-0.1.3.tgz"
-  integrity sha512-IM2rwyk9SHxnifHnoCKmB1K1su/d1BvF5C0zspCWH8rVrrNpS1NzLTjisDNJmbM69/cWcEX0vfk449LuTsQVaw==
+"@primitivefi/hardhat-dodoc@^0.2.0":
+  version "0.2.3"
+  resolved "https://registry.yarnpkg.com/@primitivefi/hardhat-dodoc/-/hardhat-dodoc-0.2.3.tgz#76aebbfa70de2d6454af29e166b1430583b54c5c"
+  integrity sha512-ver9uHa79LTDTeebOKZ/eOVRL/FP1k0s0x/5Bo/8ZaDdLWFVClKqZyZYVjjW4CJqTPCt8uU9b9p71P2vzH4O9A==
   dependencies:
     squirrelly "^8.0.8"