Forgotten Runes Warrior Guild contest - GimelSec's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L145 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L275 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L441 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L564 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L571

Vulnerability details

Impact

In Dutch Auction phase, the current price is determined in currentDaPrice(), It uses daStartTime, daPriceCurveLength and daDropInterval to calculate the current price. However these variables can be changed during the phase unconditionally, which means that the price can be manipulated by the owner.

Proof of Concept

The owner can change these variables unconditionally

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

    function setDaStartTime(uint256 _newTime) public onlyOwner {
        daStartTime = _newTime;
    }

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

    function setDaPriceCurveLength(uint256 _newTime) public onlyOwner {
        daPriceCurveLength = _newTime;
    }

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

    function setDaDropInterval(uint256 _newTime) public onlyOwner {
        daDropInterval = _newTime;
    }

Tools Used

vim

Recommended Mitigation Steps

These variables shouldn’t be changed after the phase started. So just add a check in these setter

    function setDaPriceCurveLength(uint256 _newTime) public onlyOwner {
        require(!daStarted(), 'Auction has started');
        daPriceCurveLength = _newTime;
    }

Summary

We list 2 low-critical findings:

• (Low) floating pragma
• (Low) It’s better to use a constant to define weth

(Low) floating pragma

Impact

Floating pragma may cause unexpected compilation time behaviour and introduce unintended bugs.

Proof of Concept

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

Recommended Mitigation Steps

Don't use ^, lock pragma to ensure compiler version. e.g. pragma solidity 0.8.0;

(Low) It’s better to use a constant to define weth

Impact

There is a specific weth address on blockchain, it’s better to use a constant for avoiding typo or phishing address when setWethAddress, which will also lead to _refundAddress failing.

Proof of Concept

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

Recommended Mitigation Steps

Use a constant rather than using setWethAddress.

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

ForgottenRunesWarriorsMinter.sol
162:        for (uint256 i = 0; i < numWarriors; i++) {
220:        for (uint256 i = 0; i < numWarriors; i++) {
259:        for (uint256 i = 0; i < count; i++) {
355:        for (uint256 i = startIdx; i < endIdx + 1; i++) {

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}

Use ++XXX rather than XXX += 1 to save gas

Proof of Concept

ForgottenRunesWarriorsGuild.sol
104:        numMinted += 1;

ForgottenRunesWarriorsMinter.sol
248:        numClaimed += 1;

Recommendation

Use ++numMinted and ++numClaimed to save gas.