An unofficial audit of a Solidity Game smart contract: Wizards & Dragons Game

Two days ago Wizards & Dragons Game, an ethereum game has released their smart contract to be reviewed publicly.

After following the Secureum Bootcamp about Smart Contract security I decided to take this opportunity and put into practice everything I've learned during this Bootcamp period.

Premise

I'm not a professional auditor, I'm just a full-stack web2 developer that is learning everything in web3 step by step. So take this as a developer that is making his homework :D

This audit is an unordered list of what I've found while I reviewed the smart contract. They are not well-formatted and mostly rushed because I have done this review in ~5 hours during a single day. I have full-time web2 work so I've done this audit before and after my day job.

Not having a specification document nor documentation available, only a few code comments, and only a couple of hours of time I spent most of the time reading the code of the contracts and trying to create a mental map of how those contracts were working and interacting each other.

No tests were provided with the code so I would need to create all of them from scratch, something impossible to do in a short period of time.

Contracts on which the audit was based

• WnD.sol
• Traits.sol
• WNDGame.sol
• GP.sol
• Tower.sol
• SacrificialAlter.sol

Audit Finding

Slither Finding

If you want to know more about this tool you should check it on GitHub: Slither.

Slither is a Solidity static analysis framework written in Python 3. It runs a suite of vulnerability detectors, prints visual information about contract details, and provides an API to easily write custom analyses. Slither enables developers to find vulnerabilities, enhance their code comprehension, and quickly prototype custom analyses.

I have started these findings from number 100 because I've done the Slither run after all the manual review (as you should).

WD-100: check gpToken.transferFrom return value

I see in the code that there are 2 parts in the code where you execute gpToken.transferFrom without checking the result of the operation.

WD-101: check all divisons

In solidity, numbers get automatically rounded when divided and the result is a floating-point number. Check all the code for cases where the division is done before the multiplication.

Always do the multiplication first and division after.

Example: uint256 encodedLen = 4 * ((data.length + 2) / 3); in Traits.sol
Check Documentation to know more.

WD-102: WNDGame stuck money?

It seems that the mint function in WNDGame.sol receives ether to mint tokens but there's not a withdraw method to withdraw them. The only withdraw function I see is in the WnD.sol contract that does not seem to have a way to receive/get redirected eth from WNDGame.sol

General

WD-001: Floating pragma

In all the contracts the pragma version of the compiler is not locked pragma solidity ^0.8.0;.
See SWC-103 for more information.

WD-002: Hardhat console imported in deployed contract

In both Tower.sol and WNDGame.sol I see that the import "hardhat/console.sol"; is still in the code. It should be only on the dev version of the project and never shipped with the production version. It just increases the contract size and the gas cost of deployment.

As far as I can see console.log() is not used inside contract code.

WD-003: import not used in interfaces

IWnDGame.sol is importing importing OpenZeppelin IERC721Enumerable.sol but not using to extend the IWnDGame from it.

WD-004: missing explanation about lastWrite mechanism

I see in a lot of the contract's code the usage of lastWrite to check lastWrite[tx.origin] < block.number. The developer should add more documentation on the reason behind those checks.

WD-005: Emission of events missing for critical actions

I see that many critical actions are missing event emission. For example GP.sol is missing events for actions like addAdmin, removeAdmin, updateOriginAccess.

• GP.sol: missing events for functions addAdmin, removeAdmin, updateOriginAccess
• SacrificialAlter.sol: missing events for functions addAdmin, removeAdmin, updateOriginAccess, setContracts, setType, setExchangeAmt, updateOriginAccess
• Tower.sol: missing events for functions setRescueEnabled, _payDragonTax
• Traits.sol: missing events for functions setWnD, uploadTraits
• WnD.sol: missing events for functions addAdmin, removeAdmin, updateOriginAccess, withdraw, setPaidTokens, setContracts
• WNDGame.sol: missing events for functions setContracts, setTreasureChestId, addToWhitelist, setPublicSaleStart, setMaxGpCost, payTribute, makeTreasureChests, sellTreasureChests, sacrifice

Some events are for security and transparency reasons, some events are to allow web3/external applications to use contract data to display information.

WD-006: Event does not have indexed parameters

In Tower.sol all three events TokenStaked, WizardClaimed, DragonClaimed does not have indexed parameters. Each event in solidity can have at max 3 indexed parameters.

Indexed parameters allow web3 applications to filter events by those parameters. It's super important to build a successful smart contract to allow external resources to filter by those.

WD-007: create an external contract for the lastWrite mechanism

The lastWrite mechanism is used in GP.sol, SacrificialAlter.sol and WnD.sol but in all three are implemented from scratch using the same code.

For both security, gas consumption, and code readability it would be better if a new utility contract is created and all the code is added to that contract. Other contracts that need that mechanism can simply inherit from it.

WD-009: replace admins mechanism with AccessControl

The admin mechanism could be replaced with OpenZeppelin AccessControl. In this case, you would automatically also get the implementation of important events when a role is granted or removed.

