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
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 functionsaddAdmin
,removeAdmin
,updateOriginAccess
-
SacrificialAlter.sol
: missing events for functionsaddAdmin
,removeAdmin
,updateOriginAccess
,setContracts
,setType
,setExchangeAmt
,updateOriginAccess
-
Tower.sol
: missing events for functionssetRescueEnabled
,_payDragonTax
-
Traits.sol
: missing events for functionssetWnD
,uploadTraits
-
WnD.sol
: missing events for functionsaddAdmin
,removeAdmin
,updateOriginAccess
,withdraw
,setPaidTokens
,setContracts
-
WNDGame.sol
: missing events for functionssetContracts
,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 addressaddr1
. - 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.
Top comments (0)