Forgotten Runes Warrior Guild contest - pauliax's results

• It is a good practice to explicitly implement interfaces, that is IForgottenRunesWarriorsGuild and IForgottenRunesWarriorsMinter to enforce compile-time checks if they do match.

• Function initialize can be called unlimited times, usually init indicates that it should only be called once.

• Any reason why withdrawAll is payable?

  function withdrawAll() public payable onlyOwner

• selfRefund could also validate that the sale has ended, e.g.:

  require(block.timestamp >= daStartTime + daPriceCurveLength)

• Consider using EnumerableSet to store daMinters and avoid duplicates.

• High trust in owner privileges. While I assume that the owner is trustable, I still must mention this in the report. Basically, an owner is a god of the contracts and can set any parameter anytime with no validations leaving many paths to negatively impact users. My suggestion is to consider at least adding 'paused' modifier to all the config set functions and maybe some delay when the changes take effect, e.g. to change the config parameter you first have to pause the contract, then initiate the change which will be satisfied in 10 minutes. This will leave users some time to react and adjust to these changes.

• You can leave only the second check, it should cover both cases:

  require(numSold < maxDaSupply, 'Auction sold out');
  require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');

• You can just change the condition to i <= endIdx to eliminate extra math operation here:

  for (uint256 i = startIdx; i < endIdx + 1; i++)