Forgotten Runes Warrior Guild contest - sseefried's results

QA Report

This report contains links to a private fork of the contest repo. The GitHub user code423n4 has been added as a collaborator so that the judges can view the code.

Remarks/Recommendations

• the video where you walked through the design of the system and the smart contracts was a great idea and I hope that this becomes standard practice for future sponsors.
• The functions that returned ETH and tokens accidentally sent to the address were something I haven't seen before and appeared quite useful.

Low Risk: Order of operations in calculating dropPerStep loses precision

Impact

The following code in the currentDaPrice function loses precision because a divide is performed too early.

uint256 dropPerStep = (startPrice - lowestPrice) /
    (daPriceCurveLength / daDropInterval);

An equivalent expression which loses less precision is:

uint256 dropPerStep = (startPrice - lowestPrice) * daDropInterval / daPriceCurveLength

It just so happens that these two expressions are equal for the values which are specified in the contract:

• startPrice = 2.5 ether
• lowestPrice = 0.6 ether
• daPriceCurveLength = 380 minutes
• daDropInterval = 10 minutes

However, if you set daPriceCurveLength = 379 minutes you immediately get two different values. The value using the existing expression is 51351351351351351 while the new, more precise, value is 50131926121372031. This is a difference of approximately 2.4%, which is quite large.

This affects how much money that can be made from the NFT auction as the dropPerStep is calculated to be higher than it otherwise would have been.

As long as daDropPerInterval evenly divides into daPriceCurveLength there is no problem with the original calculation.

Recommended Mitigation Steps

Use the more precise calculation of dropPerStep

uint256 dropPerStep = (startPrice - lowestPrice) * daDropInterval / daPriceCurveLength

Low-risk: Validate constants in constructor and parameters in set* functions

FIXME:sseefried

constructor(IForgottenRunesWarriorsGuild _warriors, address _weth) {
    setWarriorsAddress(_warriors);
    setWethAddress(_weth);
    setVaultAddress(msg.sender);
}

At a minimum I would recommend

require(_warriors.MAX_WARRIORS >= maxForSale);
require(maxForSale + maxForClaim <= _warriors.MAX_WARRIORS);
require(startPrice == finalPrice); // should be equal at beginning
require(daStartTime < mintlistStartTime);
require(mintlistStartTime < publicStartTime);
require(publicStartTime < claimsStartTime);

Add suitable error messages so the reason for a failure to a call to the constructor is clear.

I would also recommend similar checks in the set* functions e.g. setStartPrice, setLowestPrice etc. as one might be under pressure when one has to use these functions.

For example something like this might be a good idea.

function setLowestPrice(uint256 _newPrice) public onlyOwner {
    require(_newPrice <= startPrice);
    lowestPrice = _newPrice;
}

In general, it would be worth spending some time thinking about the invariants. i.e. those things that should always be true of the state of the contracts storage locations.

Low Risk: Potential loss of funds if finalPrice not set when self refunds start

The refundOwed function is defined as:

function refundOwed(address minter) public view returns (uint256) {
    uint256 totalCostOfMints = finalPrice * daNumMinted[minter];
    uint256 refundsPaidAlready = daAmountRefunded[minter];
    return daAmountPaid[minter] - totalCostOfMints - refundsPaidAlready;
}

It's possible that finalPrice has not been set to a different value than its initial value. This is stated in the following code which is part of bidSummon:

if (numSold == maxDaSupply) {
    // optimistic: save gas by not setting on every mint, but will
    // require manual `setFinalPrice` before refunds if da falls short
    finalPrice = currentPrice;
}

It is also mentioned in the YouTube video.

In the case that the user minted an NFT for less than finalPrice this will lead to totalCostOfMints being larger than daAmountPaid[minter], which will lead to an integer underflow bug. Fortunately, this is caught by Solidity 0.8.0 and will just lead to a revert.

Although refundOwed is just a view function it is called by function _refundAddress, which means that calls to function selfRefund would revert. This means that users can't be refunded until the correct finalPrice has been set.

