_refundExcess implements wrong logic

Summary

refundExcess function implements wrong logic which makes it impossible to refund excess eth to the msg.sender.

Vulnerability Detail

Following is the _refundExcess function

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

It tries to refund excess eth which might be sent by the user when he sends eth for paying for the fees. But issue is that all the eth which user sends while minting is transferred to the fee manager contract when collectMintfee function in fee manager contract is called. Thus there is no eth left in the edition contract. Therefore every time it wouldn't refund any eth . All minting function are as follows and it can be clearly seen that all eth is forwarded to fee manager contract.

function mint(
        address to_,
        uint256 tokenId_,
        uint256 amount_,
        address referrer_,
        bytes calldata data_
    ) external payable override {
        // wake-disable-next-line reentrancy
        FEE_MANAGER.collectMintFee{value: msg.value}(
            this, tokenId_, amount_, msg.sender, referrer_, works[tokenId_].strategy
        );

        _issue(to_, tokenId_, amount_, data_);
        _refundExcess();
    }

    /// @notice Mint a new token for the given work with a public comment.
    /// @param to_ The address to mint the token to.
    /// @param tokenId_ The ID of the work to mint.
    /// @param amount_ The amount of tokens to mint.
    /// @param referrer_ The address of the referrer.
    /// @param data_ The data associated with the mint. Reserved for future use.
    /// @param comment_ The public comment associated with the mint. Emitted as an event.
    /// @dev This function is used to mint a token with a public comment, allowing the mint to be associated with a message which will be emitted as an event.
    function mintWithComment(
        address to_,
        uint256 tokenId_,
        uint256 amount_,
        address referrer_,
        bytes calldata data_,
        string calldata comment_
    ) external payable {
        Strategy memory strategy = works[tokenId_].strategy;
        // wake-disable-next-line reentrancy
        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_);
    }

    /// @notice Mint multiple tokens for the given works.
    /// @param to_ The address to mint the tokens to.
    /// @param tokenIds_ The IDs of the works to mint.
    /// @param amounts_ The amounts of each work to mint.
    /// @param data_ The data associated with the mint. Reserved for future use.
    function mintBatch(
        address to_,
        uint256[] calldata tokenIds_,
        uint256[] calldata amounts_,
        bytes calldata data_
    ) external payable {
        for (uint256 i = 0; i < tokenIds_.length; i++) {
            Work storage work = works[tokenIds_[i]];

            // wake-disable-next-line reentrancy
            FEE_MANAGER.collectMintFee{value: msg.value}(
                this, tokenIds_[i], amounts_[i], msg.sender, address(0), work.strategy
            );

            _checkTime(work.opensAt, work.closesAt);
            _updateSupply(work, amounts_[i]);
        }

        _batchMint(to_, tokenIds_, amounts_, data_);
        _refundExcess();
    }

    /// @notice Mint a token to a set of receivers for the given work.
    /// @param receivers_ The addresses to mint the tokens to.
    /// @param tokenId_ The ID of the work to mint.
    /// @param amount_ The amount of tokens to mint.
    /// @param data_ The data associated with the mint. Reserved for future use.
    function mintBatch(
        address[] calldata receivers_,
        uint256 tokenId_,
        uint256 amount_,
        bytes calldata data_
    ) external payable {
        // wake-disable-next-line reentrancy
        FEE_MANAGER.collectMintFee{value: msg.value}(
            this, tokenId_, amount_, msg.sender, address(0), works[tokenId_].strategy
        );

        for (uint256 i = 0; i < receivers_.length; i++) {
            _issue(receivers_[i], tokenId_, amount_, data_);
        }

        _refundExcess();
    }

Now another scenario for the current implmentation of the _refundExcess is that there can be scenario where mistakenly eth is send to the edition contract but also in that case the excess eth doesn't belongs to the msg.sender , so then also there is mistake in sending the excess eth to the next user which calls the mint function. Any excess eth sent to the contract can be considered as a user mistake but sending the eth to the next msg.sender of the mint functionlity is also wrong therefore it is an issue.

Not only this if the excess eth sent by the user(while paying for the fees) is not returned to the user it can get accumulated in the fee manager contract and this next user who tries to mint can use this in its advantage by sending less eth while calling mint functions. Now as there would be excess eth left in the fee manager contract , the transaction wouldn't revert becasue of less eth send with the function call thus allowing a user to pay less eth as fees and get minted same amount of tokens.

Impact

This can cause excess eth to get accumulated in the fee manager contract and cause loss of eth for the user who sent excess eth. Also it can cause a malicious user to mint same amount of tokens by paying less fees.

Code Snippet

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

Tool used

Manual Review

Recommendation

Instead of checking the eth balance of the edition contract,check whether the eth balance of the fee manager is greater than zero at the end of every mint function and return that amount of eth to the msg.sender.

Duplicate of #269

vsharma4394 commented 4 months ago

Escalate

This is a valid issue . Also i have explained another issue in refundExcess function which can be seen from the following lines

Now another scenario for the current implmentation of the _refundExcess is that there can be scenario where mistakenly eth is send to the edition contract but also in that case the excess eth doesn't belongs to the msg.sender , so then also there is mistake in sending the excess eth to the next user which calls the mint function.
Any excess eth sent to the contract can be considered as a user mistake but sending the eth to the next msg.sender of the mint functionlity is also wrong therefore it is an issue.

Atleast it is a duplicate of #269 . From the above lines i have also explained a unique issue, so it can be considered as a different issue too. Leaving it to the judges.

sherlock-admin3 commented 4 months ago

Escalate

This is a valid issue . Also i have explained another issue in refundExcess function which can be seen from the following lines
Now another scenario for the current implmentation of the _refundExcess is that there can be scenario where mistakenly eth is send to the edition contract but also in that case the excess eth doesn't belongs to the msg.sender , so then also there is mistake in sending the excess eth to the next user which calls the mint function.
Any excess eth sent to the contract can be considered as a user mistake but sending the eth to the next msg.sender of the mint functionlity is also wrong therefore it is an issue.
Atleast it is a duplicate of #269 . From the above lines i have also explained a unique issue, so it can be considered as a different issue too. Leaving it to the judges.

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.