An unofficial audit of a Solidity Game smart contract: Wizards & Dragons Game
Tags: solidity, web3, ethereum, blockchain
Published: 2021-12-01 18:26:08

Two days ago [Wizards & Dragons Game](https://twitter.com/wndgame), an ethereum game has released their smart contract to be reviewed publicly.

After following the [Secureum Bootcamp](https://twitter.com/thesecureum) 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](https://etherscan.io/address/0x70d1300f5a28c57f9ebdfc8095086143da6e2a01#code)
- [Traits.sol](https://etherscan.io/address/0x7859557618d788d4cd0eb54d5eb02fcd097fe300#code)
- [WNDGame.sol](https://etherscan.io/address/0xf1a18fd1fd2d47f363d340a74fd3513b63f388ad#code)
- [GP.sol](https://etherscan.io/address/0x7ac9f25995c8ddef712b2ef52162ab14b2e89765#code)
- [Tower.sol](https://etherscan.io/address/0xe1b5a572ee3b9c348189bce0d98aa2f7bcb9f00b#code)
- [SacrificialAlter.sol](https://etherscan.io/address/0x312Cf426B79FFE25aA28329E5087b0f12Fff1d2e#code)

## Audit Finding

### Slither Finding

If you want to know more about this tool you should check it on GitHub: [Slither](https://github.com/crytic/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](https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply) 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](https://swcregistry.io/docs/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](https://docs.openzeppelin.com/contracts/4.x/api/access#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](https://docs.openzeppelin.com/contracts/4.x/erc20#api:presets.adoc#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](https://docs.soliditylang.org/en/v0.8.4/security-considerations.html#use-the-checks-effects-interactions-pattern) 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](https://docs.soliditylang.org/en/latest/units-and-global-variables.html?highlight=block#block-and-transaction-properties).

### 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](https://docs.soliditylang.org/en/latest/units-and-global-variables.html?highlight=block#block-and-transaction-properties).

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](https://github.com/WnDDev/wndcontracts). Still, no specification, documentation, or tests are added.