A user can easily claim all of the distributions when `merkleRoot` is not set

[ddimitrov22]

Severity

Impact: High, as all the tokens can be claimed by a single user

Likelihood: High, as there are no validations checks and a user can create as many accounts as he wants

Description

When createDistribution is called, there are few input parameters that are not validated and are optional as stated by the comments:

    function createDistribution(
        address token,
        bool isERC20,
        uint96 amountPerClaim,
        uint24 walletCount,
        uint40 startTime,
        uint40 endTime,
        bytes32 merkleRoot, // optional
        string calldata title, // optional
        string calldata ipfsCID // optional
    ) external {
        if (token == address(0)) revert MerkleDistributor__InvalidParams('token');
        if (amountPerClaim == 0) revert MerkleDistributor__InvalidParams('amountPerClaim');
        if (walletCount == 0) revert MerkleDistributor__InvalidParams('walletCount');
        if (endTime <= block.timestamp) revert MerkleDistributor__InvalidParams('endTime');
        if (startTime >= endTime) revert MerkleDistributor__InvalidParams('startTime');

       ...
        distribution.merkleRoot = merkleRoot; // optional
        distribution.title = title; // optional
        distribution.ipfsCID = ipfsCID; // optional

        emit Created(distributions.length - 1, token, isERC20, startTime);
    }

This means that the merkleRoot can be set equal to bytes32(0). The problem arises if the merkleRoot is not set because the verification inside claim will be skipped:

    function claim(uint256 distributionId, bytes32[] calldata merkleProof) external {
        Distribution storage distribution = distributions[distributionId];
        ...
        // Verify the merkle proof
        if (distribution.merkleRoot != bytes32(0) && !MerkleProof.verify( //@audit this will be skipped
            merkleProof,
            distribution.merkleRoot,
            keccak256(abi.encodePacked(msg.sender))
        )) revert MerkleDistributor__InvalidProof();

        // Mark it claimed and send the token
        distribution.isClaimed[msg.sender] = true;
        distribution.claimedCount += 1;

        if (distribution.isERC20) {
            IERC20(distribution.token).safeTransfer(msg.sender, distribution.amountPerClaim);
        } else {
            // Only support an ERC1155 token at id = 0
            IERC1155(distribution.token).safeTransferFrom(address(this), msg.sender, 0, distribution.amountPerClaim, "");
        }

        emit Claimed(distributionId, msg.sender);
    }

If distribution.merkleRoot = bytes32(0) the if block will be skipped, then the msg.sender will be marked as claimed and the tokens will be transferred. The problem is that a user can create as many accounts as he wishes and there is absolutely no verification who will claim the distribution as long as it is not the same msg.sender. This can lead to the same user claiming all the tokens from different accounts.

Recommendations

Consider not allowing to create distribution without passing a valid merkleRoot.

[0x3agle]

• I also thought about it. But, while creating a distribution, setting the merkleRoot is optional

function createDistribution(
        address token,
        bool isERC20,
        uint96 amountPerClaim,
        uint24 walletCount,
        uint40 startTime,
        uint40 endTime,
        bytes32 merkleRoot, // optional
        string calldata title, // optional
        string calldata ipfsCID // optional

• In this scenario, the responsibility falls on the creator to set the merkleRoot.

• To mitigate this, there are a couple of approaches:

  • Implement a Warning: A warning that alerts the creator about the risks associated with not setting a merkleRoot. This can be a comment in the code or front-end.
  • Making merkleRoot Mandatory: A more stringent, yet safer approach would be to make the merkleRoot parameter mandatory. This would prevent the function from executing unless a merkleRoot is specified, thereby ensuring that the token distribution cannot be monopolized by a single user.

[sydneyitguy]

Thanks for your suggestion.

Like @0x3agle mentioned, we made the merkleRoot optional intentionally because there's a need for open airdrops (similar use case scenario with BRC20), first-come-first-serve style, even with those multiple alt accounts. They have to pay the gas fees as the minimal acquisition cost anyway.

This is just a tool, so we don't want to restrict user behavior, leaving it as the creator's responsibility. I added a notice comment on the function call and will also add a warning on the front-end.