threadmodeling - Funds from users can be drained through frontrunning `FeeManager.collectMintFee` with arbitrary inputs

[sherlock-admin4]

threadmodeling

high

Funds from users can be drained through frontrunning FeeManager.collectMintFee with arbitrary inputs

Summary

FeeManager.collectMintFee can be called from anyone with any parameters, hence trigger the transfer of funds from any user, with any arbitrary amount of fee, while the attacker being the referrer and getting a share

Vulnerability Detail

FeeManager.collectMintFee isn't access controlled:

File: FeeManager.sol
183:     function collectMintFee(
184:         IEdition edition_,
185:         uint256 tokenId_,
186:         uint256 amount_,
187:         address payer_,
188:         address referrer_
189:     ) external payable {
190:         _collectMintFee(
191:             edition_, tokenId_, amount_, payer_, referrer_, getMintFee(edition_, tokenId_, amount_)
192:         );
193:     }

Also, the parameters aren't validated. getMintFee() will look-up the strategy used and will multiply it by the quantity down the line:

File: FeeManager.sol
250:     function getMintFee(Strategy memory strategy_, uint256 quantity_)
251:         public
252:         view
253:         returns (Fee memory fee)
254:     {
255:         // Return the total fee (creator's mint fee + protocol flat fee)
256:         return Fee({asset: ETH_ADDRESS, amount: quantity_ * (strategy_.mintFee + protocolFlatFee)});
257:     }

We're then in a _collectMintFee scenario computation each one's share.

The payer (victim) is assumed to have approved the contract for transferring funds and will be the from and feePayer_ address here :

File: FeeManager.sol
448:     function _route(Fee memory fee_, Target memory feeReceiver_, address feePayer_) internal {
449:         // Cross-chain fee routing is not supported yet
450:         if (block.chainid != feeReceiver_.chainId) revert NotRoutable();
451:         if (fee_.amount == 0) return;
452: 
453:         _transfer(fee_.asset, fee_.amount, feePayer_, feeReceiver_.target);
454:     }
...
461:     function _transfer(address asset_, uint256 amount_, address from_, address to_) internal {
462:         if (asset_ == ETH_ADDRESS) {
463:             to_.safeTransferETH(amount_);
464:         } else {
465:             asset_.safeTransferFrom(from_, to_, amount_);
466:         }
467:     }

While the _feeReceivers are computed from storage, this isn't the case for the user-input address referrer_:

File: FeeManager.sol
401:         _route(
402:             Fee({asset: fee_.asset, amount: fee_.amount - protocolShare}),
403:             _feeReceivers[getRouteId(edition_, tokenId_)],
404:             payer_
405:         );
406: 
407:         uint256 referrerShare =
408:             _splitProtocolFee(edition_, fee_.asset, protocolShare, payer_, referrer_);
...
420:         uint256 mintReferrerShare = getMintReferrerShare(amount_, referrer_);
421:         uint256 collectionReferrerShare = getCollectionReferrerShare(amount_, referrers[edition_]);
422:         referrerShare = mintReferrerShare + collectionReferrerShare;
423: 
424:         _route(
425:             Fee({asset: asset_, amount: amount_ - referrerShare}),
426:             Target({target: protocolFeeReceiver, chainId: block.chainid}),
427:             payer_
428:         );

Let's remind here that the user won't gain anything: they'll just be paying fees, while having previously approved the contract for a future operation (be it a specific amount of approval or an infinite approval). If they try to call a mint function after that, they'll need to pay fees again. Given how we are usually offered to sign an approve transaction on Metamask before triggering an operation, this here can be easily frontrunnable, with the attacker earning a share by abusing all of the user's approved funds.

Impact

Users' funds are drained while nothing is gained in return.

Calls to collectMintFee(), in Edition's mint functions, are DOSd by either allowance consumed or funds drained from users

Code Snippet

• https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L401-L428
• https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L448-L467
• https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L250-L257
• https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L183-L193

Tool used

Manual Review

Recommendation

Several paths can be thought of: collectMintFee should be permissioned, like several other functions from FeeManager. Authorized addresses should be kept track of. The approval mechanism can be moved to transfer funds from Edition contracts

[ccashwell]

Per the Q&A, only ETH is supported, so this issue is not exploitable at this time. However, I've taken the suggestion of adding permission checks to the collect*Fee functions to eliminate the risk of being called by any unexpected parties.