sherlock-audit / 2024-04-titles-judging

1 stars 1 forks source link

0rpse - refunding excess ETH does not work properly #343

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

0rpse

high

refunding excess ETH does not work properly

Summary

Edition.sol::_refundExcess function tries to send excess ETH sent by the user back to the user after minting but as msg.value is sent to FeeManager.sol refunding is broken.

Vulnerability Detail

Whenever a user mints tokens through Edition.sol msg.value will be forwarded to FeeManager.sol, after minting and fee distribution is complete, Edition.sol will try to refund the excess ETH sent by the user but _refundExcess function only does this if (msg.value > 0 && address(this).balance > 0), because all msg.value has been forwarded to FeeManager.sol this if statement will never be true and all of the excess ETH will stay in FeeManager.sol. At this point users have the ability to call collectMintFee to drain this ETH inside FeeManager.sol.

Impact

Refunding excess ETH does not work, excess ETH sent can be stolen.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider redesigning refund process, you could move refunding operation to FeeManager.sol passing in the minter, as you are already forwarding the msg.value to FeeManager.sol, refund from there.

Duplicate of #269

0rpse commented 1 month ago

Escalate

dupe of https://github.com/sherlock-audit/2024-04-titles-judging/issues/269

sherlock-admin3 commented 1 month ago

Escalate

dupe of https://github.com/sherlock-audit/2024-04-titles-judging/issues/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 1 month ago

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

Evert0x commented 1 month ago

Result: High Duplicate of #269

sherlock-admin4 commented 1 month ago

Escalations have been resolved successfully!

Escalation status: