sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

Dots - Edition.sol doesn't actually refunt excess ETH #245

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

Dots

high

Edition.sol doesn't actually refunt excess ETH

Summary

Edition.sol doesn't actually refunt excess ETH.

Vulnerability Detail

When minting tokens, all of the ETH gets sent to the FeeManager contract, which would leave the Edition.sol contract empty to begin with. So any excess ETH would actually get stuck in the FeeManager contract.

Impact

The user will not recieve his excess ETH back.

Code Snippet

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

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/main/wallflower-contract-v2/src/editions/Edition.sol#L512-L516

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

Tool used

Manual Review

Recommendation

Think of a way to return the excess ETH from FeeManager.sol.

Duplicate of #269

Dots69 commented 3 months ago

@Hash01011122 isn't this a dup of https://github.com/sherlock-audit/2024-04-titles-judging/issues/269 ?

0xjuaan commented 3 months ago

Escalate

dupe of #269

sherlock-admin3 commented 3 months ago

Escalate

dupe 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.

cvetanovv commented 3 months ago

I agree with the escalation. This issue is a duplicate of #269.

Evert0x commented 3 months ago

Result: High Duplicate of #269

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: