Forgotten Runes Warrior Guild contest - VAD37's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L173-L176 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L627-L630

Vulnerability details

Token like USDT known for using non-standard ERC20. (Missing return boolean on transfer).

Contract function forwardERC20 will always revert when try to transfer this kind of tokens.

Impact

Cannot withdraw some special ERC20 token through contract call. Unexpected contract functionality = Medium severity

Migration

Use SafeTransferLib.safeTransfer instead of IERC20 transfer. This accepts ERC20 token with no boolean return like USDT.

Judge has assessed an item in Issue #72 as Medium risk. The relevant finding follows:

Use SafeTransferLib for all ETH send

With the exception of issueRefund, other transfer should allow forward all gas() to finish the transaction. To prevent case of costing more than 23000 gas for transfer.

• Line1
• Line2
• Line3
• Line4 Replace with SafeTransferLib version

QA

Quote from README:

- The times all need to be set correctly
- The `finalPrice` of the Dutch Auction is used in subsequent phases and it set automatically _if_ the DA sells out. If the DA fails to sell out, the owner needs to set this value manually for the subsequent phases to be priced correctly.

Trust multisig owner as sensible actor and all centralization risks here are low and can only happen due to accident or unintended behaviour.

TOC

1. Use SafeTransferLib for all ETH send
2. Missing Events for tracking
3. Duplicate StartTime function and missing require check
4. Centralization risk: missing require check for setting time in correct order
5. Centralization risk: setFinalPrice should only allow set lower price not any price after and during auction
6. Centralization risk: missing selfRefundsStartTime check set time only after auction time
7. Variable that can be immutable

Use SafeTransferLib for all ETH send

With the exception of issueRefund, other transfer should allow forward all gas() to finish the transaction. To prevent case of costing more than 23000 gas for transfer.

• Line1
• Line2
• Line3
• Line4 Replace with SafeTransferLib version

Missing Events for tracking

It is a good practice to use events for tracking important configurations.

At least add event for mint, bid, summon and most importantly setFinalPrice

Duplicate set startTime function and missing require check

daStartTime, mintListStartTime, publicStartTime, claimsStartTime can be set in setPhaseTimes(). It is redundant, cost more gas and missing required check to prevent accident by having same atomic function in public.

Centralization risk: missing require check for setting time in correct order

setDaStartTime, setMintlistStartTime, setPublicStartTime, setClaimsStartTime does not have same require check as setPhaseTimes().

This open up possibility to set wrong configuration that break sale timeline.

It can cause issue when dutch auction end, but owner can restart auction again due to failed auction. Price reaches new minimum when sold out. Already refunded user can get very new refunded price, which can cause confusion when some user now can get refunded again due to lower final price.

Centralization risk: setFinalPrice should only allow set lower price not any price after and during auction

owner can change final price after dutch auction end to help public sale. But refunding logic is based on finalPrice, user might get refunded more than intended if finalPrice change back to higher amount.

So change final price during self refunding period should not be allowed.

Also for transparency, it is much better for admin can only set lower final price after auction end instead of any price.

Centralization risk: missing selfRefundsStartTime check set time only after auction time

Refund amount is based on finalPrice, which can change anytime during dutch auction and by team.

To prevent accident, refund should be required to set way long after dutch action end, even after claimSummon. To give team enough time to set a sensible finalPrice amount.

Otherwise, user might get refunded with different price if time overlap with dutch auction.

Variable that can be immutable

Help remove bloat function from Minter contract to cut down deployment cost. As these address only needed to change once during constructor.

• NFT Warrior Address
• WETH address

Gas

TOC

1. Reentrancy guard for mint function is redundant
2. ForgottenRunesWarriorsGuild.sol mint improvement
3. publicSummon duplicate require check
4. IssueRefund send batch improvement

Reentrancy guard for mint function is redundant

ForgottenRunesWarriorsGuild.sol can remove nonReentrant modifier because Minter contract already have one and only minter can call mint function.

If worry about reentrancy attack, then move line numMinted +=1 to above _safeMint() function to prevent callback inside ERC721 contract.

ForgottenRunesWarriorsGuild.sol mint improvement

Replace these line with

    // save 100 gas from not calling cache number again.    
    uint256 tokenId = numMinted++; // This also prevent any reentrancy attack from minting same token twice
    _safeMint(recipient, tokenId);     

publicSummon duplicate require check

    require(numSold < maxForSale, 'Sold out');
    require(numSold + numWarriors <= maxForSale, 'Not enough remaining');

Both require check if sale number pass maxForSale. Only 2nd require is necessary. First require only useful for information that user might never see.

IssueRefund send batch improvement

• First, there is no need for refunding with WETH as it complicated thing, and it is not necessary. User should own up their mistake for using contract does not have default Ether fallback.
• Second, use SafeTransferLib instead of current _safeTransferETH()

Improvement based on DisperseApp and SafeTransferLib. New batch refund cut around ~380 gas per address.

Replace issueRefund with:

    
    function issueRefunds(uint256 startIdx, uint256 endIdx)
        public
        onlyOwner
        nonReentrant
    {
        require(selfRefundsStarted(), 'Self refund period not started'); //Prevent accident refund during auction as price might change.
        uint256 end = endIdx + 1;
        for (uint256 i = startIdx; i < end; i++) {
            address minter = daMinters[i];
            uint256 owed = refundOwed(minter);
            if (owed > 0) {
                daAmountRefunded[minter] += owed;
                safeTransferETH(minter, owed);
            }
        }
    }

    function safeTransferETH(address to, uint256 amount) internal {
        bool success;
        assembly {
            // I agree that refund call should not forward all gas()
            // Contract DOS fallback can spend all gas received. 
            success:= call(30000, to, amount, 0, 0, 0, 0)
        }
        // save WETH as fallback for griefer in batch call is unnecessary. User can call selfRefund to get WETH as fallback.
        // Because if the receiver contract do not accept Ether fallback, what will happen if they do not support ERC20 withdrawal as well. The money will be stuck.
        if(!success) {
            IWETH(weth).deposit{value: amount}();            
            IERC20(weth).transfer(to, amount);        
        }
    }