Virtualness

QuillAudits24 Jul 2023Files named in the report

QuillAudits reviewed Virtualness on 24 Jul 2023: 41 findings, 0 critical; 19 marked fixed, 22 open.

No open critical findings

Open the report Open Virtualness’s security record

Report hosted on quillaudits.com.

Statuses are the security firm’s own, from this report. Trustblock does not review code.

Findings41

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

Severity	Open	Fixed
Critical	0	0
High	1	0
Medium	2	3
Low	19	16

MediumroyaltyInfo function call can fail for the token contracts which don’t support/implement ERC2981Fixed

transferFunds() calls royaltyInfo(tokenId, price) on collection contract to take receiver and royaltyAmount. In case any collection doesn’t implement ERC2981 standard the royaltyInfo() function would be absent in that contract and the call will fail.

MediumShould use SafeERC for critical operations in contractFixed

TransferExecutor:transferFunds() function uses _transferTo() to transfer tokens “from” to “to” address. Here _transferTo() calls transferERC20() from the ExecutionDelegate contract.But in transferERC20() it can happen that the transferFrom() will fail to transfer tokens (e.g in case of not having enough tokens in account). In this case if the token contract is a non reverting token (which doesn’t revert on failure and returns bool value) the boolean status value is not getting checked after that call. So it can happen that the malicious buyer won’t have anything in the account to transfer and would be able to buy tokens for free (i.e without spending tokens to buy NFT)

MediumUnable to transfer royalty fee while lazymintingFixed

While lazy minting tokens with fulfillLazyMintOrder() , in matchAndLazyMint() function transferFunds() and tokenLazyMint() gets executed. But here while executing transferFunds() the “(address receiver, uint256 royaltyAmount) = IERC2981Upgradeable(collection).royaltyInfo(tokenId, price);” will return receiver as zero address and it won’t send royaltyAmount to the receiver. The reason the receiver would be zero address is because the royalty info for the token is getting set after the transferFunds() call i.e in tokenLazyMint().

Low_saveSupply() function should have error messageFixed

In function _saveSupply() there is a require statement on L247 stating supply of token should be zero but it does not have a meaningful error message if the function is called on the same token id again.

LowCall _disableInitializers() in constructorFixed

Add a constructor and call _disableInitializers() in constructor of this contract,calling this in the constructor of a contract will prevent that contract from being initialized or reinitialized to any version. It is recommended to use this to lock implementation contracts that are designed to be called through proxies.

LowCall _disableInitializers() in constructorFixed

Add a constructor and call _disableInitializers() in the constructor of this contract,calling this in the constructor of a contract will prevent that contract from being initialized or reinitialized to any version. It is recommended to use this to lock implementation contracts that are designed to be called through proxies.

LowCall _disableInitializers() in constructorFixed

LowcancelOrder function should have error message Description:Fixed

In function cancelOrder() the require statement on L274 checks if message.sender is the trader who is going to cancel the order but there is no error message set if the msg.sender is not the trader so it will revert wihtout the error message.

LowcancelOrder() doesn’t revert for already canceled orderFixed

Lets say, Seller/trader wants to cancel the order he checks on the website/interface that the order isn’t executed yet and proceeds to cancel it. at that moment someone buys the order before it gets canceled i.e someone bought it before cancelOrder transaction executes. So in this case the cancelOrder() won’t execute the code in the if block because cancelledOrFilled[hash] was set to true when the buyer bought it. So it will proceed execution without setting it to true again but also without reverting the transaction. In this case it can happen that the seller will get false assurance that his transaction got canceled even though it was executed. Adding require statement to revert transaction in this case will help for handling this situation as adding some check from the backend will can’t handle this situation e.g dapp backend checks that the cancelledOrFilled[hash] is false means the order is not filled yet but as mentioned in above example it can happen that the order will get filled once user executes cancelOrder() transaction.

Lowredundant abicoder declarationFixed

pragma abicoder v2; getting declared , but ABI coder v2 is activated by default from 0.8.0 version. As can be seen here https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html

Show 9 more

Lowredundant abicoder declarationFixed

pragma abicoder v2; getting declared , but ABI coder v2 is activated by default from 0.8.0 version. As can be seen here https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html

LowRedundant importFixed

Initializable is getting imported but UUPSUpgradeable already imports Initializable which makes it redundant.

LowUnlocked pragmaFixed

Contracts are using floating pragma (pragma solidity ^0.8.17), Contracts should be deployed with the same compiler version and flags that they have been tested thoroughly. Using floating pragma does not ensure that the contracts will be deployed with the same version. It is possible that the most recent compiler version gets selected while deploying a contract which has higher chances of having bugs in it.

LowUnused and commented code in the contractFixed

In the lazyMint() function there is some code on L187 and L197 which is commented out and is not used.

LowUnused and commented code in the contractFixed

In the lazyMint() function there is some code on L209 which is commented out and is not used.

LowUnused event in contractFixed

Event Creators(uint256 indexed tokenId, LibPart.Part[] indexed creators) is not used in contract.

LowUnused importsFixed

ERC721Upgradeable and AccessControlUpgradeable are not getting inherited by ERC721Virtualness. Unused/uninherited imports can be removed.

LowUse call{} instead of send for eth transferFixed

In function _transferTo() send() is used to transfer eth which is not the recommended way. The recommended way to transfer eth is to use call{}. The reason being send and transfer both uses 2300 gas for transferring but because there can be changes in gas in future the gas limit for both those options might change which will result in failure of the eth transfer.

LowvalidateOrderTime() allows expirationTime to be set to 0 Description:Fixed

In the contract VirtualnessV1Core the function validateOrderTime() allows expiration time to be set to 0. It can happen that the order signature remains unexecuted for long time and someone will able to execute that order later where seller may don’t want to execute that order for that price specified before.