Users may get extra refund after minting than intended

Summary

During minting, the user sends in appropriate funds to the Editions.sol contract to pay for the required minting fees. The tokens are issued as required & if there is any extra funds left in the contract after the execution, the remaining balance is sent back to the caller.

However, the issue is that if there are any funds remaining in the contract beforehand, they also get included in the refund amount to the user calling the any of the mint functions.

Vulnerability Detail

When calculating the refund amount, the function checks if msg.value & the contract's balance is greater than 0, then sends the whole contract's balance to the caller.

File: Edition.sol

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

There are in total 6 payable functions inside the Editions.sol contract. 4 of them are functions related to minting & the remaining two are for granting & revoking roles.

File: Edition.sol

    function grantRoles(address user_, uint256 roles_)
        public
        payable
        ...

    function revokeRoles(address user_, uint256 roles_)
        public
        payable
        ...

Thus its possible that funds sent when granting or revoking roles inside the contract will be later sent to the user calling the mint functions through _refundExcess(). Or if the contract initially had some balance during the time of creation, the funds in the contract would be sent to the first caller calling any of the mint functions.

Impact

This vulnerability demonstrates leak of value where a user receives a higher refund than actual amount intended.

Code Snippet

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

Tool used

Manual Review

Recommendation

A suitable recommendation would be to check the contract's balance at the start & end of the function & refund the difference to the user instead of sending the whole contract's balance. Or either remove the payable keyword from the grantRoles & revokeRoles functions .

sherlock-audit / 2024-04-titles-judging

Shubham - Users may get extra refund after minting than intended #404