Kalogerone - Editions::mintBatch function will always revert

[sherlock-admin3]

Kalogerone

medium

Editions::mintBatch function will always revert

Summary

Editions::mintBatch function used to mint more than 1 tokens to a single receiver will always revert.

Vulnerability Detail

This is the Editions::mintBatch function:

    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();
    }

As we can see, the function is sending the whole msg.value to the FeeManager.sol contract inside a for loop. This means, if the tokenIds_ array has a length > 1, then the function will revert because it has already sent the whole msg.value and at the 2nd loop there are no more funds to send.

Impact

The mintBatch function is unusable as it will always revert if a user wants to mint more than 1 tokens, which is the whole purpose of this function. Otherwise, the user would just use the mint function.

Proof of Code

Paste the following code in Edition.t.sol file:

    uint256[] tokenIds = [1, 1, 1];
    uint256[] amounts = [1, 1, 1];

    function test_mint_batch_doesnt_work() public {
        vm.expectRevert();
        edition.mintBatch{value: 10 ether}(address(1), tokenIds, amounts, new bytes(0));
    }

Running the test with forge test --mt test_mint_batch_doesnt_work -vvvvv we can see that it passes (so it reverts as expected) and it also reverts with OutOfFunds error.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L277

Tool used

Manual Review

Recommendation

My recommendation is to calculate the necessary fee in each loop and only transfer the fee for each mint:

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

+          fee = mintFee(tokenIds_[i], amounts_[i]);
+          FEE_MANAGER.collectMintFee{value: fee}(
-          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();
    }

Duplicate of #280

[sammy-tm]

Escalate

Dup of #280

[sherlock-admin3]

Escalate

Dup of #280

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