Read the findings of the 2022 Web3 Security Report

The given input is missing the check for the non-zero address.

For example:
- contract `game`: `_admin` in the function `setAdmin()`; `_petAddress`, `NEEAddress`, `AQDCAddress`, `proxyRegistryAddress` and `recipient` in the function `constructor()`; `_petAddress`, `NEEAddress` and `AQDCAddress` in the function `setResourceAddress()`; `recipient` in the function `setRecipient()`;
- contract `INO`: `_admin` in the function `setAdmin()`; `ornamentAddr`, `USDTAddr`, `AQDCAddress`, and `recipient` in the function `constructor()`; `ornamentAddr`, `NEEAddress` and `AQDCAddress` in the function `setResourceAddress()`; `recipient` in the function `setRecipient()`;
- contract `LuckyBox`: `_admin` in the function `setAdmin()`; `_petAddress`, `NEEAddress`, `AQDCAddress`, `proxyRegistryAddress` and `recipient` in the function `constructor()`; `_petAddress`, `USDTAddress` and `AQDCAddress` in the function `setResourceAddress()`; `recipient` in the function `setRecipient()`;
- contract `ERC721Tradable`: `proxyRegistryAddress` in the function `constructor()`;
- contract `ExchangeCore`: `_addr` in the function `setWhitelistedTarget()`; `_addr` in the function `setWhitelistedPaymentToken()`; `newProtocolFeeRecipient` in the function `changeProtocolFeeRecipient()`; `token`, `from` and `to` in the function `transferTokens()`;
- contract `Exchange`: `registryAddress`, and `protocolFeeAddress` in the function `constructor()`
- contract `NEE`: `_admin` in the function `setAdmin()`; `_proxyRegistryAddress` in the function `constructor()`;
- contract `AQDC`: `_admin` in the function `setAdmin()`; `_proxyRegistryAddress` and `_issuer` in the function `constructor()`.

Missing input validation

There are require checks contain no error message that accompanies the condition they evaluate.

Require statements with no error message

In the protocol, many contract object addresses are initialized in the function `constructor()`. Some of the addresses can even be updated by the contract owner.

Uncertain contract addresses

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to differing compiler version numbers. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

Unlocked compiler version

In the contract `game`, the role `owner` has the authority over the following function:

- function `setAdmin()`: set/unset the address `_admin` to be a whitelisted admin,
- function `setResourceAddress()`: change the resource addresses, e.g. `petAddress`, `NEEAddress` and `AQDCAddress`,
- function `setRecipient()`: change the `recipient` address.

Also, the role `_admin` has the authority over the following function:

- function `breed()`: burn out the `owner`'s `NEE` tokens, transfer the `owner`'s `AQDC` tokens to the `recipient` address and call the function `PetsNFT.spawnPet()` to mint a new `PetsNFT` token,
- function `evolution()`: burn out the `owner`'s `NEE` tokens, transfer the `owner`'s `AQDC` tokens to the `recipient` address and call the function `PetsNFT.evolutionPet()` to mint a new `PetsNFT` token.

Any compromise to the privileged accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in game.sol

In the contract `ProxyRegistry`, the role `owner` has the authority over the following function:

- function `grantAuthentication()/revokeAuthentication()`: set/unset the address `addr` to be a whitelisted contract that can call the function `proxy()`.

In the contract `OwnableDelegateProxy`, the role `owner` has the authority over the following function:

- function `setRevoke()`: change the value of `revoked` flag to allow/block calling the function `proxy()` from the previously mentioned whitelisted proxy.

Any compromise to the privileged accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in proxy.sol

According to the official document of the solidity language("https://docs.soliditylang.org/en/v0.8.7/types.html?highlight=delegatecall#members-of-addresses"), both of the  `call()` and `delegatecall()` are low-level functions and should be used with care. Specifically, any unknown contract might be malicious and if you call it, you hand over control to that contract which could in turn call back into your contract, so be prepared for changes to your state variables when the call returns. The regular way to interact with other contracts is to call a function on a contract object (contract.funtion()).