This has been classified as a low risk issue because the fix is just to call function setFinalPrice and set it to the correct value. However, until this is done the user is denied service.

Proof of Concept

This hardhat test shows that a revert does occur under the conditions specified above, and also that the owner calling setFinalPrice fixes the problem.

Recommended Mitigation Steps

Add a require statement similar to the following in setSelfRefundsStartTime

function setSelfRefundsStartTime(uint256 _newTime) public onlyOwner {
    require(finalPrice < startPrice, "finalPrice hasn't been set yet");
    selfRefundsStartTime = _newTime;
}

and similarly for selfRefund

function selfRefund() public nonReentrant {
    require(selfRefundsStarted(), 'Self refund period not started');
    require(finalPrice < startPrice, "finalPrice hasn't been set yet");
    _refundAddress(msg.sender);
}

This will also lead to a revert but at least the user knows why.

Low Risk: Return values of ERC20.transfer are not checked

Impact

The return values of ERC20 transfers are not checked at the following locations:

• ForgottenRunesWarriorsMinter.sol:402
• ForgottenRunesWarriorsMinter.sol:629
• ForgottenRunesWarriorsGuild.sol:175

Not all ERC20 tokens have the same behaviour on transfer. Some revert on failure but others just return false. In this particular case the owner of the contract has control over the weth contract address so this bug can only be assessed as being low risk.

The first occurrence of this issue is important since _safeTransferETHWithFallback is indirectly called by selfRefund and the other refunding functions. This could mean that users don't end up receiving their funds.

The last two occurrences are only used to to transfer out tokens that are accidentally sent to to the contracts so they are of less consequence.

Proof of Concept

See lines of code above

Recommended Mitigation Steps

The mitigation is to rewrite all uses of transfer to check the return value. For example:

function _safeTransferETHWithFallback(address to, uint256 amount) internal {
    if (!_safeTransferETH(to, amount)) {
        IWETH(weth).deposit{value: amount}();
        (bool success, _) = IERC20(weth).transfer(to, amount);
        require(success, "Transfer failed");
    }
}

Non-critical: ensure startPrice and finalPrice match at beginning

It seems intended behaviour that startPrice be equal to finalPrice at contract initialisation. To make sure this is the case just set the initial value of finalPrice equal to startPrice

uint256 public finalPrice = startPrice;

As it currently stands, it's error prone to use a repeated value of 2.5 ether

Non-critical: currentPrice calculation could be simplified

The code is currently:

if (stepDeduction > startPrice) {
    return lowestPrice;
}
uint256 currentPrice = startPrice - stepDeduction;
return currentPrice > lowestPrice ? currentPrice : lowestPrice;

However, this would allow currentPrice = startPrice - stepDeduction to be very close to 0 which you don't really want.

You could replace the code with the following which is closer to your intent

if (stepDeduction > startPrice - lowestPrice) {
    return lowestPrice;
}
return startPrice - stepDeduction;

The code is not incorrect as it is. It could just be a bit clearer.

Non-critical: Don't use magic numbers for min and max warriors

The following code appears in bidSummon:

require(
    numWarriors > 0 && numWarriors <= 20,
    'You can summon no more than 20 Warriors at a time'
);

Define MIN_WARRIORS and MAX_WARRIORS constants instead or perhaps even make them public constants that the owner can set. e.g. just like setDaStartTime and the other set* functions.

Non-critical: _safeTransferETH should check that to address exists

The Solidity documentation states that:

Warning

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

_safeTransferETH is indirectly called from _refundAddress which, in turn, is called from three functions: issueRefunds, refundAddress and selfRefund.

The only possibly dangerous call is refundAddress which has signature function refundAddress(address minter). However, if called with the wrong minter address owed would be calculated as 0 in _refundAddress and no call to _safeTransferETH.

For this reason this issue is classified as Non-critical.

Function _safeTransferETH is safe only because it is called from functions that meet the precondition that the to address exists. However, if the code is modified at a future date, this might not hold anymore. It is best to check for the existence of the to address within function _safeTransferETH.