Gala Games - GalaV2

CertiK20 Apr 2023Files named in the report

Audit

CertiK reviewed Gala (Gala Games - GalaV2) on 20 Apr 2023: 13 findings, 0 critical; all marked fixed.

No open critical findings

Open the report Open Gala’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings13

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	0	0
Medium	0	0	1
Low	0	0	12

MediumZero Address Can be `blocklisted`Fixed

The `BLOCKLISTER_ROLE` can `blocklist()` the zero address. This would prevent all mints and burns and it may not be immediately clear what is causing the issue.

Low`_totalMinted` vs. `_totalSupply`Fixed

### `Gala` Token - Version 1 In version 1 of the `Gala` token, the `_totalMinted` value is only updated during calls to `mint()`, where it is updated as: ```solidity _totalMinted = _totalMinted.add(amount); ``` In the hook `_beforeTokenTransfer()`, the update to this value is checked against the set `_cap` when minting: ```solidity require(totalMinted().add(amount) <= cap(), "ERC20Capped: cap exceeded"); ``` Since `_totalMinted` is only updated in `mint()` and is checked against the `_cap` value, this means that only 50 billion `GALA` will ever be minted, and that as users burn the token, the `_totalSupply` will monotonically decrease towards 0. --- ### `GalaV2` While `_totalMinted` is still updated and checked during functions `mint()` and `mintBulk()` in the same way for `GalaV2`, the value `_totalMinted` is now updated in functions `burn()` and `burnFrom()` as follows: ```solidity _totalMinted = _totalMinted - amount; ``` This means that as tokens are burned, an equivalent amount becomes available again to be minted. As such, this new update to `_totalMinted` makes the value consistent with `_totalSupply` throughout the lifetime of the `GalaV2` token. Indeed, the `_cap` on `_totalMinted` is also a `_cap` on the `_totalSupply`, making the values equivalent in all respects, within the current contract's set up.

LowCostly Operation Inside LoopFixed

Accessing storage variables in a loop can be costly in terms of gas consumption. ```solidity=86 _totalMinted = _totalMinted + amounts[i]; ```

LowDiscussion on `MINTER_ROLE`Fixed

There is custom functionality created for accomplishing granting or revoking the `MINTER_ROLE`: `addMinter()` and `removeMinter()`. Moreover, the functions `mint()` and `mintBulk()` also perform specific checks to ensure that the `MINTER_ROLE` is also not blocklisted. Please provide more information on the types of accounts you plan to grant the `MINTER_ROLE`. Is it within the design plans of the project to allow for multiple accounts to take this role; will general users be given this privilege, where there may be a need to blocklist bad actors?

LowInconsistency Between `mint()` and `mintBulk()`Fixed

Function `mint()` allows for a zero input `amount`, while each `amounts[i]` must be positive in `mintBulk()`

LowInconsistent Use of `msg.sender` and `_msgSender(Fixed

The child contract uses both `msg.sender` and `_msgSender()` in its functions, while the inherited contracts consistently use `_msgSender()`. If `_msgSender()` is ever updated to be anything other than `msg.sender`, this may cause inconsistencies.

LowInheritance `PausableUpgradeable` is not InitialiFixed

Inherited contract `PausableUpgradeable` is not initialized through call to `__Pausable_init()` in the `initialize()` function.

LowMinters can be added and removed when contract isFixed

Functions `addMinter()` and `removeMinter()`, which grant the `MINTER_ROLE`, can only be called when the contract is not `_paused`; however, `grantRole()` and `revokeRole()` may still be called for any address and role, even when the contract is `_paused`.

LowMissing Zero Address ValidationFixed

The cited address input is missing a check that it is not `address(0)`.

LowRedundant `notBlocklisted` and `whenNotPaused` ChFixed

The checks using the `notBlocklisted` and `whenNotPaused` modifiers are redundant in the following functions, since the function calls interact with hook `_beforeTokenTransfer()`, covering all possible cases: - `mint()`; - `mintBulk()`; - `burn()`; - `burnFrom()`

Show 3 more

LowRedundant Checks on Admin RolesFixed

Functions `addMinter()`, `addUpgrader()`, `addBlocklister()`, and `transferAdmin()` all have redundant checks to modifier `onlyRole`. The aforementioned functions specify that only the `DEFAULT_ADMIN_ROLE` may call these functions; by default, this is the `adminRole` assigned to all roles. The redundancy also exists within functions `removeMinter()`, `removeUpgrader()`, and `removeBlocklister()`. In the case where `DEFAULT_ADMIN_ROLE` account calls a function like `addMinter()`, the check on the `_msgSender()` confirming they are the `DEFAULT_ADMIN_ROLE` is performed twice; once at the beginning of `addMinter()` and once at the execution of `grantRole()`. --- It is noted that if the contract is ever upgraded, each `role` may be assigned an `adminRole` distinct from `DEFAULT_ADMIN_ROLE`. This can be done through adding additional functionality which interacts with internal function `_setRoleAdmin()`. For example, it is possible that another "MinterAdmin" is assigned separate from the `DEFAULT_ADMIN_ROLE` as the `adminRole` for `MINTER_ROLE`. In such a case, this "MinterAdmin" would be able to call `grantRole()` and `revokeRole()` directly, but would not be able to call function `addMinter()`, since this function specifies that only `DEFAULT_ADMIN_ROLE` may perform the call. Furthermore, `DEFAULT_ADMIN_ROLE` would cease to be able to call `addMinter()` in this instance.

LowThe keyword `super` is unnecessaryFixed

The use of the `super` keyword is not necessary since there is no override and there is a well-defined path to the internal `_pause()` and `_unpause()` functions inherited from `ERC20PausableUpgradeable`, and to the public `hasRole()` function `from `AccessControlUpgradeable`.

LowVariable Can Be Declared As `constant`Fixed

The variable `_cap` assigned in the initializer is never modified throughout the contract and is a hardcoded value. Instead, the variable can be initialized before the function call, with keyword `constant`, to optimize gas usage.