thirdweb A-2

Multiwrap.sol supports receiving ETH by auto-wrapping incoming ETH to WETH. It does this by converting native tokens in CurrencyTransferLib through interaction with external WETH contract. After wrapping, the Multiwrap contract holds on to the wrapped native tokens until an unwrap is requested.

However, when a user invokes unwrap() for an asset with underlying ETH, it always reverts, because the WETH contract cannot transfer native tokens back to Multiwrap due to its missing receive function. As a result, the user's ETH is permanently stuck in the WETH contract, and the user cannot retrieve back his assets.

Consider implementing the receive() function in Multiwrap to fix this issue.

In SignatureDrop.sol, reveal() is used to replace placeholder tokenBaseUri for a particular batch with final tokenBaseUri based on previously provided encrypted string. reveal() is protected and callable by a user with privileged role MINTER. It uses and relies on the getRevealURI() to retrieve decrypted final tokenBaseUri. For a proper reveal(), getRevealURI() must not revert.

However, in DelayedReveal.sol, getRevealURI() is a public function and can be called by anyone. Also, this function can only be executed once. Called with a bad input, its last line updates state to cause all followup executions to revert:

function getRevealURI(uint256 _batchId, bytes calldata _key) public returns (string memory revealedURI) {
    bytes memory encryptedURI = encryptedBaseURI[_batchId];
    require(encryptedURI.length != 0, "nothing to reveal.");

    revealedURI = string(encryptDecrypt(encryptedURI, _key));

    delete encryptedBaseURI[_batchId];
}

An attacker may simply invoke getRevealURI() with any key to cause a permanently invalid contract state for a not yet revealed batch. This is due to encryptDecrypt() not reverting even if an incorrect _key is provided by the caller.

Consider changing getRevealURI() visibility to internal. In addition, consider introducing an extra argument to getRevealURI(), e.g. expectedRevealedURI and corresponding guard condition to check if expectedRevealedURI matches revealedURI generated by encryptDecrypt method. This additional check may prevent contract owner from intentionally or accidentally breaking their batch reveal when they provide an incorrect decryption key.

In Multiwrap.sol, an public invocation of PermissionsEnumberable#renounceRole() with a valid role argument can corrupt state in the PermissionsEnumberable#roleMembers variable for that particular role. Take the following example call trace:

PermissionsEnumerable#renounceRole(minter_role, Alice)
    Permissions#renounceRole(minter_role, account)
        Permissions#_revokeRole(minter_role, account)
    PermissionsEnumerable#removeMember(minter_role, account)

And the following implementation of removeMember():

function _removeMember(bytes32 role, address account) internal {
  uint256 idx = roleMembers[role].indexOf[account];
        delete roleMembers[role].members[idx];
        delete roleMembers[role].indexOf[account];
}

When _removeMember() is called with a valid role and unknown account, idx is 0, causing the contract to remove an unrelated member in the following line. This results in a corrupted state.

Consider updating Permissions.sol#renounceRole to check if the account actually has the role that is being renounced.

In Multiwrap.sol, the supportsInterface() function overrides both ERC1155Receiver's and ERC721Upgradeable's implementations:

function supportsInterface(bytes4 interfaceId)
    public
    view
    virtual
    override(ERC1155Receiver, ERC721Upgradeable)
    returns (bool)
{
    return
        super.supportsInterface(interfaceId) ||
        interfaceId == type(IERC721Upgradeable).interfaceId ||
        interfaceId == type(IERC2981Upgradeable).interfaceId;
}

Due to how multiple inheritance works in Solidity, calling super will not invoke the supportsInterface() implementations for both parent contracts. As a result, this contract will not be recognized as an ERC1155Receiver by external contracts, possibly blocking 3rd party integration.

Consider updating supportsInterface() to properly advertise ERC1155Receiver support like so:

function supportsInterface(bytes4 interfaceId)
    public
    view
    virtual
    override(ERC1155Receiver, ERC721Upgradeable)
    returns (bool)
{
    return
        interfaceId == type(IERC2981Upgradeable).interfaceId ||
          ERC1155Receiver.supportsInterface(interfaceId) ||
          ERC721Upgradeable.supportsInterface(interfaceId);
}

In SignatureDrop.sol, the default contract admin can lazy mint a batch with 0 tokens by calling lazyMint(). As a result, the internal identifier for the new empty batch becomes the same as the identifier for the previous batch. Due to this identifier overlap, followup actions targeting the new batch result in changes for the previous batch. This allows an admin to overwrite tokenBaseURI for the previous batch maliciously or accidentally by calling reveal() for new batch as depicted in the following test:

function test_delayedReveal_withNewLazyMintedEmptyBatch() public {
    vm.startPrank(deployerSigner);

    bytes memory encryptedURI = sigdrop.encryptDecrypt("ipfs://", "key");
    sigdrop.lazyMint(100, "", encryptedURI);
    sigdrop.reveal(0, "key");

    string memory uri = sigdrop.tokenURI(1);
    assertEq(uri, string(abi.encodePacked("ipfs://", "1")));

    bytes memory newEncryptedURI = sigdrop.encryptDecrypt("ipfs://secret", "key");
    sigdrop.lazyMint(0, "", newEncryptedURI);
    sigdrop.reveal(1, "key");

    // token uri for token 1 is overwritten and it shouldn't
    string memory newUri = sigdrop.tokenURI(1);
    assertEq(newUri, string(abi.encodePacked("ipfs://secret", "1")));

    vm.stopPrank();
}

