Forgotten Runes Warrior Guild contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L480-L500

Vulnerability details

Impact

The values of daStartTime and mintlistStartTime are very important for this contract, as they are used for calculating price of NFT. In function setPhaseTimes() owner of contract can set those variables values and there are some checks but those checks are not enough. for example if admin set daStartTime as 0x by mistake, the price will be calculated as lowestPrice and attacker can buy all the tokens with lowest price in the beginning of auction.

Proof of Concept

This is setPhaseTimes() code and there is no advanced checks for newDaStartTime and newMintlistStartTime values. by setting them with wrong values, users or team can lose funds.

    function setPhaseTimes(
        uint256 newDaStartTime,
        uint256 newMintlistStartTime,
        uint256 newPublicStartTime,
        uint256 newClaimsStartTime
    ) public onlyOwner {
        // we put these checks here instead of in the setters themselves
        // because they're just guardrails of the typical case
        require(
            newPublicStartTime >= newMintlistStartTime,
            'Set public after mintlist'
        );
        require(
            newClaimsStartTime >= newPublicStartTime,
            'Set claims after public'
        );
        setDaStartTime(newDaStartTime);
        setMintlistStartTime(newMintlistStartTime);
        setPublicStartTime(newPublicStartTime);
        setClaimsStartTime(newClaimsStartTime);
    }

daStartTime will be set as newDaStartTime and contract uses daStartTime to calculated currentPrice. so this function should check that newDaStartTime is bigger than block.timestamp. admin shouldn't be able to set daStartTime to a time in the past because if he does that by mistake, attackers can mint tokens for themself with lowest price. mintlistStartTime will bet set as newMintlistStartTime and contract uses mintlistStartTime to stop auction phase, and use finalPrice for next phases. if there wasn't enough time between daStartTime and mintlistStartTime then the price algorithm could not reach the lower amount possible and contract will start mintlistSummon phase with wrong finalPrice (admin coudn't have enough time before mintlistSummon phase to set price). so contract should check that newMintlistStartTime> newDaStartTime + daPriceCurveLength + 1 Hour to avoid this issue.

Tools Used

VIM

Recommended Mitigation Steps

Add more checks to the function. like this:

        require(
            newDaStartTime >= block.timestamp + 1 Hour,
            'Set correct start time'
        );

        require(
           newMintlistStartTime >= newDaStartTime + daPriceCurveLength  + 1 Hour,
            'Set correct start time'
        );

Lines of code

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

Vulnerability details

Impact

Owner will use teamSummon() to mint tokens for specific recipient, but there is no limit on the number of tokens this functions create, so if by mistake owner sends wrong number of tokens, the effect can not be fixed. And there is no reentrancy for this function, so if owner mint toke for bad recipient, then function mint() of ForgottenRunesWarriorsGuild will make external contract call (because of safeMint()) to the recipient, and recipient can call both of the contracts function(right now because of the reentrancy mechanism in mint() attacker can't steal fund)

Proof of Concept

This is the code of teamSummon(), as you can see there is no limit based on count or block.timestamp, so if owner by mistake call this with big number in the middle of phases, then contract will mint most of the tokens and whole processes and phases that community, expected to happen will fail.

    function teamSummon(address recipient, uint256 count) external onlyOwner {
        require(address(recipient) != address(0), 'address req');
        for (uint256 i = 0; i < count; i++) {
            _mint(recipient);
        }
    }

Also this function calls _mint() which calls warriors.mint(recipient) and in warriors.mint() we have:

    function mint(address recipient) public override nonReentrant returns (uint256) {
        require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
        require(_msgSender() == minter, 'Not a minter');
        uint256 tokenId = numMinted;
        _safeMint(recipient, tokenId);
        numMinted += 1;
        return tokenId;
    }

In the line safeMint(), there is going to be an external contract call to recipient, so if the receiver was bad actor, he can call contract ForgottenRunesWarriorsMinter functions, even those with nonReentrant modifier because teamSummon() has no nonReentrant modifier. but of course because all of the functions like bidSummon and mintlistSummon and publicSummon calls mint() (in the ForgottenRunesWarriorsGuild) and it has nonRentrant modifier, attacker can't do any harm in minting but if mint() didn't had this modifier attacker could have made contract to mint more than MAX_WARRIORS with this steps:

1. owner calls teamSummon() with recipient which is contract address and belongs to a bad actor.
2. teamSummon() will call mint() in ForgottenRunesWarriorsGuild for that recipient.
3. mint() will make external call to recipient becasue it's contract address and the code uses _safeMint().
4. attacker contract's token receiver function will be called and attacker contract will call bidSummon(1) (or other summon functions based on the phase).
5. bidSummon() will call mint() in ForgottenRunesWarriorsGuild. (let's assume mint() don't have nonReentrant).
6. because in mint() we don't have check-effect-intract pattern and contract increase numMinted value after the _safeMint(), so this numMInted will be same as before #1 and attacker can bypass numMinted < MAX_WARRIORS, and after the whole transaction finished numMinted will be increased by 2 but and could bypass MAX_WARRIORS.

    function mint(address recipient) public override nonReentrant returns (uint256) {
        require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
        require(_msgSender() == minter, 'Not a minter');
        uint256 tokenId = numMinted;
        _safeMint(recipient, tokenId);
        numMinted += 1;
        return tokenId;
    }

Tools Used

VIM

Recommended Mitigation Steps

Add nonReentrant for teamSummon() and also set limitation condition for count in that function.

In contract ForgottenRunesWarriorsGuild, function mint() the check-effect-interaction pattern has not been followed. This is the code:

function mint(address recipient) public override nonReentrant returns (uint256) {
        require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
        require(_msgSender() == minter, 'Not a minter');
        uint256 tokenId = numMinted;
        _safeMint(recipient, tokenId);
        numMinted += 1;
        return tokenId;
    }

numMinted += 1; should be before than _safeMint(recipient, tokenId). becasue _safeMint() calls external contract, so if in that receiver contract, it tries to use numMinted value, it will get wrong number.

In ForgottenRunesWarriorsMinter, function bidSummon(), it only needs to add msg.sender to daMinters if msg.sender is new minter. so we can add condition to check (daNumMinted[msg.sender] == 0) for adding to daMinters.push(msg.sender) and daMinters list will be lower and issueRefunds() will use lower gas.

if(daNumMinted[msg.sender] == 0) {
        daMinters.push(msg.sender);
}