Macro audit june 2022 (#183)

* [H-2] Batch reveal is permanently corrupted by invocation of public getRevealURI method

* [L-3] LazyMint of a new batch can affect previous batch

* [L-4] Incorrect handling of invalid role approvals/removals in Permissions.sol

* [L-5] Incorrect processing of role approval in PermissionsEnumerable.sol

* [L-6] claimCondition.startTimestamp is not enforced

* docs update

* reduce size by merging checks

* update tests

* [Q-1] Emitted TokensLazyMinted event does not match spec

* [L-3] test fix

* [Q-3] Some events could use indexed attributes

* [Q-5] Visibility for following methods can be changed from public to external

* [Q-4] Missing natspec comments

* [Q-3] test fixes

* forge updates

* fix claim quantity limit

* add test

* run prettier

* replace requires with custom errors

replace requires with custom errors

* standardize errors

* update tests

* run prettier

* [L-6] claimCondition.startTimestamp is not enforced -- revised fix

* [L-6] claimCondition.startTimestamp is not enforced

* reorganize errors and checks

* 0xMacro audit Jun '22: Multiwrap (#181)

* [H-1] Wrapped ETH cannot be unwrapped

* [L-1] Anyone can call renounceRole(), and delete the first valid entry from roleMembers for any role

* [L-2] Incorrect supportsInterface implementation

* [G-4] Require revert msg string length > 32 for Multiwrap: L176 (edited)

* [G-3] Optimization done in CurrencyTransferLib for rare edge cases, increases gas costs for all other use cases.

* [G-2] Improvements for _setBundle()

* [L-7] Unsafe usage of msg.value within CurrencyTransfer library

* [G-5] Return early in PermissionsEnumerable#getRoleMember

Co-authored-by: Krishang Nadgauda <[email protected]>

* 0xMacro audit Jun '22: DropERC1155 (#182)

* (Q-1) Unused variable

* fix claim quantity limit

fix claim quantity limit

* cleanup

* remove thirdwebfee

remove thirdwebfee

Co-authored-by: Krishang Nadgauda <[email protected]>
Co-authored-by: yash <[email protected]>
Co-authored-by: Krishang Nadgauda <[email protected]>

* custom errors cleanup mega commit

* delete commented require statements

* fix timestamp check

* run prettier

* REVERT G-3: breaks Marketplace

* Change revert test -> balances test post behaviour fix

* Fix faulty >= to >

* package bump

* package release

* add bot check to SigDrop

* fix forge build errors

* Update permissions usage in pack tests

* run prettier

* pack docs update

Co-authored-by: yash <[email protected]>
Co-authored-by: Krishang Nadgauda <[email protected]>
Co-authored-by: Krishang Nadgauda <[email protected]>

Author: 4 people

Diff for: .DS_Store

@@ -597,6 +597,12 @@ contract DropERC1155 is
         emit PrimarySaleRecipientUpdated(_saleRecipient);
     }
 
+    /// @dev Lets a contract admin set the recipient for all primary sales.
+    function setSaleRecipientForToken(uint256 _tokenId, address _saleRecipient) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        saleRecipient[_tokenId] = _saleRecipient;
+        emit SaleRecipientForTokenUpdated(_tokenId, _saleRecipient);
+    }
+
     /// @dev Lets a contract admin update the default royalty recipient and bps.
     function setDefaultRoyaltyInfo(address _royaltyRecipient, uint256 _royaltyBps)
         external

Diff for: contracts/drop/DropERC1155.sol

@@ -3,13 +3,23 @@ pragma solidity ^0.8.0;
 
 import "./interface/IContractMetadata.sol";
 
