Forgotten Runes Warrior Guild contest - Cr4ckM3's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L400

Vulnerability details

Impact

For customized minter contract who does not implement ERC20 withdraw interface, the WETH transferred can be locked forever, in other words, lost.

Proof of Concept

_safeTransferETHWithFallback is used to refund ETH to the minters. If ETH is rejected, WETH ERC20 transfer will be used. However I suggest that we should not assume that the all the users implement the minter contract properly. An ERC20 receiver is unable to reject the transfer like they do to the ETH transfer, thus, if minter contracts do not implement the ERC20 withdraw method, the transferred tokens are lost. Or they may have their own self-refund logic we have no idea how to cooperate with.

Maybe skipping such refunding cases is a better idea as the minter can find other way to get their fund back, like self-refund or just contact your team to withdraw the ETH for them.

Tools Used

Code review.

Recommended Mitigation Steps

Just skip minters who reject ETH, leave them to do their self-refund business.

A sample:

function _refundAddress(address minter) private {
        uint256 owed = refundOwed(minter);
        if (owed > 0 && _safeTransferETH(minter, owed)) {
            daAmountRefunded[minter] += owed;
        }
    }

Redundant check

Code

require can be removed as msg.sender can never be zero address. Or do you mean to check require(address(token) != address(0)); ?