Shaheen - `mintBatch()` will always revert for the users

[sherlock-admin3]

Shaheen

medium

mintBatch() will always revert for the users

Summary

The mintBatch(address to_,uint256[] calldata tokenIds_,uint256[] calldata amounts_, bytes calldata data_) function will always revert for the users with the OutOfFunds error.

Vulnerability Detail

The mintBatch() function is in-place to allow users to mint multiple tokens for the given works.

    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]];
            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 that this function calls the collectMintFee() in a loop, the collectMintFee takes the fee from the user and then it calls_batchMint() to mint NFTs to the users.

Issue

The problem is that this function calls collectMintFee() in a loop & with all the given msg.value. So when the loop iterates the 2nd time, it doesn't have any funds to give to the collectMintFee(), so the function reverts the execution with the OutOfFunds error.

Proof-of-Concept

    function test_batchMint_outOfFunds_PoC() public {
        uint[] memory tokenIds = new uint[](2);
        tokenIds[0] = 1;
        tokenIds[1] = 2;
        uint[] memory tokenAmts = new uint[](2);
        tokenAmts[0] = 1;
        tokenAmts[1] = 1;
        vm.expectRevert(); //> reverts because of "outOfFunds"
        edition.mintBatch{value: 1 ether}(address(1), tokenIds, tokenAmts, new bytes(0));
    }

Just wanted to add an intresting additional thing as a proof, when we run the above test without the vm.expectRevert() & with foundry verbosity's 3rd level -vvv, this the output we get:

// Ran 1 test for test/editions/Edition.t.sol:EditionTest
// [FAIL. Reason: EvmError: Revert] test_batchMint() (gas: 156239)
// Traces:
//   [156239] EditionTest::test_batchMint()
//     ├─ [142878] Edition::mintBatch{value: 1000000000000000000}(0x0000000000000000000000000000000000000001, [1, 2], [1, 1], 0x)
//     │   ├─ [86455] FeeManager::collectMintFee{value: 1000000000000000000}(Edition: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1, 1, EditionTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000000, Strategy({ asset: 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE, mintFee: 10000000000000000 [1e16], revshareBps: 2500, royaltyBps: 250 }))
//     │   │   ├─ [3000] 0x0000000000000000000000000000000000000001::fallback{value: 10000000000000000}()
//     │   │   │   └─ ← [Return] 
//     │   │   ├─ [0] 0x0000000000000000000000000000000000C0FFEE::fallback{value: 600000000000000}()
//     │   │   │   └─ ← [Stop] 
//     │   │   ├─ emit FeeCollected(edition: Edition: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], work: 1, asset: 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE, fee: 10600000000000000 [1.06e16], referrerShare: 0)
//     │   │   └─ ← [Stop] 
//     │   ├─ [0] FeeManager::collectMintFee{value: 1000000000000000000}(Edition: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 2, 1, EditionTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000000, Strategy({ asset: 0x0000000000000000000000000000000000000000, mintFee: 0, revshareBps: 0, royaltyBps: 0 }))
//     │   │   └─ ← [OutOfFunds] EvmError: OutOfFunds
//     │   └─ ← [Revert] EvmError: Revert
//     └─ ← [Revert] EvmError: Revert

// Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.42ms (192.70µs CPU time)
// 

Impact

• Broken Functionality of the protocol

Code Snippet

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

Tool used

ChonCh

Recommendation

Do not give all of the given msg.value in the first loop's iteration only, caluculate the fee first and then only give that amount as a value to the collectMintFee().

Duplicate of #280

[ShaheenRehman]

Escalate

Judge Sahab! This finding is a valid dup of #280, In fact this Watson added a coded PoC as well. Thanks!

[sherlock-admin3]

Escalate

Judge Sahab! This finding is a valid dup of #280, In fact this Watson added a coded PoC as well. Thanks!

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:

• ShaheenRehman: accepted