sherlock-audit / 2024-04-titles-judging

6 stars 6 forks source link

ast3ros - Excess minting fee is not refunded to the user #309

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 3 months ago

ast3ros

high

Excess minting fee is not refunded to the user

Summary

The mint function fails to refund excess ETH sent over the required minting fee due to an oversight in the fee handling process within the contract.

Vulnerability Detail

The mint function is designed to collect minting fees and issue tokens. It sends the entire msg.value, which is the ETH provided by the user, to the FEE_MANAGER to collect fees. This design fails to account for any excess ETH beyond the actual minting fee, as all msg.value is transferred, leaving the contract balance at zero.

    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();
    }

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

However, the _refundExcess function attempts to refund any remaining balance to the user, but since the contract balance is zero after the fee transfer, no refund occurs:

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

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

The impact exits in other functions such as mintWithComment and mintBatch as well.

Impact

This issue leads to users losing excess ETH sent for minting beyond the required fees, affecting trust and usability of the platform.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/2ac20d07d0b4562c0b0ee15e1becbf786f0ed896/wallflower-contract-v2/src/editions/Edition.sol#L228-L242 https://github.com/sherlock-audit/2024-04-titles/blob/2ac20d07d0b4562c0b0ee15e1becbf786f0ed896/wallflower-contract-v2/src/editions/Edition.sol#L512-L516

Tool used

Manual Review

Recommendation

Implement a precise fee calculation before calling FEE_MANAGER.collectMintFee. Only the calculated fee should be sent to the FEE_MANAGER contract, ensuring any excess ETH remains in the contract's balance, allowing the _refundExcess function to properly refund the user.

Duplicate of #269

thangtranth commented 2 months ago

Escalate

This is a valid issue. Please help to check. It's a duplicate of #269

sherlock-admin3 commented 2 months ago

Escalate

This is a valid issue. Please help to check. It's a duplicate of #269

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 commented 2 months ago

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

Evert0x commented 2 months ago

Result: High Duplicate of #269

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status: