xiaoming90 - Excess ETH of the victim can be stolen by malicious external parties due to re-entrancy attack

[sherlock-admin3]

xiaoming90

high

Excess ETH of the victim can be stolen by malicious external parties due to re-entrancy attack

Summary

Excess ETH of the victim can be stolen by malicious external parties due to re-entrancy attack.

Vulnerability Detail

Per Line 241 below, it is expected that there will be excess ETH residing on the contract at the end of the transaction. The _refundExcess function is implemented with the intention of sweeping excess ETH back to the caller of the mint function at the end of the transaction.

However, the problem is when someone (e.g., Bob) mints a new token for a given work, a number of parties can gain control of the transaction and re-enter the Edition contract. The mint function uses a number of functions that can pass execution control to external parties.

The following is a list of external parties that could re-enter during the minting execution:

1. Fee recipients (e.g., Edition's creator and attribution, Collection and minting referrers) - At Line 236, when the FEE_MANAGER.collectMintFee is executed, native ETH is transferred to the fee recipients, who will gain control of the execution.

2. Token recipients - At Line 240 below, the _issue function will call the ERC1155._mint function internally. The ERC1155._mint contains an ERC1155 hook (onERC1155Received) that will pass the control to the token recipient, who will gain control of the execution.

Once any of the above-mentioned parties gain control of the execution, they can re-enter the Edition contract and execute any of the mint functions (all mint functions will execute the _refundExcess function). This will result in the excess ETH residing on the contract being swept to the malicious party's address instead of Bob's address, effectively stealing Bob's assets.

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

File: 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}(
237:             this, tokenId_, amount_, msg.sender, referrer_, works[tokenId_].strategy
238:         );
239: 
240:         _issue(to_, tokenId_, amount_, data_);
241:         _refundExcess();
242:     }

[!IMPORTANT]

This issue also affects the Edition.mintWithComment and Edition.mintBatch functions. The write-up is the same and is omitted for brevity.

Impact

Loss of assets, as shown in the above scenario. The victim's excess ETH can be stolen by malicious external parties.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider adding a re-entrancy guard to all minting functions to prevent the fee recipient from re-entering the Edition contract during issuing/minting.

function mint(
    address to_,
    uint256 tokenId_,
    uint256 amount_,
    address referrer_,
    bytes calldata data_
- ) external payable override {
+ ) external payable override nonReentrant() {

[pqseags]

Will address

[cducrest]

Escalate

Invalid in the current code version as Edition will not hold any ETH. As presented in the code snippets, the total msg.value eth is transferred from Edition to FeeManager during a mint:

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

This is also the case for Edition.mintWithComment() and Edition.mintBatch().

[sherlock-admin3]

Escalate

Invalid in the current code version as Edition will not hold any ETH. As presented in the code snippets, the total msg.value eth is transferred from Edition to FeeManager during a mint:

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

This is also the case for Edition.mintWithComment() and Edition.mintBatch().

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]

While I believe the escalating Watson is correct about Editions contract not holding ETH, the problem is that the attacker exploits the current call with excess ETH. When fee sent to the external party it can reenter using the excess ETH that initiated the mint. And yes, there is a possibility the users will send excess ETH, cause mint fee is subsequent to change. Hence, the report is not about draining Editions contract, but stealing fund from the user.

Planning to reject the escalation and leave the issue as it is.

[0xjuaan]

@WangSecurity but the entire msg.value is sent to the FeeManager as @cducrest said, so excess ETH will never be in the Edition contract. When the external party re-enters, the balance of the Edition contract will be 0 ether. Not sure how this can be deemed a High.... when the impact is not even possible.

[WangSecurity]

Yes, you're both correct. I believe this is a valid issue cause the fees shouldn't be sent to FeeManager and remain in the Edition, but in the current implementation it's not working like that. Therefore, this bug is valid only if the bug with incorrect msg.value is sent to the FeeManager. Therefore, the future issue rule should be applied here, planning to accept the escalation and invalidate the report.

[Hash01011122]

Agreed with @0xjuaan and @WangSecurity, this issue can be invalidated.

[kennedy1030]

I think that this issue should be valid. Every minting functions calls to _refundExcess function. It means that the Edition.sol is designed to refund all excess ethers to users. So, I think that this issue could not be seen as a result out of a fix to another issue, because refunding is intended in its design.

[WangSecurity]

In the current implementation of the protocol, the _refundExcess function will not be called due to a bug in #269, therefore, this issue will only arise when 269 is fixed. Hence, future issue rule should be applied.

Planning to accept the escalation and invalidate the issue.

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• cducrest: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/titlesnyc/wallflower-contract-v2/pull/1