Reality NFT

CertiK2 Dec 2022Files named in the report

CertiK reviewed Reality Metaverse (Reality NFT) on 2 Dec 2022: 22 findings, 0 critical; 16 marked fixed, 2 acknowledged, 4 open.

No open critical findings

Open the report Open Reality Metaverse’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings22

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

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	2	0
Medium	0	0	4
Low	4	0	12

Mediumconstant `EXCHANGERATE_PRECISION` value is invaliFixed

Constant `EXCHANGERATE_PRECISION` is set to be `10^18`, which is value `24` in type `uint256`. Based upon context, it appears this value should be `10**18`.

MediumImplementation Contract Is Not Initialized AutomaFixed

When an implementation contract is deployed and the function `initialize()` is called from the proxy contract, it will execute in the context of the proxy contract storage. Hence the state variable `_initialized` and `_initializing` will be 0 and false, respectively. Attackers can directly call the function `initialize(),` bypass the `initializer` modifier, and feed in malicious inputs which can, in turn, affect the proxy.

MediumNo Zero Address Validation Check for `sellerAddreFixed

There is no check that `sellerAddress` and `buyerAddress` are not `address(0)`. The respective functions in which each address is used call `_verifyOffer()` and `_verifyBid()` from `TradeSigVerifier`. Each of these functions checks that the returned address from the `recover()` function in OpenZeppelin's ECDSA contract is equal to the specified address. However, OpenZeppelin's ECDSA contract returns `address(0)` when recovery fails.

MediumPotential Reentrancy Attack (Not Involving Ether)Fixed

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. ### External call(s) ```solidity=39 _mint(msg.sender, tokenId, amount, ""); ``` * This function call executes the following external call(s). * In `RealityProperties.__doSafeTransferAcceptanceCheck`, * `IERC1155ReceiverUpgradeable(to).onERC1155Received(operator,from,id,amount,data)`

LowClarification on How `nonce` is updatedFixed

In the contract `Trade`, users provide an `offer` input which is verified through functions in the inherited contract `TradeSigVerifier`. A nonce value is included in the `offer` input and is an important aspect in preventing replay attacks. Namely, it is important that the nonce is not repeated.

LowDivide Before MultiplyFixed

Performing integer division before multiplication truncates the low bits, losing the precision of calculation. ```solidity return vestedToAllocation[_to] / stepsCount * stepNumber; ```

LowFunction `supportsInterface()` Does not return trFixed

The standard EIP-165 outlines that the `supportsInterface()` function should return true for any interfaceID this contract implements. This contract inherits `AccessControl` through the contract `ERC1155HandlerStorage` and as such, should return true when `interfaceId` is equivalent to `type(IAccessControl).interfaceId`.

LowImproper Usage of `public` and `external` TypeFixed

`public` functions that are never called by the contract could be declared as `external`.

LowMissing `onlyInitializing()` modifier on functionFixed

Functions that should only be called during initialization should be protected with modifier `onlyInitializing()` so that they may only be invoked by functions with the `initializer()` modifier.

LowMissing Emit EventsFixed

Functions that affect the status of sensitive variables should emit notifications of updates to storage.

Show 6 more

LowMissing Zero Address ValidationFixed

Addresses should be checked before assignment or external calls to ensure they are not zero addresses.

LowNo check that `tokenId` is not 0Fixed

The function `fractionalTransferByAdapter()` can be called by any user since it is external. When called, `tokenId` is checked for the `msg.sender` address. For a regular user, the `tokenId` will be 0. Regardless, `_transferFrom()` will still be executed. Such a vulnerability could cause a logical inconsistency. Similarly, functions `_safeTransferFrom()`, `_safeBatchTransferFrom()`, `_mint()`, `_mintBatch()`, `_burn()`, and `_burnBatch()` have no check that `id` is not zero.

LowNo state variable used as a `__gap` in upgradeablFixed

The cited contracts are upgradeable, but do not include a `__gap` state variable. Such a state variable is used to hold space in storage so new state variables can be added in future implementations without causing storage incompatibility.

LowNo validation check that third party `tokenAddresFixed

Functions `transferFrom()` and `batchTransferFrom()` do not perform checks that the specified input address `tokenAddress` is a supported token. As such, these functions can potentially be used on any token address, including ones with unvetted malicious contract logic.

LowUnchecked ERC-20 `transfer()`/`transferFrom()` CaFixed

The return value of the transfer()/transferFrom() call is not checked in the following lines. Since some ERC-20 tokens return no values and others return a `bool` value, they should be handled with care. ```solidity=219 mainCurrency.transferFrom(sender, address(this), amount); ``` --- ```solidity=268 stableCurrency.transferFrom(sender, address(this), stableAmount); ``` --- ```solidity=368 mainCurrency.transfer(to, mainCurrency.balanceOf(address(this)) - _activeBiddingMainTotal); ``` --- ```solidity=374 stableCurrency.transfer(to, stableCurrency.balanceOf(address(this)) - _activeBiddingStableTotal); ``` --- ```solidity=429 mainCurrency.transfer(to, amount); ``` --- ```solidity=431 stableCurrency.transfer(to, amount); ``` --- ```solidity=124 erc20Contract.transfer(to, tokensExcess); ``` --- ```solidity=146 erc20Contract.transfer(sender, toWithdraw); ``` --- ```solidity=87 token.transfer(voucher.receiver, withdrawalAmount); ``` --- ```solidity=105 token.transfer(msg.sender, remainingBalance); ```

LowUse of non-upgradeable `AccessControl` in an upgrFixed

The abstract contract `ERC1155HandlerStorage` inherits from OpenZeppelin's `AccessControl` contract, rather than `AccessControlUpgradeable`. Since `ERC1155HandlerStorage` is itself intended to be upgradeable, all inherited contracts and libraries used should also be set up for upgradeability.