Forgotten Runes Warrior Guild contest - Picodes's results

NC01 - Contract should be coherent between _msgSender and msg.sender

Examples:

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#L101

Useless nonReentrant keywords

The only way to mint is to interact with the Minter, so you only need either the Minter functions either the mint function to be nonReentrant.

You could get rid of nonReentrant keywords and _checkOnERC721Received during mint

First, to me _checkOnERC721Received is mostly useful as a safeguard for NFT traders during the NFT lifecycle, and to me using this process during mint is a useless gas overhead as you don't expect smart contracts to mint by error. Furthermore from past reentrancy attacks it seems this standard has done more harm than good during mint phases. Basically you could use _mint instead of _safeMint

Assuming you remove it, you'd save the external call, and as well all the nonReentrant keywords as it's the only arbitrary external call you're doing.

Concerned code:

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

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

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

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

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

Pausable inheritance could be removed

You already have a way to pause all critical functions which is to use setPhaseTimes, so you don't really need to use Pausable and whenNotPaused

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

tokenId stack variable could be removed

This stack variable could be removed: https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L102

During the refund process, a rework could save you a lot of gas

(Needs a bit of rework). Currently you allow for partial refunds, which is costing you one SLOAD per refund as you need to write in daAmountRefunded . If you remove this feature (so refunds are either done or not), then you can use booleans to store if the refund of someone has been processed or not.

Then you can use bitmaps -using a uint256 to store 256 booleans as bits- (for example https://github.com/Uniswap/merkle-distributor/blob/master/contracts/MerkleDistributor.sol or https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/BitMaps.sol) to store the booleans indicating if refunds have been processed or not. If done wisely, you could end up having only 1 SLOAD and 1 SWRITE per 256 address, which would save you a ton of gas.

Basically when refunding for a batch, you’ll just need to subprocess by batch of 256 address, load the corresponding uint in the bitmap, and save it at the end of the sub processing.

Side Note

Very fan of the on-chain storage trick you came up with !