+/**
+ *  Thirdweb's `ContractMetadata` is a contract extension for any base contracts. It lets you set a metadata URI
+ *  for you contract.
+ *
+ *  Additionally, `ContractMetadata` is necessary for NFT contracts that want royalties to get distributed on OpenSea.
+ */
+
 abstract contract ContractMetadata is IContractMetadata {
     /// @dev Contract level metadata.
     string public override contractURI;
 
     /// @dev Lets a contract admin set the URI for contract-level metadata.
     function setContractURI(string memory _uri) external override {
-        require(_canSetContractURI(), "Not authorized");
+        if (!_canSetContractURI()) {
+            revert ContractMetadata__NotAuthorized();
+        }
+
         _setupContractURI(_uri);
     }
 

Diff for: contracts/feature/ContractMetadata.sol

@@ -3,6 +3,11 @@ pragma solidity ^0.8.0;
 
 import "./interface/IDelayedReveal.sol";
 
+/**
+ *  Thirdweb's `DelayedReveal` is a contract extension for base NFT contracts. It lets you create batches of
+ *  'delayed-reveal' NFTs. You can learn more about the usage of delayed reveal NFTs here - https://blog.thirdweb.com/delayed-reveal-nfts
+ */
+
 abstract contract DelayedReveal is IDelayedReveal {
     /// @dev Mapping from id of a batch of tokens => to encrypted base URI for the respective batch of tokens.
     mapping(uint256 => bytes) public encryptedBaseURI;
@@ -13,14 +18,13 @@ abstract contract DelayedReveal is IDelayedReveal {
     }
 
     /// @dev Returns the decrypted i.e. revealed URI for a batch of tokens.
-    function getRevealURI(uint256 _batchId, bytes calldata _key) public returns (string memory revealedURI) {
+    function getRevealURI(uint256 _batchId, bytes calldata _key) public view returns (string memory revealedURI) {
         bytes memory encryptedURI = encryptedBaseURI[_batchId];
-        require(encryptedURI.length != 0, "nothing to reveal.");
+        if (encryptedURI.length == 0) {
+            revert DelayedReveal__NothingToReveal(_batchId);
+        }
 
         revealedURI = string(encryptDecrypt(encryptedURI, _key));
-
-        // yash - added this, and removed view mutability
-        delete encryptedBaseURI[_batchId];
     }
 
     /// @dev See: https://ethereum.stackexchange.com/questions/69825/decrypt-message-on-chain

Diff for: contracts/feature/DelayedReveal.sol

@@ -1,16 +1,14 @@
 // SPDX-License-Identifier: Apache-2.0
 pragma solidity ^0.8.0;
 
-import "./interface/IClaimConditionsSinglePhase.sol";
-import "./interface/IDrop.sol";
-import "./LazyMint.sol";
+import "./interface/IDropSinglePhase.sol";
 import "../lib/MerkleProof.sol";
 import "../lib/TWBitMaps.sol";
 
-abstract contract DropSinglePhase is LazyMint, IDrop, IClaimConditionsSinglePhase {
+abstract contract DropSinglePhase is IDropSinglePhase {
     using TWBitMaps for TWBitMaps.BitMap;
 
-    /*///////////////////////////////////////////////////x////////////
+    /*///////////////////////////////////////////////////////////////
                             State variables
     //////////////////////////////////////////////////////////////*/
 
@@ -36,6 +34,10 @@ abstract contract DropSinglePhase is LazyMint, IDrop, IClaimConditionsSinglePhas
      */
     mapping(bytes32 => TWBitMaps.BitMap) private usedAllowlistSpot;
 
+    /*///////////////////////////////////////////////////////////////
+                                Errors
+    //////////////////////////////////////////////////////////////*/
+
     /*///////////////////////////////////////////////////////////////
                             Drop logic
     //////////////////////////////////////////////////////////////*/
@@ -68,7 +70,10 @@ abstract contract DropSinglePhase is LazyMint, IDrop, IClaimConditionsSinglePhas
         );
 
         // Verify claim validity. If not valid, revert.
-        bool toVerifyMaxQuantityPerTransaction = _allowlistProof.maxQuantityInAllowlist == 0;
+        // when there's allowlist present --> verifyClaimMerkleProof will verify the maxQuantityInAllowlist value with hashed leaf in the allowlist
+        // when there's no allowlist, this check is true --> verifyClaim will check for _quantity being equal/less than the limit
+        bool toVerifyMaxQuantityPerTransaction = _allowlistProof.maxQuantityInAllowlist == 0 ||
+            claimCondition.merkleRoot == bytes32(0);
 
         verifyClaim(_dropMsgSender(), _quantity, _currency, _pricePerToken, toVerifyMaxQuantityPerTransaction);
 
@@ -90,14 +95,16 @@ abstract contract DropSinglePhase is LazyMint, IDrop, IClaimConditionsSinglePhas
         // Mint the relevant NFTs to claimer.
         uint256 startTokenId = transferTokensOnClaim(_receiver, _quantity);
 
-        emit TokensClaimed(0, _dropMsgSender(), _receiver, startTokenId, _quantity);
+        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) external override {
-        require(_canSetClaimConditions(), "Not authorized");
+        if (!_canSetClaimConditions()) {
+            revert DropSinglePhase__NotAuthorized();
+        }
 
         bytes32 targetConditionId = conditionId;
         uint256 supplyClaimedAlready = claimCondition.supplyClaimed;
@@ -107,7 +114,9 @@ abstract contract DropSinglePhase is LazyMint, IDrop, IClaimConditionsSinglePhas
             targetConditionId = keccak256(abi.encodePacked(_dropMsgSender(), block.number));
         }
 
-        require(supplyClaimedAlready <= _condition.maxClaimableSupply, "max supply claimed already");
+        if (supplyClaimedAlready > _condition.maxClaimableSupply) {
+            revert DropSinglePhase__MaxSupplyClaimedAlready(supplyClaimedAlready);
+        }
 
         claimCondition = ClaimCondition({
             startTimestamp: _condition.startTimestamp,
@@ -134,24 +143,42 @@ abstract contract DropSinglePhase is LazyMint, IDrop, IClaimConditionsSinglePhas
     ) public view {
         ClaimCondition memory currentClaimPhase = claimCondition;
 
-        require(
-            _currency == currentClaimPhase.currency && _pricePerToken == currentClaimPhase.pricePerToken,
-            "invalid currency or price."
-        );
+        if (_currency != currentClaimPhase.currency || _pricePerToken != currentClaimPhase.pricePerToken) {
+            revert DropSinglePhase__InvalidCurrencyOrPrice(
+                _currency,
+                currentClaimPhase.currency,
+                _pricePerToken,
+                currentClaimPhase.pricePerToken
+            );
+        }
 
         // If we're checking for an allowlist quantity restriction, ignore the general quantity restriction.
-        require(
-            _quantity > 0 &&
-                (!verifyMaxQuantityPerTransaction || _quantity <= currentClaimPhase.quantityLimitPerTransaction),
-            "invalid quantity."
-        );
-        require(
-            currentClaimPhase.supplyClaimed + _quantity <= currentClaimPhase.maxClaimableSupply,
-            "exceed max claimable supply."
-        );
+        if (
+            _quantity == 0 ||
+            (verifyMaxQuantityPerTransaction && _quantity > currentClaimPhase.quantityLimitPerTransaction)
+        ) {
+            revert DropSinglePhase__InvalidQuantity();
+        }
+
+        if (currentClaimPhase.supplyClaimed + _quantity > currentClaimPhase.maxClaimableSupply) {
+            revert DropSinglePhase__ExceedMaxClaimableSupply(
+                currentClaimPhase.supplyClaimed,
+                currentClaimPhase.maxClaimableSupply
+            );
+        }
 
         (uint256 lastClaimedAt, uint256 nextValidClaimTimestamp) = getClaimTimestamp(_claimer);
-        require(lastClaimedAt == 0 || block.timestamp >= nextValidClaimTimestamp, "cannot claim.");
+        if (
+            currentClaimPhase.startTimestamp > block.timestamp ||
+            (lastClaimedAt != 0 && block.timestamp < nextValidClaimTimestamp)
+        ) {
+            revert DropSinglePhase__CannotClaimYet(
+                block.timestamp,
+                currentClaimPhase.startTimestamp,
+                lastClaimedAt,
+                nextValidClaimTimestamp
+            );
+        }
     }
 
     /// @dev Checks whether a claimer meets the claim condition's allowlist criteria.
@@ -168,12 +195,17 @@ abstract contract DropSinglePhase is LazyMint, IDrop, IClaimConditionsSinglePhas
                 currentClaimPhase.merkleRoot,
                 keccak256(abi.encodePacked(_claimer, _allowlistProof.maxQuantityInAllowlist))
             );
-            require(validMerkleProof, "not in whitelist.");
-            require(!usedAllowlistSpot[conditionId].get(merkleProofIndex), "proof claimed.");
-            require(
-                _allowlistProof.maxQuantityInAllowlist == 0 || _quantity <= _allowlistProof.maxQuantityInAllowlist,
-                "invalid quantity proof."
-            );
+            if (!validMerkleProof) {
+                revert DropSinglePhase__NotInWhitelist();
+            }
+
+            if (usedAllowlistSpot[conditionId].get(merkleProofIndex)) {
+                revert DropSinglePhase__ProofClaimed();
+            }
+
+            if (_allowlistProof.maxQuantityInAllowlist != 0 && _quantity > _allowlistProof.maxQuantityInAllowlist) {
+                revert DropSinglePhase__InvalidQuantityProof(_allowlistProof.maxQuantityInAllowlist);
+            }
         }
     }
 

Diff for: contracts/feature/DropSinglePhase.sol

@@ -3,6 +3,12 @@ pragma solidity ^0.8.0;
 
 import "./interface/ILazyMint.sol";
 
+/**
+ *  Thirdweb's `LazyMint` is a contract extension for any base NFT contract. It lets you 'lazy mint' any number of NFTs
+ *  at once. Here, 'lazy mint' means defining the metadata for particular tokenIds of your NFT contract, without actually
+ *  minting a non-zero balance of NFTs of those tokenIds.
+ */
+
 abstract contract LazyMint is ILazyMint {
     /// @dev Largest tokenId of each batch of tokens with the same baseURI.
     uint256[] private batchIds;
@@ -17,7 +23,9 @@ abstract contract LazyMint is ILazyMint {
 
     /// @dev Returns the id for the batch of tokens the given tokenId belongs to.
     function getBatchIdAtIndex(uint256 _index) public view returns (uint256) {
-        require(_index < getBaseURICount(), "invalid index.");
+        if (_index >= getBaseURICount()) {
+            revert LazyMint__InvalidIndex(_index);
+        }
         return batchIds[_index];
     }
 
@@ -32,7 +40,7 @@ abstract contract LazyMint is ILazyMint {
             }
         }
 
-        revert("No batch id for token.");
+        revert LazyMint__NoBatchIDForToken(_tokenId);
     }
 
     /// @dev Returns the baseURI for a token. The intended metadata URI for the token is baseURI + tokenId.
@@ -46,7 +54,7 @@ abstract contract LazyMint is ILazyMint {
             }
         }
 
