Forgotten Runes Warrior Guild contest - marximimus's results

QA report

Missing @return tags

There are multiple functions where the @return tag is missing.

1. In function currentDaPrice

272: /**
273:  * @notice returns the current dutch auction price
274:  */
275: function currentDaPrice() public view returns (uint256) {

2. In function daStarted

299 /**
300:  * @notice returns whether the dutch auction has started
301:  */
302: function daStarted() public view returns (bool) {

3. In function mintlistStarted

306 /**
307:  * @notice returns whether the mintlist has started
308:  */
309: function mintlistStarted() public view returns (bool) {

4. In function publicStarted

313 /**
314:  * @notice returns whether the public mint has started
315:  */
316: function publicStarted() public view returns (bool) {

5. In function claimsStarted

220 /**
321:  * @notice returns whether the claims phase has started
322:  */
323: function claimsStarted() public view returns (bool) {

6. In function selfRefundsStarted

327 /**
328:  * @notice returns whether self refunds phase has started
329:  */
330: function selfRefundsStarted() public view returns (bool) {

7. In function numDaMinters

299 /**
300:  * @notice returns the number of minter addresses in the DA phase (includes duplicates)
301:  */
302: function numDaMinters() public view returns (uint256) {

8. In function refundOwed

    /**
     * @notice returns the amount owed the address
     * @param minter address the address of the account that wants a refund
     */
    function refundOwed(address minter) public view returns (uint256) {

9. In function _safeTransferETH

    /**
     * @notice Transfer ETH and return the success status.
     * @dev This function only forwards 30,000 gas to the callee.
     * @param to account who to send the ETH to
     * @param value uint256 how much ETH to send
     */
    function _safeTransferETH(address to, uint256 value)

Use ! instead of == false

It is highly recommended to use ! instead of == false in following lines:

In file ForgottenRunesWarriorsMinter.sol:

182: require(mintlistMinted[msg.sender] == false, 'Already minted');
238: require(claimlistMinted[msg.sender] == false, 'Already claimed');

Gas optimization report

Unnecessary checks in function currentDaPrice

Checks found at the end of the function currentDaPrice are unnecessary. As long as all values are set correctly (that is daPriceCurveLength is not smaller than daDropInterval), this function will always return startPrice - stepDeduction.

Code:

292: if (stepDeduction > startPrice) {
293:     return lowestPrice;
294: }
295: uint256 currentPrice = startPrice - stepDeduction;
296: return currentPrice > lowestPrice ? currentPrice : lowestPrice;

Caching variables on stack

Consider caching some variables and reading them from stack in order to optimize gas usage. The following variables should be cached:

1. Variables numSold and maxDaSupply in function bidSummon

Code:

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

154: numSold += numWarriors;

156: if (numSold == maxDaSupply) {

Proposed changes:

uint256 _numSold = numSold;
uint256 _maxDaSupply = maxDaSupply;

136: require(_numSold < _maxDaSupply, 'Auction sold out');
137: require(_numSold + numWarriors <= _maxDaSupply, 'Not enough remaining');

_numSold += numWarriors;

154: numSold = _numSold;

156: if (_numSold == _maxDaSupply) {

2. Variable numSold in function mintlistSummon

Code:

177: require(numSold < maxForSale, 'Sold out');

193: numSold += 1;

Proposed changes:

uint256 _numSold = numSold;

177: require(_numSold < maxForSale, 'Sold out');

193: numSold = _numSold + 1;

3. Variables numSold and maxForSale in function publicSummon

Code:

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

219: numSold += numWarriors;

Proposed changes:

uint256 _numSold = numSold;
uint256 _maxForSale = maxForSale;

207: require(_numSold < _maxForSale, 'Sold out');
208: require(_numSold + numWarriors <= _maxForSale, 'Not enough remaining');

219: numSold = _numSold + numWarriors;

4. Variable numClaimed in function claimSummon

Code:

234: require(numClaimed < maxForClaim, 'No more claims');

248: numClaimed += 1;

Proposed changes:

uint256 _numClaimed = numClaimed;
234: require(_numClaimed < maxForClaim, 'No more claims');

248: numClaimed = _numClaimed + 1;

5. Variables daStartTime, startPrice, daPriceCurveLength and daDropInterval in function currentDaPrice

Code:

279: if (block.timestamp >= daStartTime + daPriceCurveLength) {
280:     // end of the curve
281:     return lowestPrice;
282: }
283:
284: uint256 dropPerStep = (startPrice - lowestPrice) /
285:     (daPriceCurveLength / daDropInterval);
286:
287: uint256 elapsed = block.timestamp - daStartTime;
288: uint256 steps = elapsed / daDropInterval;
289: uint256 stepDeduction = steps * dropPerStep;

// This line has been changed to address the first issue
return startPrice - stepDeduction;

Proposed changes:

uint256 _daStartTime = daStartTime;
uint256 _daPriceCurveLength = daPriceCurveLength;

279: if (block.timestamp >= _daStartTime + _daPriceCurveLength) {
280:     // end of the curve
281:     return lowestPrice;
282: }

uint256 _startPrice = startPrice;
uint256 _daDropInterval = daDropInterval;

284: uint256 dropPerStep = (_startPrice - lowestPrice) /
285:     (_daPriceCurveLength / _daDropInterval);

287: uint256 elapsed = block.timestamp - _daStartTime;
288: uint256 steps = elapsed / _daDropInterval;
289: uint256 stepDeduction = steps * dropPerStep;

// This line has been changed to address the first issue
return _startPrice - stepDeduction;

Don't initialize variables with default values

1. In function bidSummon:

Code:

162: for (uint256 i = 0; i < numWarriors; i++) {

2. In function publicSummon

Code:

220: for (uint256 i = 0; i < numWarriors; i++) {

3. In function teamSummon

Code:

259: for (uint256 i = 0; i < count; i++) {

Long revert strings

Consider making revert strings not longer than 32 characters in order to save gas. The following revert strings are longer than 32 characters:

In file ForgottenRunesWarriorsMinter.sol:

142: 'You can summon no more than 20 Warriors at a time' (49 characters)
148: 'Ether value sent is not sufficient' (34 characters)
212: 'You can summon no more than 20 Warriors at a time' (49 characters)

Use != 0 instead of > 0

Consider changing comparisons whether a value is greater than 0 to comparisons whether a value is not equal to 0 in order to save gas.

In file ForgottenRunesWarriorsMinter.sol:

141: numWarriors > 0 && numWarriors <= 20

Unnecessary check in function forwardERC20s

In function forwardERC20s there is a check whether msg.sender is the null address. However this can never be the case, so this check should be removed.

Code:

174: require(address(msg.sender) != address(0));