Usage of the `call()/delegatecall()` functions

Although there are two `HowToCall` options: `Call` and `DelegateCall`. The protocol only uses the `Call` option and there is no usage of `DelegateCall`.

Also, if the function `address.delegatecall` is used, the code of the given address is used, but all other aspects (storage, balance, …) are taken from the current contract which is the contract. `OwnableDelegateProxy`.

Redundant `HowToCall.DelegateCall`

In the contract `ornament`, the role `owner` has the authority over the following function:

- function `setAdmin()`: set/unset the address `_admin` to be a whitelisted admin,
- function `setBaseURI()`: change the `baseURI` of the `PetsNFT` tokens.

Also, the role `_admin` has the authority over the following function:

- function `mintNFTByLuckyBox()`: mint the `ornament` with amount limitation(10000) to an arbitrary address `_to`,
- function `mintNFT()`: mint the token `ornament` to an arbitrary address `_to`,
- function `mintNFTs()`: mint the token `ornament` to an arbitrary address `_to`.

Any compromise to the privileged accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in ornament.sol

In the contract `LuckyBox`, the role `owner` has the authority over the following function:

- function `setAdmin()`: set/unset the address `_admin` to be a whitelisted admin,
- function `setResourceAddress()`: change the resource addresses, e.g. `petAddress`, `USDTAddr` and `AQDCAddr`,
- function `setUnitPriceUSDT()`: change the unit price (`UINT_PRICE_USDT`) of the lucky box,
- function `setUnitPriceAQDC()`: change the unit price (`UINT_PRICE_AQDC`) of the lucky box,
- function `setUnitPriceNEE()`: change the unit price (`UINT_PRICE_NEE`) of the lucky box,
- function `setMaxTotalSupply()`: change the value of `maxTotalSupply`,
- function `setRecipient()`: change the address `recipient` which is used to receive the user's USDT/AQDC for buying the lucky box.

Also, the role `_admin` has the authority over the following function:

- function `setAllowSales()`: change the `isAllowedSalesByUSDT` and `isAllowedSalesByAQDC` flags to allow/block the lucky box selling with USDT or AQDC.

Any compromise to the privileged accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in LuckyBox.sol

In the contract `ExchangeCore`, the role `owner` has the authority over the following function:

- function `setWhitelistedTarget()`: change the value of the boolean variable `whitelistedTarget[_addr]` to set/unset the `_addr` as a whitelisted target,
- function `setWhitelistedPaymentToken()`: change the value of the boolean variable `whitelistedTarget[_addr]` to set/unset the `_addr` as a whitelisted payment token,
- function `changeProtocolFeeRecipient()`: change the `protocolFeeRecipient` address which will receive the protocol fee.

Any compromise to the `owner` accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in exchange.sol

The contract `Exchange` provides the order matching and NFT transfer on-chain, but the sell/buy order(listing) and the bid operations are maintained and processed off-chain or by third-party protocols out of the audit scope. The scope of the audit treats 3rd party entities as black boxes and assume their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts, such as increasing fees of 3rd parties, etc.

Third party dependencies

The function `atomicMatch_()` in the contract `Exchange` does not have any restriction so that any one can call it to match and process a specific sell/buy order, even with a cancelled or finished order. This may result that the order `taker` will get the privilege to set all the order properties, especially for the `feeRecipient`, `RelayerFee` and `ProtocolFee` which can directly affect the sell price and fee amount.

Restriction on `atomicMatch_()`

In the contract `INO`, the role `owner` has the authority over the following function:

- function `setAdmin()`: set/unset the address `_admin` to be a whitelisted admin,
- function `setAQDCAddress()`: change the address `AQDCAddr`,
- function `setUnitPriceUSDT()`: change the unit price (`UINT_PRICE_USDT`) of the lucky box,
- function `setMaxTotalSupply()`: change the value of `maxTotalSupply`,
- function `startUnlockCountDown()`: change the value of `unlockedTime`,
- function `setRecipient()`: change the address `recipient` which is used to receive the user's USDT for buying the lucky box,
- function `recovery()`: transfer the extra `AQDC` tokens to an arbitrary non-zero address `_addr`.

