sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

KupiaSec - Design Flaw in `Edition::_refundExcess` Function Implementation #380

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 5 months ago

KupiaSec

high

Design Flaw in Edition::_refundExcess Function Implementation

Summary

The Edition::_refundExcess function is designed to refund any excess Ether sent by minters during the mint process. However, the current implementation directs the entire Ether balance of the Edition to the FeeManager contract, leaving no funds remaining to facilitate the intended refund functionality. As a result, the Edition::_refundExcess function is unable to return excess Ether payments to the minters. On the other hand, anyone can withdraw the entire Eth balance of the Edition through Edition.mint functions which calls Edition::_refundExcess.

Vulnerability Detail

The Edition::_refundExcess function is invoked during the edition minting process.

The current implementation of the Edition::_refundExcess function is as follows::

Edition.sol
512: function _refundExcess() internal {
513:         if (msg.value > 0 && address(this).balance > 0) {
514:             msg.sender.safeTransferETH(address(this).balance);
515:         }
516:     }

As evident from the provided code, the Edition::_refundExcess function refunds remaining balance of the Edition after the minting process. However, the current implementation directs the Ether to the FeeManager contract, leaving no funds remaining to facilitate the intended refund functionality.

Edition.sol
228: function mint(
229:         address to_,
230:         uint256 tokenId_,
231:         uint256 amount_,
232:         address referrer_,
233:         bytes calldata data_
234:     ) external payable override {
235:         // wake-disable-next-line reentrancy
236:         FEE_MANAGER.collectMintFee{value: msg.value}(//@audit send eth to the fee manager
237:             this, tokenId_, amount_, msg.sender, referrer_, works[tokenId_].strategy
238:         );
239: 
240:         _issue(to_, tokenId_, amount_, data_);
241:         _refundExcess();
242:     }

On the other hand, if there is some Eth in Edition, anyone can receive the entire Eth balance of Edition by calling this function. The only prevelidged address should be able to withdraw the Eth balance of the Edition.

Impact

Code Snippet

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

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

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

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

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

Tool used

Manual Review

Recommendation

It is recommended to implement the refund functionality for excess mint fees in the FeeManager contract, rather than the Edition contract.

Duplicate of #269

KupiaSecAdmin commented 4 months ago

Escalate

This is a duplicate of #269

sherlock-admin3 commented 4 months ago

Escalate

This is 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 4 months ago

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

Evert0x commented 4 months ago

Result: High Duplicate of #269

sherlock-admin2 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: