Forgotten Runes Warrior Guild contest - tintin's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L610 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L618 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L164 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L175 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L629

Vulnerability details

Impact

In Minter:withdraw and Minter:withdrawAll, send is used to withdraw ETH from the contract to the designated vault address. send only forwards 2300 gas, and gas costs can change in the future. If the vault is a smart contract that requires more than 2300 gas to receive ETH, the call will always fail and thus the team will not be able to withdraw the funds.

There are also other uses of send and transfer that are linked above. I believe medium severity to be valid since the vault address can be changed if needed.

Proof of Concept

ForgottenRunesWarriorsMinter.sol contains withdraw():

    function withdraw(uint256 _amount) public onlyOwner {
        require(address(vault) != address(0), 'no vault');
        require(payable(vault).send(_amount));
    }

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L610

Tools Used

Manual review

Recommended Mitigation Steps

Replace all of these with the suggested call.value function and check the boolean return value. Since these functions are access controlled, re-entrancy is a non issue.

Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

1. No check for out-of-bounds array access when issuing refunds in issueRefunds. When looping through the provided start and end indexes, the endIdx can be >= the length of the array, throwing an error and reverting, wasting gas. https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L355

2. Add additional checks in setPhaseTimes for incorrect daStartTime and mintlistStartTime according to the timeline outlined. Especially for the daStartTime, since the logic around finalPrice is vital to other functions. https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L480-L500