bhilare_ - _refundExcess function won't refund users while minting a token for a given work.

[sherlock-admin4]

bhilare_

medium

_refundExcess function won't refund users while minting a token for a given work.

Summary

While minting a new token, if user sends extra ETH, then it is expected that user will get their refund , which would be done by the _refundExcess function. But it won't be the case and they won't get refund because of wrong implementation of refundExcess function.

Vulnerability Detail

While minting as you can see:

/// @notice Mint a new token for the given work.
    /// @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.
    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();
    }

User pays ETH, which is then being used for calling collectMintFee function in FEE_MANAGER contract, which handles the mint fee payment & then token is being issued and after that _refundExcess is being called, which is expected to handle the refund process.

The issue is :- FEE_MANAGER.collectMintFee{value: msg.value}( this, tokenId_, amount_, msg.sender, referrer_, works[tokenId_].strategy );

while calling this function with sending the value, the user's sent ETH would be then transferred to the FEE_MANAGER contract. And then after processing the mint fee payment, the remaining value of ETH would be in FEE_MANAGER contract only.

But if we see _refundExcess implementation :-

/// @notice Refund any excess ETH sent to the contract.
    /// @dev This function is called after minting tokens to refund any ETH left in the contract after all fees have been collected.

    // @SuS msg.value is not stored here , it is stored in FeeManager.
    function _refundExcess() internal {
        if (msg.value > 0 && address(this).balance > 0) {
            msg.sender.safeTransferETH(address(this).balance);
        }
    }

It is expecting that the ETH will be in the Edition.sol contract only, and hence while checking it is checked whether address(this).balance > 0, if yes then safeTransferETH to the msg.sender

But since the ETH would be forwarded in FEE_MANAGER contract while calling collectMintFee function with { value : msg.value }, balance of Edition contract will always be zero , and hence would not process the refunding.

Hence the implementation is wrong.

Impact

Users will not get refund as being expected by protocol, because of wrong implementation.

Code Snippet

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

Tool used

Manual Review

Recommendation

Instead of address(this).balance , while checking, the balance of FEE_MANAGER should be checked, and accordingly from FEE_MANAGER safeTransferETH should be done.

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