Forgotten Runes Warrior Guild contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

The teamSummon method is too centralized.

Proof of Concept

Using the teamSummon function of ForgottenRunesWarriorsMinter, the owner address can mint arbitrary amount of warriors.

If the private key of the owner or minter address is compromised, the attacker will be able to mint an unlimited amount of warriors.

I believe this is unnecessary and poses a serious centralization risk.

Recommended Mitigation Steps

Consider reduce centralization risks with timelock contracts.

1. METADATA_PROVENANCE_HASH is not restricted to be a hash. It could be any kind of string.

2. It was found some transfer without checking the boolean result, ERC20 standard specify that the token can return false if this call was not made, so it's mandatory to check the result of these calls.

• ForgottenRunesWarriorsGuild.sol#L175
• ForgottenRunesWarriorsMinter.sol#L402
• ForgottenRunesWarriorsMinter.sol#L629

3. The setXX methods of ForgottenRunesWarriorsMinter must emit an event in order to be detected by dapps.

• ForgottenRunesWarriorsMinter.sol#L441-L602

4. The contract ForgottenRunesWarriorsMinter is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while paused should be avoided.

1. There are require messages bigger than 32 bytes. More than 32 bytes for message will incur an extra gas costs.

• ForgottenRunesWarriorsGuild.sol#L70
• ForgottenRunesWarriorsGuild.sol#L116
• ForgottenRunesWarriorsMinter.sol#L142-L148
• ForgottenRunesWarriorsMinter.sol#L212

2. Unused constant, the constant R is not used and could be removed

• ForgottenRunesWarriorsGuild.sol#L33

3. Remove nonReentrant modifier from mint method.

It's possible to remove the modifier if the storage changes are produced before the _safeMint method.

    function mint(address recipient)
        public
        override
        returns (uint256)
    {
        require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
        require(_msgSender() == minter, 'Not a minter');
        uint256 tokenId = numMinted++;
        _safeMint(recipient, tokenId);
        return tokenId;
    }

4. Change the incremental logic from i++ to ++i in order to save some opcodes:

• ForgottenRunesWarriorsMinter.sol#L162
• ForgottenRunesWarriorsMinter.sol#L220
• ForgottenRunesWarriorsMinter.sol#L259
• ForgottenRunesWarriorsMinter.sol#L355

5. Redundant require. the require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining'); already checked the previous one.

• ForgottenRunesWarriorsMinter.sol#L136

6. It's compared a boolean value using == false, instead of using the boolean value, or NOT opcode, it's cheaper to use NOT when the value it's false, or just the value without == true, when it's true, because it will use less opcode inside the VM.

• ForgottenRunesWarriorsMinter.sol#L182
• ForgottenRunesWarriorsMinter.sol#L238