Read the findings of the 2022 Web3 Security Report

When the users sell or buy NFTs, the function `sellNFTsForETH` and `buyNFTsForETH` call the function `_verify` to verify whether the signature is signed by the `operator`. It seems the NFT liquidity is under the control of the `operator`.

```solidity
function _verify(        
        bytes32 _typeHash,
        ERC721Details[] memory _erc721Details,
        ERC1155Details[] memory _erc1155Details,  
        uint256 _totalPrice,
        uint256 _deadline,
        bytes memory signature
    ) internal view returns (bool) {
        bytes32 messageHash = _getMessageHash(_typeHash, _erc721Details, _erc1155Details, _totalPrice, _deadline);
        bytes32 ethSignedMessageHash = _getSignedMessageHash(messageHash);

        return _recoverSigner(ethSignedMessageHash, signature) == operator;
    }
```
Any compromise to the `operator` account may allow the hacker to take advantage of this authority and disrupt the NFT trade.

Centralization Related Risks on `operator`

Escrow Account `nftOwner` has a high Centralization Risk. When users sell NFTs, the NFTs will be transferred to the `nftOwner` address. When users buy NFTs, the NFTs will be transferred from the `nftOwner` address. The `nftOwner` address plays a vital role in the liquidity of NFTs. Any compromise to the `nftOwner` account may have unpredictable impacts.

For example:
```solidity
  function sellNFTsForETH( 
            ERC721Details[] calldata _erc721Details, 
            ERC1155Details[] calldata _erc1155Details,             
            uint256 _totalPrice,          
            uint256 _deadline,
            bytes calldata _signature
    ) external nonReentrant whenNotPaused{
      ...
      IERC721(_erc721Details[i].tokenAddr).safeTransferFrom(
            _msgSender(),
            nftOwner,                              
            _tokenId
      );
      ...
      IERC1155(_erc1155Details[i].tokenAddr).safeTransferFrom(
          _msgSender(),
          nftOwner,
          _tokenId,
          _amounts,
          ""
      );
      ...
    }
```

Centralization Risk - Escrow Account

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

Missing Input Validation

After [EIP-1884](https://eips.ethereum.org/EIPS/eip-1884) was included in the Istanbul hard fork, it is not recommended to use `.transfer()` or `.send()` for transferring ether as these functions have a hard-coded value for gas costs making them obsolete as they are forwarding a fixed amount of gas, specifically `2300`. This can cause issues in case the linked statements are meant to be able to transfer funds to other contracts instead of EOAs.

Usage of `transfer()` for sending Ether

The signature is generated without the nonce. When a user gets the signature and the corresponding parameters, he can call the function of buy or sell more than one time. It means a user can buy the same NFT from the contract, or sell the same NFT to the contract for the same price as long as the signature is not expired.


Potential Replay Attack

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Missing Emit Events

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

Improper Usage of `public` and `external` Type

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

Missing Error Messages

This finding is based on the commit [a792310390e573df7723edeaabf6f1b81c69bb98](https://github.com/vcx-devs/VirgoNFT-contract/commit/a792310390e573df7723edeaabf6f1b81c69bb98)

In the contract `Ownable` the role `_owner` has authority over the functions shown in the diagram below.
Any compromise to the `_owner` account may allow the hacker to take advantage of this authority.
![](https://accelerator-tasks-prod.s3.amazonaws.com/ec88bad480d34c77a46d748f405252c4/diagrams/centralization_Ownable__owner.svg)

In the contract `VirgoInstantLiquidity` the role `managers` has authority over the functions shown in the diagram below.
Any compromise to the `managers` account may allow the hacker to take advantage of this authority, change the configuration of the contract, and create/sign/close withdrawal transactions.
![](https://accelerator-tasks-prod.s3.amazonaws.com/ec88bad480d34c77a46d748f405252c4/diagrams/centralization_VirgoInstantLiquidity_managers.svg)


The following finding content is based on the commit [e3e53b3b23a806370f9ce95768df987d132df4e4](https://github.com/vcx-devs/VirgoNFT-contract/commit/e3e53b3b23a806370f9ce95768df987d132df4e4)

In the contract `VirgoInstantLiquidity` the role `managers` has authority over the functions listed below:
- `cancelUpdateNFTOwner`: call the function `_clearNFTOwnerRecord`
- `cancelUpdateOperator`: call the function `_clearOperatorRecord`

Any compromise to the `managers` account may allow the hacker to take advantage of this authority.

Centralization Risks in VirgoInstantLiquidity_V1.sol

This finding is based on the commit [a792310390e573df7723edeaabf6f1b81c69bb98](https://github.com/vcx-devs/VirgoNFT-contract/commit/a792310390e573df7723edeaabf6f1b81c69bb98)

In the public function `closeTransactions`, if the passed-in value `_ethRecordId` is out of range, the **require** statement will still pass:
```solidity
require(!ethRecords[_ethRecordId].isEnd, "This transaction is closed");
```

At the end of the "for" loop execution, the variable `temp` is 0, which means that the `_ethRecordId` does not exist in the array `pendingEthRecords`. However, the code to close the transaction is still executed because the validity of the `_ethRecordId` is not checked, and the last element of the `pendingEthRecords` will be deleted. This will cause the data for the withdrawal transactions to be in an error state.

```solidity=1673
        delete pendingEthRecords[pendingEthRecords.length - 1];
        pendingEthRecords.pop();
        ethRecords[_ethRecordId].isEnd = true;   
        emit CloseTransactions(_ethRecordId);
```


Missing Check The `temp` Value

This finding is based on the commit [a792310390e573df7723edeaabf6f1b81c69bb98](https://github.com/vcx-devs/VirgoNFT-contract/commit/a792310390e573df7723edeaabf6f1b81c69bb98)

When the client needs to update an operator or an NFT owner, one of the managers could create a proposal to transfer the operator or NFT owner to the new one, after 2/3 managers approved this proposal, the proposal will be executed.

However, there is no way to cancel a pending proposal, the rest of the managers can only approve the new proposal or do nothing to keep the proposal always as a pending one.

The Managers are Unable to Cancel a Proposal

When deploying the contract, an array of addresses will be set as privileged managers, however, there is no validation to prevent repeated addresses are passed-in. As a result, an undesired side effect maybe happened. 

For example, if all the addresses are the same and passed to the parameter `_managers`, and the actual number of managers becomes one, which is less than the `signedCount` that is 2, finally, the approvals of the managers would always less than`signedCount`, in the below functions:
- `updateOperator()` 
- `updateNFTOwner()`
- `signEthRecord()`

Virgo NFT

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

12

0

3

1

8