Forgotten Runes Warrior Guild contest - minhquanym's results

1. Inconsistent in visibility of onlyOwner functions

Impact

In ForgottenRunesWarriorsGuild, all non-view functions except mint has onlyOwner modifier but 2 of them have external visibility and others have public visibility.

Proof of concept

external

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L152 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L157

public

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L52 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L129 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L137 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L145 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L163 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L173

Recommended Mitigation Steps

If owner is EOA, should change all of them to external to save gas.

2. Use safeTransfer instead of transfer

Impact

transfer() might return false instead of reverting, in this case, ignoring return value leads to considering it successful. Use safeTransfer() or check the return value if length of returned data is > 0.

Proof of concept

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

Recommended Mitigation Steps

Use safeTransfer instead

token.safeTransfer(msg.sender, amount);

1. Use bytes32 rather than string

Impact

If data can fit into 32 bytes, then you should use bytes32 datatype rather than string as it is much cheaper in solidity. Basically, Any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.

Occurrences

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L32-L33

Recommended Mitigation Steps

Change string to bytes32

2. Unnecessary check for msg.sender != address(0)

Impact

In function ForgottenRunesWarriorsGuild.forwardERC20s, line 174 check if msg.sender != address(0). It’s unnecessary since msg.sender will never equal address(0) (either EOA address or contract address).

Instead maybe you should check if address(token) != address(0).

Similarly line 628 of ForgottenRunesWarriorsMinter.

Proof-of-concept

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

Recommended Mitigation Steps

Remove line 174 in ForgottenRunesWarriorsGuild and line 628 in ForgottenRunesWarriorsMinter

3. Check before push can keep daMinters list unique

Impact

Check if an address existed in daMinters or not can be done by checking daNumMinted[msg.sender] == 0 It can save gas when we iterate through daMinters to issue refund.

Proof of concept

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L151-L153 daMinters is only updated in line 151. Also in line 153, daNumMinted[msg.sender] += numWarriors and numWarriors > 0 is checked in line 141. So msg.sender is existed in daMinters only when daNumMinted[msg.sender] > 0

Recommended Mitigation Steps

Change line 151 to

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

4. Saving gas by caching states used multiple times

Impact

Function currentDaPrice() is called everytime users call bidSummon and cost significant amount of gas. This function uses multiple states repeatedly to calculate current price. We can save gas by caching some states that been used repeatedly. It will reduced gas cost for users especially when there is a gas war during auction.

Proof of concept

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L275-L297 Using the code in next part reduce average gas usage of bidSummon function from 285939 to 285228 (testset in repo)

Recommend Mitigation Steps

Update function currentDaPrice() to

function currentDaPrice() public view returns (uint256) {
        uint256 _startPrice = startPrice;
        uint256 _lowestPrice = lowestPrice;
        uint256 _daStartTime = daStartTime;
        uint256 _daPriceCurveLength = daPriceCurveLength;
        uint256 _daDropInterval = daDropInterval;

        if (!daStarted()) {
            return _startPrice;
        }
        if (block.timestamp >= _daStartTime + _daPriceCurveLength) {
            // end of the curve
            return _lowestPrice;
        }

        uint256 dropPerStep = (_startPrice - _lowestPrice) /
            (_daPriceCurveLength / _daDropInterval);

        uint256 elapsed = block.timestamp - _daStartTime;
        uint256 steps = elapsed / _daDropInterval;
        uint256 stepDeduction = steps * dropPerStep;

        // don't go negative in the next step
        if (stepDeduction > _startPrice) {
            return _lowestPrice;
        }
        uint256 currentPrice = _startPrice - stepDeduction;
        return currentPrice > _lowestPrice ? currentPrice : _lowestPrice;
}