search

sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

threadmodeling - `Edition._refundExcess`: misimplemented as funds aren't on `address(this)` but on `FEE_MANAGER` #234

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 4 months ago

threadmodeling

medium

Edition._refundExcess: misimplemented as funds aren't on address(this) but on FEE_MANAGER

Summary

Edition._refundExcess() doesn't work correctly as the excess funds sent from the user are forwarded to the FEE_MANAGER and aren't on the Edition contract anymore.

Vulnerability Detail

The different mint functions on the Edition contract end with a call to _refundExcess().

Its implementations checks that the user sent funds, and sends back any excess stuck in the contract

File: Edition.sol
512:     function _refundExcess() internal {
513:         if (msg.value > 0 && address(this).balance > 0) {
514:             msg.sender.safeTransferETH(address(this).balance);
515:         } 
516:     }

However, the claim here is that funds from msg.value are actually all forwarded to the FEE_MANAGER:

wallflower-contract-v2/src/editions/Edition.sol:
  236:         FEE_MANAGER.collectMintFee{value: msg.value}(
  237              this, tokenId_, amount_, msg.sender, referrer_, works[tokenId_].strategy

  262:         FEE_MANAGER.collectMintFee{value: msg.value}(
  263              this, tokenId_, amount_, msg.sender, referrer_, strategy

  287:             FEE_MANAGER.collectMintFee{value: msg.value}(
  288                  this, tokenIds_[i], amounts_[i], msg.sender, address(0), work.strategy

  311:         FEE_MANAGER.collectMintFee{value: msg.value}(
  312              this, tokenId_, amount_, msg.sender, address(0), works[tokenId_].strategy

If we look at collectMintFee()'s implementation, it will consume all msg.value without refunding anything, even in the even of fee_.amount == 0:

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:     }
...
366:     function _collectMintFee(
367:         IEdition edition_,
368:         uint256 tokenId_,
369:         uint256 amount_,
370:         address payer_,
371:         address referrer_,
372:         Fee memory fee_
373:     ) internal {
374:         if (fee_.amount == 0) return;

Therefore address(this).balance doesn't get increased on Edition for the users to claim the stored funds

Impact

Funds are stuck

Users aren't refunded automatically as expected

Users may get freebies (funds that were actually stuck in Edition but aren't rightfully theirs to claim)

An admin-callable withdraw function exist on FeeManager but isn't supposed to be part of this refund process (still, it decreases the severity from High to Medium)

Code Snippet

Tool used

Manual Review

Recommendation

Either move the function to FeeManager and pass the msg.sender's address as a parameter Or implement a refund mechanism on FeeManager sending back the excess on Edition which will then send back the excess on msg.sender