threadmodeling - The full `msg.value` Ether amount is sent to the `feeManager` without refund even when fee amount is zero

[sherlock-admin3]

threadmodeling

medium

The full msg.value Ether amount is sent to the feeManager without refund even when fee amount is zero

Summary

When calling TitlesCore.createEdition, there's a collectCreationFee() function being called on feeManager.

Whether the feeAmount there is zero or a certain amount: any msg.value from the user will always be fully sent to the feeManager without any refunds.

Vulnerability Detail

On TitlesCore.createEdition, we reach the following line: TitlesCore.sol#L138

File: TitlesCore.sol
138:         feeManager.collectCreationFee{value: msg.value}(edition_, tokenId, msg.sender);

The implementation of collectCreationFee actually receives the funds on the FeeManager contract but doesn't give anything back in the event of an amount sent while fee is 0:

• FeeManager.sol#L166-L171

File: FeeManager.sol
166:     function collectCreationFee(IEdition edition_, uint256 tokenId_, address feePayer_)
167:         external
168:         payable
169:     {
170:         Fee memory fee = getCreationFee();
171:         if (fee.amount == 0) return; 

Also, in the event of an excess sent, the exact fee.amount is forwarded but the excess stays stuck in the FeeManager contract:

• FeeManager.sol#L461-L467

  File: FeeManager.sol
  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:     }

Nowhere in the call flow is there an automatic refund mechanism for the user here.

While it could be argued that the admin can call withdraw() for each user, this is still not a viable solution, whereas the solution is simple to implement

Impact

Funds are stuck, very often.

Case by case withdraw can be made by the admin, but the mistake can easily be avoided with a code fix.

Code Snippet

• https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L461-L467
• https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L166-L171
• https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/TitlesCore.sol#L138

  Tool used

Manual Review

Recommendation

Implement a function refunding the excess