Forgotten Runes Warrior Guild contest - dirk_y's results

Lines of code

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

Vulnerability details

Impact

Currently it is possible for the team to mint new tokens at any point in the timeline with teamSummon. This could potentially be a problem if the team mints more than totalSupply - maxForSale - maxForClaim. In this instance it would mean that the publicSummon (and also claimSummon) would actually fail to mint, even though numSold < maxForSale.

Although this situation is unlikely to occur deliberately since the team are assumed to be acting in good faith, it could occur accidentally and impact the whole success of the launch if this situation occurred since the community would have been expecting a certain guaranteed amount to be available to them.

I deem this to be of Medium severity since:

"Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions"

Proof of Concept

This situation can occur because teamSummon does not have a counter or limit to how many tokens can be minted through this call. The only condition that has to be satisfied is that totalMinted < maxSupply. See logic here.

Tools Used

VSCode

Recommended Mitigation Steps

teamSummon should also have a counter and a max cap, as with the other summon methods. I propose the following lines to be added to the method:

require(numTeam < maxForTeam);
require(count > 0);
require(numTeam + count <= maxForTeam);
numTeam += count;

where maxForTeam = totalSupply - maxForClaim - maxForSale

Low priority findings

During my audit of this project I found 2 low priority findings:

1. Start times should be >= rather than >

Technically for all methods checking whether a phase has started (e.g. daStarted, mintListStarted etc.), the check should be (note the change of comparator):

return block.timestamp >= daStartTime;

When you say a phase is starting at a particular time, it should start on (not after) that time. In reality this isn't a problem though as it's only 1 second difference.

2. finalPrice should have a flag that states whether it has been updated

Currently, if the DA phase sells out, the finalPrice is set to currentPrice. However, if the DA phase does not sell out, it is the responsibility of the owner to call setFinalPrice for the price calculations to be correct for the follow on phases. It would be safer to have an additional storage variable boolean called something like finalPriceUpdated that can be set to true when the final price has been updated (either automatically or by the owner). The other summon phases could then have an additional require check like:

require(finalPriceUpdated);

This would ensure subsequent phases all have the correct price and ensures that it is not possible for any of the community to mint at the incorrect price.

Gas optimisations

Through this audit I found one area in particular in which gas could be optimised. In bidSummon, every time this method is called the msg.sender is appended to the daMinters array, even if the user has already been added. This results in unnecessary storage usage, which costs gas.

Alternatively I would suggest using a mapping in tandem with this array. For instance you could have a mapping like:

mapping(address => bool) public hasDaMinted;

In bidSummon you could then add the following logic:

if (!hasDaMinted(msg.sender)) {
  hasDaMinted(msg.sender) = true;
  daMinters.push(msg.sender);
}

Although this logic uses slightly more gas during execution, overall I believe you should save gas from lower storage usage costs and in the refund logic (fewer items to iterate over in the array).