Forgotten Runes Warrior Guild contest - teddav's results

Lines of code

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

Vulnerability details

Impact

The team (owner of the contract) can rugpull at any time by withdrawing all the funds in the contract. There is no limit to how much can be withdrawn. And the users won't be able to get their refunds.

Proof of Concept

The function setVaultAddress allows the owner of the contract to set the vault to any address. The function withdrawAll allows the owner/the team to withdraw all ETH on the contract. The function forwardERC20s allows the owner/the team to withdraw any ERC20 on the contract.

Tools Used

Manual analysis

Recommended Mitigation Steps

Only allow the withdraw functions / forwardERC20s to be called after the refund process is done

Lines of code

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

Vulnerability details

Impact

Refunds could end up being stuck for all users.

Proof of Concept

First issue: The function _safeTransferETHWithFallback has a first fallback if to send WETH if the ETH transfer fails. But the WETH transfer can fail (silently) too. You need to check the return value of WETH.transfer and revert if it is false. You can see the code of WETH here: https://etherscan.io/address/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2#code

Second issue: If the user you are trying to refund uses a smart contract, the WETH might end up being lost. If the smart contract doesnt accept ETH, then the call will fallback to WETH. But if the contract doesnt have a method to transfer out the WETH, then the WETH will be lost. Which is terrible for the user.

Tools Used

Manual analysis

Recommended Mitigation Steps

Rewrite the _safeTransferETHWithFallback function.

function _safeTransferETHWithFallback(address to, uint256 amount) internal {
        if (!_safeTransferETH(to, amount)) {
            IWETH(weth).deposit{value: amount}();
            require(IERC20(weth).transfer(to, amount), "failed WETH transfer");
        }
    }

For the second issue. I would recommend maybe having 2 different refund functions. One as ETH, and the other one as WETH