Another benefit is that the OZ implementation automatically do all the input checks

The GP.sol contract could inherit directly from the OpenZeppelin ERC20PresetMinterPauser contract.

You should also think to add a Pause mechanism in order to block the minting/burning in case of a critical situation.

WD-010: add admin mechanism should validate the address

All the addAdmin and removeAdmin functions should check to not give to the 0 address the admin role

SacrificialAlter.sol

WD-011: setContracts should revert if _gp address is 0 address

Doing so you could remove the requireContractsSet and use the pause function when you need to block the

WD-012: maxSupply in setType can be == 0?

Without a full specification or documentation doc and without any comments in code is difficult to understand if the check is needed

WD-013: exchangeAmt in setExchangeAmt can be == 0?

Setting it to 0 means that people are able to mint without paying GP.
Without a full specification or documentation doc and without any comments in code is difficult to understand if the check is needed.

WD-014: possible reentrancy issue

Please always follow the Checks-Effects-Interactions Pattern from Solidity docs to prevent reentrancy issues.

Every time you do an external call always remember to update your state and emit events before it. For example in the burn and mint methods. It's true that both methods check if the sender is the admin but even an admin could exploit the contract.

WD-015: updateOriginAccess is updating incorrectly lastWrite[tx.origin].time

In the function you are doing lastWrite[tx.origin].time = uint64(block.number); when instead you should use block.timestamp

Be aware that block.timestamp is a uint256 (same for block.number) as noted by the Solidity Documentation.

Traits.sol

WD-016: unbound array

looping over uploadTraits that is an unbound array could raise an auto-of-gas denial of service. it seems that traitData at max can contain 2^8 items so check that traits.length is less than 2^8.

WD-017: setWnD wndNFT could be set 0x address

Check that the _wndNFT parameter of setWnD function is != 0x address

WD-018: BASE 64 code could be replaced by an external library

You could replace the code of the base64 function by installing the original library and including it (like an external library as OpenZeppelin): https://github.com/Brechtpd/base64

WnD.sol

WD-019: check that _maxTokens respect what's written in the comments

In comments you say

// max number of tokens that can be minted: 60000 in production
uint256  public maxTokens;

but in the contract's constructor, you set maxTokens directly from the parameter. If the correct number of max tokens to be minted is 60000 just remove it from the constructor and declare it as a constant. If you want to still be parametrized think about setting it as immutable to reduce gas cost.

If maxTokens is set to 0 no one would be able to mint tokens.

WD-020: add checks on setContracts parameters

Check that those contracts address are != 0 address. If so you could remove the requireContractsSet and just use the paused flag/modifier reducing the logic complexity and having always a correct state.

WD-021: generate is an internal method that returns a WizardDragon but its return value is never used

In the mint function generate is called but the return value is never used. Think about using it instead of accessing to tokenTraits[minted] or just remove the returns statement.

WD-022: updateOriginAccess assign the incorrect value to lastWrite[tx.origin].time

As far as I get it should be lastWrite[tx.origin].time = uint64(block.timestamp)
Be aware that block.timestamp is a uint256 (same for block.number) as noted by the Solidity Documentation.

It's important to set the correct value because lastWrite[tx.origin].time is used as an input of the random function in generate

WD-023: _paidTokens parameter of setPaidTokens should have the same int type of PAID_TOKENS

WD-024: check setPaidTokens parameter value

PAID_TOKENS is used in WNDGame.sol. It could happen that the owner of the contract set a number equal to 0 and all the minted tokens in WNDGame would be minted for free.

WD-025: be aware of modifier orders in setPaused

If the requireContractsSet revert you will not be able to pause/unpause the contract. Orders of function modifiers matter!

WNDGame.sol

WD-026: define presalePrice as a constant if it never change

WD-027: add param checks in setContracts

Like other contracts, you could add checks on those addresses to be sure to be different from 0x address and different one between the other (gptoken cannot be equal to traits address!). In that case, you can remove the requireContractsSet and just use the pause modifier if needed.

WD-028: startedTime can be resetted multiple time.

If the owner calls setPublicSaleStart multiple time during the contract life he can reset the value of startTime that is used in currentPriceToMint

WD-029: addToWhitelist can reset an account whitelisting allowing it to mint more than 2 tokens in whitelist period

Scenario:

• addToWhitelist is called whitelisting Alice address addr1.
• Alice mint 2 items in whitelist period
• Admin call again addToWhitelist with Alice's address as a parameter. Alice whitelisting metadata gets reset and can mint again.

Given that the contract is not tracking all these methods with events it's impossible to know that the admin has reset the whitelist of some addresses.

WD-030: add nonReentrancy on both makeTreasureChests and sellTreasureChests?

Kinda tired right now but I think that you should add a reentrancy guard in there because they are internally calling the altar mint/burn that makes an external call. Double-check it.

Update 1: an official GitHub repository

The team behind the game has provided an updated version of the contracts on their GitHub repository. Still, no specification, documentation, or tests are added.