Consider adding a guard to prevent SignatureDrop#lazyMint from being invoked with 0 _amount.

Permissions.sol’s implementation allows granting the same role to an account multiple times. It also allows removing a role from an account that doesn't have that role. This may result in unexpected RoleGranted and RoleRevoked event emissions.

Consider adding guards in Permissions.sol to prevent granting the same role to a particular account, and to prevent removing a role from an account that doesn't actually have the target role.

In SignatureDrop.sol, a call to grantRole() results in the PermissionsEnumerable#_addMember() internal function being called two times. As a result, the roleMembers[role].members storage variable contains unwanted duplicate records.

Consider updating PermissionsEnumerable#grantRole to not call _addMember(), since it will already be executed as part of downstream processing.

The SignatureDrop specification describes claimCondition.startTimestamp as follows:

The unix timestamp after which the claim condition applies. The same claim condition applies until the startTimestamp of the next claim condition.

Based on the above description, SignatureDrop users may create a claimCondition to enable token claiming at a specific time in the future. However, in DropSinglePhase.sol's claim function, startTimestamp is not checked. This allows users to start claiming immediately, even if startTimestamp is set in the future.

Consider updating the implementation to check if startTimestamp condition has been satisfied or updating documentation related to startTimestamp to make it clear that it is not enforced.

Multiwrap.sol relies on CurrencyTransferLib#transferCurrencyWithWrapper() for proper operation. In this method, msg.value is used to check if necessary assets have been provided.

However, note that transferCurrencyWithWrapper() is called within a loop. Although not an issue today, if the parent contract later supports holding ETH via an upgrade, the new functionality may be vulnerable to having assets drained from the contract.

Consider not relying on msg.value directly in a library function which can be executed in a loop, and instead refactor code to execute necessary checks on a more higher/appropriate level.

In SignatureDrop's lazyMint(), the TokensLazyMinted event is emitted in following way:

emit TokensLazyMinted(startId, startId + _amount, _baseURIForTokens, _encryptedBaseURI);

DropERC721.sol another contract which has similar functionality emits this event in the following way. Notice difference in second argument.

emit TokensLazyMinted(startId, startId + _amount - 1, _baseURIForTokens, _encryptedBaseURI);

Consider updating TokensLazyMinted event emission in SignatureDrop's lazyMint() to match specification.

Upgradable contracts in the hierarchy of contracts need to have __gap variable in order for future changes not to break contract storage.

Several events could benefit from indexing:

• event OwnerUpdated - prevOwner and newOwner
• event TokensLazyMinted – startTokenId
• event TokenURIRevealed – index
• event DefaultRoyalty – newRoyaltyRecipient
• event RoyaltyForToken – royaltyRecipient
• event PlatformFeeInfoUpdated – platformFeeRecipient
• event TokensClaimed – startTokenId

Missing more detail natspec comments for some of the features (see IClaimCondition.sol as a reference):

• IDelayedReveal.sol, DelayedReveal.sol
• IContractMetadata.sol, ContractMetadata.sol
• IDropSinglePhase.sol
• ILazyMint.sol, LazyMint.sol
• IOwnable.sol, Ownable.sol
• IPermissions.sol, Permissions.sol
• IPlatformFee.sol, PlatformFee.sol
• IPrimarySale.sol, PrimarySale.sol
• IRoyaltyInfo.sol, RoyaltyInfo.sol

Visibility for following methods can be changed from public to external:

• Permissions#getRoleAdmin
• SignatureDrop#burn

Wrap executes three loops, all for iterating tokens.

• 1st loop - to check if asset is allowed
• 2nd loop - wrap > _storeTokens > _setBundle()
• 3rd loop - wrap > _transferTokenBatch

All of the above can be combined in one loop, saving gas costs. The same can be said for unwrap as well, instead of 2 loops, there can be one.

TokenBundle#_setBundle has a code path for updating the bundle, which is unused in Multiwrap’s context. It's not only unused but it's also executed while creating a bundle. As a result, whenever this method is invoked an unnecessary condition is checked each time in the loop, increasing gas costs.

Consider creating two separate functions for create and update.

The following optimizations are done in CurrencyTransferLib:

• If amount is 0, then return (in transferCurrency() and transferCurrencyWithWrapper())
• If sender is the recipient, then return (in safeTransferERC20())

The optimizations done are logically correct. But the issue is that cases when these checks are satisfied are very rare, and optimizing for them, though saves gas costs for these edge cases, increases the gas costs for all other use cases.

Consider removing these optimizations.

Reduce the length of string error messages to reduce contract size. Also consider using Solidity 0.8.4+ feature - Custom Errors .

In method PermissionsEnumerable#getRoleMember, return early when a match is found instead of iterating through the whole array on each invocation.