funkornaut - Users are not refunded extra eth

[sherlock-admin4]

funkornaut

high

Users are not refunded extra eth

Summary

The Edition contract attempts to refund its users who overspend when minting Works but it fails to do so.

Vulnerability Detail

The _refundExcess function is present in all minting functions within the Edition contract. The goal of this function is to refund users if they for some reason send excess ether to the contract when minting tokens.

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

However, this will not happen because the ether sent to Edition for minting tokens is immediately sent to the FeeManager contract. The _refundExcess function only attempts to give back any ether that lay dormant within the Editions contract when it should try to refund users if there is excess ether within the FeeManager contract.

PoC: this test will fail when trying to assert the user balance is equal to what we expect it should be if they were refunded and will log the balance of the FeeManager where we can see the extra funds sit. Run forge test --mt test_neverRefundedForMint -vvvv to see the full logs.

    function test_neverRefundedForMint() public {
        address user = makeAddr("user");
        vm.deal(user, 1 ether);

        vm.prank(user);
        uint256 mintEth = 0.0106 ether * 5;
        uint256 extraEth = 0.069 ether;
        uint256 expectedUserBalanceAfterMint = 1 ether - mintEth; //extra eth should be sent back to user
        edition.mint{value: mintEth + extraEth}(address(1), 1, 5, address(0), new bytes(0));
        console.log("user balance: ", address(user).balance);
        console.log("edition balance: ", address(edition).balance);
        console.log("fee manager balance: ", address(feeManager).balance);
        assertEq(address(user).balance, expectedUserBalanceAfterMint);
        assertEq(edition.totalSupply(1), 5);
        assertEq(address(1).balance, 0.05 ether);
        assertEq(address(0xc0ffee).balance, 0.0030 ether);
    }

Impact

Users never get refunded if they overspend ether as the protocol attempts to.

Code Snippet

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

Tool used

Manual Review Foundry

Recommendation

Refund the user from the FeeManager contract or enable strict checks that the user can not overspend ether when minting tokens.

Duplicate of #269

[0xjuaan]

Escalate

valid dupe of #269

[sherlock-admin3]

Escalate

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

[WangSecurity]

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

[sammy-tm]

@WangSecurity

I Apologise for not bringing this up during escalations, but shouldn't the "Excess ETH" issues be low/info?

I say this because I just observed another contest's escalations, "Excess Eth" issue was marked invalid by the judge because it's a matter of user input validation and invalid according to Sherlock rules. The user is responsible for keying in the correct value.

Ref : https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update-judging/issues/6

[WangSecurity]

Yeah, that's the thing I've been thinking the entire morning. Could you help understand the mint fee? Does it change every block or it's set for each collection and doesn't change?

[sammy-tm]

Mint fee is set by the creator when they publish a work. They can alter it later on if they wish, but it does not automatically change each block as such.

As far as "each" collection is concerned, every collection has multiple works, and each work has its own creator, hence a different mint fee for each.

[WangSecurity]

Thank you for the information, you can check #269 to see my comment and why I think it should remain valid. Planning to accept the escalation and duplicate with #269

[sammy-tm]

@WangSecurity

I agree with your decision, since mint fee can be altered, a user may send a transaction with the previous mint fee and in the next block the creator might have altered it and this can cause a loss for the user if the new mint fee is lower than the previous one. Medium is more appropriate.

[Evert0x]

Result: High Duplicate of #269

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: accepted