sherlock-audit / 2024-04-titles-judging

1 stars 1 forks source link

maushish - `_refundExcess()` current implementation could get transactions reverted. #363

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

maushish

high

_refundExcess() current implementation could get transactions reverted.

Summary

The current implementation of the _refundExcess method could make some users spend more funds in the minting process while others can get more funds.

Vulnerability Detail

In the current implementaion we are using address(this).balance there could be a scenarios when gas fees is more than the current contract's balance especially on the ETH MAINNET, due to which this function will simply revert the txn. https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L512

    /// @notice Refund any excess ETH sent to the contract.
    /// @dev This function is called after minting tokens to refund any ETH left in the contract after all fees have been collected.
    function _refundExcess() internal {
        if (msg.value > 0 && address(this).balance > 0) {
            msg.sender.safeTransferETH(address(this).balance);
    }
}

Impact

Some user can lose funds while other user can get more funds in return.

Code Snippet

Consider these Scenarios Scenario A:

Scenario B:

Manual Review

Recommendation

Implement a new mechanism that emits an event whenever user excess funds are stuck in the protocol so that later either ADMIN or some new role can refund this and also have an internal accounting system for such excess funds in the Edition contract.

maushish commented 1 month ago

This issue is a complete duplicate of issue no #269

0xjuaan commented 1 month ago

gas fees are not paid by the contract so this is invalid and definitely not a dupe of #269, let me know if i'm missing something @maushish