sherlock-audit / 2024-04-titles-judging

6 stars 6 forks source link

Varun_05 - mintBatch function will revert because it tries to pay excess fees than intended which won't be present in the contract. #296

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 3 months ago

Varun_05

high

mintBatch function will revert because it tries to pay excess fees than intended which won't be present in the contract.

Summary

mintBatch function will revert because it tries to pay excess fees than intended which won't be present in the contract which will prevent a use from using that function and this revert is permanent.

Vulnerability Detail

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

Main issue lies in the following line FEEMANAGER.collectMintFee{value: msg.value}( this, tokenIds[i], amounts_[i], msg.sender, address(0), work.strategy ); the above line is executed in for loop so which means it forwards eth amount = msg.value to the fee manager contract each time for loop is executed thus essentially it tries to send total msg.value*tokenIds.length amount of eth to the fee manager which is not possible because only msg.value amount of eth was send by the user for executing this function. So the for loop will fail at the Second iteration making to impossible to use this function.

Impact

This breaks the functionality of this function making it useless unless tokenIds = 1.

Code Snippet

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

Tool used

Manual Review

Recommendation

Exact mitigation of for this is difficult because we can't just divide msg.value by number of token ids because fees collected for each token id can differ as each token id can have different mint fee and other parameters.

Duplicate of #280