threadmodeling - `mintBatch()` for 2+ iterations will use stored funds or will revert

[sherlock-admin4]

threadmodeling

high

mintBatch() for 2+ iterations will use stored funds or will revert

Summary

The use of msg.value in a loop is a mispattern as its value remains fixed during the whole function call. So, for n-iteration of the loop: n * msg.value is sent. Given that the msg.sender didn't send that much: either the contract has enough stored funds to be used for free, or the call will revert

Vulnerability Detail

This is a common issue. Several blog posts explain it (1, 2)

The i:

• Edition.sol#L277-L289

  File: Edition.sol
  277:     function mintBatch(
  278:         address to_,
  279:         uint256[] calldata tokenIds_,
  280:         uint256[] calldata amounts_,
  281:         bytes calldata data_
  282:     ) external payable {
  283:         for (uint256 i = 0; i < tokenIds_.length; i++) {
  284:             Work storage work = works[tokenIds_[i]];
  285: 
  286:             // wake-disable-next-line reentrancy
  287:             FEE_MANAGER.collectMintFee{value: msg.value}(
  288:                 this, tokenIds_[i], amounts_[i], msg.sender, address(0), work.strategy
  289:             );

Impact

Use of stored funds for free (theft of stored funds)

Or DOS

mintBatch doesn't work for arrays of size > 1

Code Snippet

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

Tool used

Manual Review

Recommendation

Assuming the excess msg.value funds aren't stuck in excess in FEE_MANAGER, consider keeping track of address(this).balance between calls on the Edition contract to only forward what's left. Additionally, consider checking that the final balance isn't subtracted by less than the initial msg.value, or it means that the user could use stored funds for free