-        revert("No base URI for token.");
+        revert LazyMint__NoBaseURIForToken(_tokenId);
     }
 
     /// @dev Sets the base URI for the batch of tokens with the given batchId.

Diff for: contracts/feature/LazyMint.sol

@@ -7,7 +7,7 @@ import "../lib/TWStrings.sol";
 abstract contract LazyMintERC721 is LazyMint {
     using TWStrings for uint256;
 
-    event TokensLazyMinted(uint256 startTokenId, uint256 endTokenId, string baseURI, bytes extraData);
+    event TokensLazyMinted(uint256 indexed startTokenId, uint256 endTokenId, string baseURI, bytes extraData);
 
     /// @dev the next available non-minted token id
     uint256 public nextTokenIdToMint;

Diff for: contracts/feature/LazyMintERC721.sol

@@ -3,13 +3,21 @@ pragma solidity ^0.8.0;
 
 import "./interface/IOwnable.sol";
 
+/**
+ *  Thirdweb's `Ownable` is a contract extension to be used with any base contract. It exposes functions for setting and reading
+ *  who the 'owner' of the inheriting smart contract is, and lets the inheriting contract perform conditional logic that uses
+ *  information about who the contract's owner is.
+ */
+
 abstract contract Ownable is IOwnable {
     /// @dev Owner of the contract (purpose: OpenSea compatibility)
     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) external override {
-        require(_canSetOwner(), "Not authorized");
+        if (!_canSetOwner()) {
+            revert Ownable__NotAuthorized();
+        }
         _setupOwner(_newOwner);
     }
 

Diff for: contracts/feature/Ownable.sol

@@ -27,12 +27,15 @@ contract Permissions is IPermissions {
         return true;
     }
 
-    function getRoleAdmin(bytes32 role) public view override returns (bytes32) {
+    function getRoleAdmin(bytes32 role) external view override returns (bytes32) {
         return _getRoleAdmin[role];
     }
 
     function grantRole(bytes32 role, address account) public virtual override {
         _checkRole(_getRoleAdmin[role], msg.sender);
+        if (_hasRole[role][account]) {
+            revert Permissions__CanOnlyGrantToNonHolders(account);
+        }
         _setupRole(role, account);
     }
 
@@ -42,7 +45,9 @@ contract Permissions is IPermissions {
     }
 
     function renounceRole(bytes32 role, address account) public virtual override {
-        require(msg.sender == account, "Can only renounce for self");
+        if (msg.sender != account) {
+            revert Permissions__CanOnlyRenounceForSelf(msg.sender, account);
+        }
         _revokeRole(role, account);
     }
 
@@ -58,6 +63,7 @@ contract Permissions is IPermissions {
     }
 
     function _revokeRole(bytes32 role, address account) internal virtual {
+        _checkRole(role, account);
         delete _hasRole[role][account];
         emit RoleRevoked(role, account, msg.sender);
     }

Diff for: contracts/feature/Permissions.sol

@@ -21,6 +21,7 @@ contract PermissionsEnumerable is IPermissionsEnumerable, Permissions {
             if (roleMembers[role].members[i] != address(0)) {
                 if (check == index) {
                     member = roleMembers[role].members[i];
+                    return member;
                 }
             } else {
                 check += 1;
@@ -38,18 +39,8 @@ contract PermissionsEnumerable is IPermissionsEnumerable, Permissions {
         }
     }
 
-    function grantRole(bytes32 role, address account) public override(IPermissions, Permissions) {
-        super.grantRole(role, account);
-        _addMember(role, account);
-    }
-
-    function revokeRole(bytes32 role, address account) public override(IPermissions, Permissions) {
-        super.revokeRole(role, account);
-        _removeMember(role, account);
-    }
-
-    function renounceRole(bytes32 role, address account) public override(IPermissions, Permissions) {
-        super.renounceRole(role, account);
+    function _revokeRole(bytes32 role, address account) internal override {
+        super._revokeRole(role, account);
         _removeMember(role, account);
     }
 

Diff for: contracts/feature/PermissionsEnumerable.sol

@@ -3,6 +3,12 @@ pragma solidity ^0.8.0;
 
 import "./interface/IPlatformFee.sol";
 
+/**
+ *  Thirdweb's `PlatformFee` is a contract extension to be used with any base contract. It exposes functions for setting and reading
+ *  the recipient of platform fee and the platform fee basis points, and lets the inheriting contract perform conditional logic
+ *  that uses information about platform fees, if desired.
+ */
+
 abstract contract PlatformFee is IPlatformFee {
     /// @dev The address that receives all platform fees from all sales.
     address private platformFeeRecipient;
@@ -17,13 +23,17 @@ abstract contract PlatformFee is IPlatformFee {
 
     /// @dev Lets a contract admin update the platform fee recipient and bps
     function setPlatformFeeInfo(address _platformFeeRecipient, uint256 _platformFeeBps) external override {
-        require(_canSetPlatformFeeInfo(), "Not authorized");
+        if (!_canSetPlatformFeeInfo()) {
+            revert PlatformFee__NotAuthorized();
+        }
         _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");
+        if (_platformFeeBps > 10_000) {
+            revert PlatformFee__ExceedsMaxBps(_platformFeeBps);
+        }
 
         platformFeeBps = uint16(_platformFeeBps);
         platformFeeRecipient = _platformFeeRecipient;