Also, the role `_admin` has the authority over the following function:

- function `setAllowSales()`: change the `isAllowedSales` flag to allow/block the lucky box selling.

Any compromise to the privileged accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in INO.sol

The variable `_threshold` and the length of the passing-in parameter `_owners` does not have reasonable boundaries. Especially, when there is only one owner in the `_owners` array, the value `_threshold` can only be 1 and this multi-sign contract will act as a single externally owned account(EOA).

Lack of reasonable boundary

The function that affects the status of sensitive variables should be able to emit events as notifications to customers.

For example:
- `multiSign.transferOwner()`,
- `multiSign.changeThreshold()`,
- `multiSign.cancelMessageSign()`.

Missing emit events

The change of `owner` by function `transferOwner()` overrides the previously set `owner` with the new one without guaranteeing the new `owner` is able to actuate transactions on-chain.

Pull-over-push pattern

In the contract `NEE`, the role `owner` has the authority over the following function:

- function `setAdmin()`: set/unset the address `_admin` to be a whitelisted admin,
- function `setSignAdmin()`: change the `signAdmin` addresses.

Also, the role `_admin` has the authority over the following function:

- function `cancelOrder()`: change the `isAllowedSalesByUSDT` and `isAllowedSalesByAQDC` flags to allow/block the lucky box selling with USDT or AQDC,
- function `mint()`: mint any arbitrary amount of the `NEE` tokens to any arbitrary accounts.

Also, the role `signAdmin` with  has the authority over the following function:

- function `mint()`: mint an arbitrary amount of the `NEE` tokens to any arbitrary `account` with the correct signature, only can be called once.


In the contract `AQDC`, the role contract deployer has the authority over the following function:

- function `constructor()`: mint all the `AQDC` tokens to any arbitrary accounts `_issuer`.

Any compromise to the privileged accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in NEEAQDC.sol

In the contract `PetsNFT`, the role `owner` has the authority over the following function:

- function `setBaseURI()`: change the `baseURI` of the `PetsNFT` tokens,
- function `setAdmin()`: set/unset the address `_admin` to be a whitelisted admin.

Also, the role `_admin` has the authority over the following function:

- function `mintLuckyBox()`: mint lucky box `PetsNFT` tokens to the any arbitrary address `_owner`,
- function `batchMintLuckyBox()`: mint any amount(less than 100) of lucky box `PetsNFT` tokens to the any arbitrary address `_owner`,
- function `spawnPet()`: mint a `PetsNFT` token to the address `_owner` as long as it owns two `PetsNFT` tokens,
- function `evolutionPet()`: mint a `PetsNFT` token to the address `_owner` and burn out two `PetsNFT` tokens owned by the address `_owner`,
- function `retirePet()`: burn out a specific `PetsNFT` token.

Any compromise to the privileged accounts may allow the hacker to take advantage of this authority and users' assets may suffer loss.

Centralization risk in PetsNFT.sol

Literally speaking, the constant variable `MAX_SUPPLY_TOKEN_ID` is the max total supply of `PetsNFT` token IDs. In another word, the maximum `PetsNFT` token ID is `MAX_SUPPLY_TOKEN_ID`. However, the linked codes will mint token ID that larger than `MAX_SUPPLY_TOKEN_ID`. So could you please explain the design details about the variable `MAX_SUPPLY_TOKEN_ID`?

```solidity
  require(_petsId[2] > MAX_SUPPLY_TOKEN_ID);
  ...
  _mint(_owner, _petsId[2]);
```

Ambigious usage of `MAX_SUPPLY_TOKEN_ID`

The function `getMaxSupplyLuckyBox()` calculates the maximum supply of lucky box by the calculation statement `maxLuckyBoxId - minLuckyBoxId + 1`. However, since both the variables `maxLuckyBoxId` and `minLuckyBoxId` are constant variables, the calculation result is always 100000 which is actually the value of `maxLuckyBoxId`.

AQUANEE

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

21

0

12

0

9