sammy - `Edition.sol::mintBatch()` will always revert for `tokenIds_.length` greater than 1

[sherlock-admin3]

sammy

high

Edition.sol::mintBatch() will always revert for tokenIds_.length greater than 1

Summary

The mintBatch() function is used to mint multiple tokens in one transaction. However, doing so will cause the function to revert.

Vulnerability Detail

The mintBatch() loops through the tokenIds_ array and calls the FEE_MANAGER.collectMintFee() function to collect the fee for each mint :

    function mintBatch(
        address to_,
        uint256[] calldata tokenIds_,
        uint256[] calldata amounts_,
        bytes calldata data_
    ) external payable {
        for (uint256 i = 0; i < tokenIds_.length; i++) {
            Work storage work = works[tokenIds_[i]];

            // wake-disable-next-line reentrancy
            FEE_MANAGER.collectMintFee{value: msg.value}(
                this, tokenIds_[i], amounts_[i], msg.sender, address(0), work.strategy
            );

            _checkTime(work.opensAt, work.closesAt);
            _updateSupply(work, amounts_[i]);
        }

        _batchMint(to_, tokenIds_, amounts_, data_);
        _refundExcess();
    }

This logic is incorrect as in the first iteration of the loop, msg.value amount of wei is sent to the FEE_MANAGER contract. Subsequently, during the collectMintFee() function call, the FEE_MANAGER contract spends only the mint fee amount and keeps the rest of msg.value in the contract. Because of this, during the next iteration of the loop, the Edition.sol contract does not have enough balance to call the FEE_MANAGER.collectMintFee() with the same msg.value amount as in the first iteration. This will cause the transaction to revert.

Note that the value of msg.value remains constant in a single function call and the Edition.sol contract does not hold any balance before the mintBatch() function is called.

Impact

DOS/ loss of functionality/ unexpected revert

Code Snippet

Tool used

Manual Review

Recommendation

Make the following changes in the function :

-            FEE_MANAGER.collectMintFee{value: msg.value}(
-                this, tokenIds_[i], amounts_[i], msg.sender, address(0), work.strategy
+            uint256 mintFee_ = FEE_MANAGER.getMintFee(work.strategy, amounts_[i]).amount;
+            FEE_MANAGER.collectMintFee{value: mintFee_}(
+                this, tokenIds_[i], amounts_[i], msg.sender, address(0), work.strategy  
             );

Duplicate of #280

[sammy-tm]

Escalate This is a dup of #280 and is valid

[sherlock-admin3]

Escalate This is a dup of #280 and is valid

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

Agree with the escalation, planning to accept and duplicate with #280

[Evert0x]

Result: Medium Duplicate of #280

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• sammy-tm: accepted