Shaheen - _refundExcess() Functionality will Never Work and Users will Loose Ethers as the Funds will be Stuck in the FeeManager Contract

[sherlock-admin4]

Shaheen

medium

_refundExcess() Functionality will Never Work and Users will Loose Ethers as the Funds will be Stuck in the FeeManager Contract

Summary

_refundExcess() will never work and users will loose ethers as the funds will be stuck in the FeeManager contract (temporarily)

Vulnerability Detail

The refundExcess() function is called after minting tokens to refund any ETH left in the contract after all fees have been collected. As we can see, this function transfers the Edition contract's balance to the user (of course when expected):

    function _refundExcess() internal {
        if (msg.value > 0 && address(this).balance > 0) {
            msg.sender.safeTransferETH(address(this).balance);
        }
    }

To understand the vulnerablity, we need to look at one of the minting functions. There are four minting functions, all utilizing refundexcess() mint(), mintWithComment() & both mintBatch() functions. Let's take only mintWithComment() to undertand the issue:

    function mintWithComment(address to_, uint256 tokenId_, uint256 amount_, address referrer_, bytes calldata data_, string calldata comment_) external payable {
        Strategy memory strategy = works[tokenId_].strategy;
        FEE_MANAGER.collectMintFee{value: msg.value}(this, tokenId_, amount_, msg.sender, referrer_, strategy);
        _issue(to_, tokenId_, amount_, data_);
        _refundExcess();
        emit Comment(address(this), tokenId_, to_, comment_);
    }

As we can see, the mintWithComment() function, first calls the FeeManager's collectMintFee() function, which calculates and takes the fee from the user), then it calls the _issue() function, which mints an NFT to the user and then the _refundExcess() function will called to return the excess amount to the user.

Issue

The problem is, that when the mintWithcomment() function calls the collectMintFee(), it gives all the msg.value to the FeeManager contract. But the FeeManager contract never returns it back to the Edition contract, which means all the excess fee will be stuck in the FeeManager contract and users will never get any excess amount back as the the refundExcess() function only checks and transfers Editions Contracts balance.

Proof-of-Concept

    function test_mintAndRefundExcessFee_PoC() public {
        vm.deal(address(5), 1.0106 ether);
        assertEq(address(feeManager).balance, 0);

        vm.prank(address(5));
        edition.mint{value: 1.0106 ether}(address(1), 1, 1, address(0), new bytes(0));

        assertEq(edition.totalSupply(1), 1);
        assertEq(address(5).balance, 0); //> No Refund
        assertEq(address(1).balance, 0.01 ether);
        assertEq(address(0xc0ffee).balance, 0.0006 ether);
        assertEq(address(feeManager).balance, 1 ether); //> Excess fee Stuck in FeeManager 
    }

Impact

• Loss of funds for the users
• Broken functionality of the protocol
• Temporary freezing of the funds

Code Snippet

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

Tool used

Eyes

Recommendation

Make sure that the FeeManager contracts returns the excess fee amount to the Edition contract.

Duplicate of #269

[ShaheenRehman]

Escalate

Judge Sahab! This finding is a valid dup of #269, In fact this Watson added a coded PoC as well. Thanks!

[sherlock-admin3]

Escalate

Judge Sahab! This finding is a valid dup of #269, In fact this Watson added a coded PoC as well. Thanks!

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

Agree with the escalation, planning to accept and duplicate with #269

[Evert0x]

Result:
High
Duplicate of #269

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ShaheenRehman: accepted