Read the findings of the 2022 Web3 Security Report

In contract `Ownable`, the `owner` role has the authority over the following function:
- `Ownable.transferOwnership(address _newOwner)`: Transfer the ownership to the address `_newOwner`. 

In contract `EmblemVault`, the `owner` role has the authority over the following function:
- `EmblemVault.changeName(string calldata, string calldata)`: Change the name and symbol of the NFT.
- `EmblemVault.mint( address, uint256, string calldata, string calldata)`: Mint a new NFT.
- `EmblemVault.updateContractURI(string memory)`: Update the contract URI.
- `EmblemVault.updateTokenUri(uint256, string memory)`: Update the URI of a valid NFT.

Any compromise to the `owner` account may allow the hacker to take advantage of this and manipulate the project.

Centralization Risk

In the function `NFTokenEnumerableMetadata._burn()`, the relevant state variables, `ownerToIds` that records an owner's list of the owned NFT Ids, and `idToOwnerIndex` that maps an NFT Id to its index in the owner's token list, have not been updated after the NFT burned. 

State Variables Not Updated in `_burn()`

The implementation of the `isContract` check can not cover all scenarios. The check can be bypassed if the call is from the constructor of a smart contract or when the contract is destroyed. Because, in that case, the codesize will also be zero.

Ineffective `isContract()` Check

According to the [Solidity style guide](https://docs.soliditylang.org/en/v0.6.0/style-guide.html#order-of-layout), the variable declarations and event declarations should be followed by function declarations. In the contract `NFTokenEnumerableMetadata`, the variable declarations from line 1260 - 1280 follow the function declarations. In the contract `EmblemVault`, the event declaration follows the function declarations. 

Order of Layout

In the function `EmblemVault._setTokenPayload()`, the variable name `_uri` is used to represent the URI, and it does not reflect the payload it needs to pass into the function. 

Misleading Variable Name

The variable name "NOT_OWNER_APPROWED_OR_OPERATOR" should be "NOT_OWNER_APPROVED_OR_OPERATOR" judging from its context and functionality.

Typo in Variable Name

In the comment line 768 - 769 of the function `NFToken.safeTransferFrom()`, it says `Throws unless `msg.sender` is the current owner, an authorized operator, or the approved address for this NFT.` While the functions, `NFToken.safeTransferFrom()`, in lines 779 and 800 call the following implementation that only allows the NFT owner to transfer, excluding the authorized operator and approved address. 
```solidity=1077
  function _safeTransferFrom(
    address _from,
    address _to,
    uint256 _tokenId,
    bytes memory _data
  )
    private
    canTransfer(_tokenId)
    validNFToken(_tokenId)
  {
    address tokenOwner = idToOwner[_tokenId];
    require(tokenOwner == _from, NOT_OWNER);
    require(_to != address(0), ZERO_ADDRESS);

    _transfer(_to, _tokenId);

    ...
  }
```

Similarly, in the comment line 812 - 813 of the function `NFToken.transferFrom()`, it says `Throws unless `msg.sender` is the current owner, an authorized operator, or the approved address for this NFT.` The implementation of the function only allows the NFT owner to transfer, excluding the authorized operator and approved address. 
```solidity=821
  function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
  )
    external
    override
    canTransfer(_tokenId)
    validNFToken(_tokenId)
  {
    address tokenOwner = idToOwner[_tokenId];
    require(tokenOwner == _from, NOT_OWNER);
    require(_to != address(0), ZERO_ADDRESS);

    _transfer(_to, _tokenId);
  }
```

Inconsistency Between Comment and Code

The function `NFToken.setApprovalForAll()` enables or disables approval for an `operator` to manage all of
caller, `msg.sender`'s assets. However, it does not add a check to make sure that the `operator` should not be the `msg.sender`. 

`setApprovalForAll()` Allows `msg.sender` to Be Operator

The function `EmblemVault._burn()` that burns an NFT token calls the `NFTokenEnumerableMetadata._burn()` with the implementation:
```solidity=1360
  function _burn(
    uint256 _tokenId
  )
    internal
    override
    virtual
  {
    super._burn(_tokenId);
    ...
```
which in turn calls the `NFToken._burn()` with the implementation:
```solidity=1003
  function _burn(
    uint256 _tokenId
  )
    internal
    virtual
    validNFToken(_tokenId)
  {
    address tokenOwner = idToOwner[_tokenId];
    _clearApproval(_tokenId);
    _removeNFToken(tokenOwner, _tokenId);
    emit Transfer(tokenOwner, address(0), _tokenId);
  }
```
In line 1012, the function `NFToken._removeNFToken(tokenOwner, _tokenId)`:
```solidity=1022
  function _removeNFToken(
    address _from,
    uint256 _tokenId
  )
    internal
    virtual
  {
    require(idToOwner[_tokenId] == _from, NOT_OWNER);
    ownerToNFTokenCount[_from] = ownerToNFTokenCount[_from] - 1;
    delete idToOwner[_tokenId];
  }
```
updates a private state variable, `ownerToNFTokenCount`, that is declared in the contract `NFToken`. Notice that `ownerToNFTokenCount[_from]` will cause an overflow as the initial value is 0.

It's advisable to implement the `NFTokenEnumerableMetadata._burn()` without utilizing the `NFToken._burn()` and use the `NFTokenEnumerableMetadata._removeNFToken()` instead.

`_burn()` Modifies Private State Variable 

In the constructor of contract `VaultHandlerV8`, the `_price` is multiplied with 10 first, then the result is raised to the power of `decimals`. The base 10 is supposed to be raised to the power of `decimals` first, then multiplied with the `_price`. 
```solidity=94
    constructor(address _nftAddress, address _paymentAddress, address _recipientAddress, uint256 _price) {
        addWitness(owner);
        metadataBaseUri = "https://api.emblemvault.io/s:evmetadata/meta/";
        nftAddress = _nftAddress;
        paymentAddress = _paymentAddress;
        recipientAddress = _recipientAddress;
        initialized = true;
        uint decimals = BasicERC20(paymentAddress).decimals();
        price = _price.mul(10) ** decimals;
        vaultContractCount = 0;
    }
```

Incorrect `price` Conversion

In the contract `VaultHandlerV8`, the role `owner` has authority over the following functions:
- `VaultHandlerV8.addVaultContract(address vaultContract, uint _type, bool curated)` to add a vault contract;
- `VaultHandlerV8.removeVaultContract(address vaultContract)` to remove a vault contract;
- `VaultHandlerV8.toggleShouldBurn()` to toggle the status of `shouldBurn`;
- `VaultHandlerV8.addWitness(address _witness)` to add witness;
- `VaultHandlerV8.removeWitness(address _witness)` to remove witness;
- `VaultHandlerV8.changeMetadataBaseUri(string calldata _uri)` to change the metadataBaseUri;
- `VaultHandlerV8.transferPaymentOwnership(address newOwner)` to transfer the payment ownership;
- `VaultHandlerV8.transferNftOwnership(address _nftAddress, address newOwner)` to transfer the nft ownership;
- `VaultHandlerV8.mint(address _nftAddress, address _to, uint256 _tokenId, string calldata _uri, string calldata _payload)` to mint an NFT at address `_nftAddress`;
- `VaultHandlerV8.changeName(address _nftAddress, string calldata name, string calldata symbol)` to change the name and symbol of an NFT;
- `VaultHandlerV8.updateTokenUri(address _nftAddress, uint256 _tokenId,string memory _uri)` to update an NFT uri;
- `VaultHandlerV8.changePayment(address payment)` to change the payment address;
- `VaultHandlerV8.changeRecipient(address _recipient)` to change the recipient address;
- `VaultHandlerV8.changeNft(address token)` to change the NFT;
- `VaultHandlerV8.changePrice(uint256 _price)` to change the price of NFT.

Any compromise to the `owner` account may allow a hacker to take advantage of this authority and manipulate the project.

Centralization Related Risks in Contract `VaultHandlerV8`

Based on the modifier `isRegisteredVaultContract()`:
```solidity=81
    modifier isRegisteredVaultContract(address _vaultContract) {
        require(vaultContracts[_vaultContract]._type > 0, "Vault contract is not registered");
        _;
    }
```
the registered Vault contract should have `_type > 0`. 

Lack of Input Validation of `_type`

The assigned values to the address variables should be verified as a non-zero value to prevent being mistakenly assigned as `address(0)` in the aforementioned functions. 

Lack of Zero Address Validation

The `require` statement can be used to check for conditions and throw an exception if the condition is not met, in which case the descriptive error message provided by the developer will appear and help to tracking error and debugging.

Lack of Error Message

The `require` statement is redundant, because the function `addVaultContract()` already had the access control `onlyOwner`.

Redundant Code

The functions that affect the status of sensitive variables should emit events for better tracking contract status. For example,
- `VaultHandlerV8.buyWithSignature()`
- `VaultHandlerV8.buyWithSignedPrice()`
- `VaultHandlerV8.addVaultContract()`
- `VaultHandlerV8.removeVaultContract()`
- `VaultHandlerV8.moveVault()`
- `VaultHandlerV8.toggleShouldBurn()`
- `VaultHandlerV8.addWitness()`
- `VaultHandlerV8.removeWitness()`
- `VaultHandlerV8.transferToStaking()`
- `VaultHandlerV8.transferWithCode()`
- `VaultHandlerV8.addPreTransfer()`
- `VaultHandlerV8.deletePreTransfer()`
- `VaultHandlerV8.changeMetadataBaseUri()`
- `VaultHandlerV8.changeName()`
- `VaultHandlerV8.updateTokenUri()`
- `VaultHandlerV8.changePayment()`
- `VaultHandlerV8.changeRecipient()`
- `VaultHandlerV8.changeNft()`
- `VaultHandlerV8.changePrice()`

Lack of Event Emitting in Contract `VaultHandlerV8`

In the function `VaultHandlerV7.transferWithCode()`, an NFT token would be transferred to the `_to` address after checking the provided signature:
```solidity=577
        require(witnesses[getAddressFromSignature(_tokenId, _nonce, signature)], 'Not Witnessed');
```
However, only the `_tokenId` and `_nonce` are signed. The `_to` address is not included in the signature. It allows attackers to transfer the NFT token to an arbitrary recipient as long as they obtain the signature.

When a `transferWithCode` transaction is submitted, in most cases it is a pending transaction at the beginning and will not be included in a block immediately. Attackers spotting this pending transaction would be able to get the signature by decoding the input. Then they can front-run another `transferWithCode` transaction with the signature and another address before the original transaction is executed. In this way, the attackers can steal the NFT tokens to be transferred.

Ineffective Signature Design

In contract `VaultHandlerV7`, the `owner` role has the authority over the following function:
- `VaultHandlerV7.transferOwnership(address payable newOwner)`: Transfer the ownership to the address `newOwner`.
- `VaultHandlerV7.claim(uint256 tokenId)`: Burn an NFT of `tokenId`.
- `VaultHandlerV7.addPreMint(address, string calldata, uint256, bytes32)`: Add pre-mint NFT.
- `VaultHandlerV7.deletePreMint(address, uint256)`: Delete pre-mint NFT. 
- `VaultHandlerV7.addWitness(address)`: Add an address as witness.
- `VaultHandlerV7.removeWitness(address)`: Remove an address from witness.
- `VaultHandlerV7.changeMetadataBaseUri(string calldata)`: Change the base URI. 
- `VaultHandlerV7.transferPaymentOwnership(address)`: Change the ownership of payment.
- `VaultHandlerV7.transferNftOwnership(address)`: Transfer the ownership of the NFTs.
- `VaultHandlerV7.mint(address, uint256, string calldata, string calldata)`: Mint a new NFT.
- `VaultHandlerV7.changeName(string calldata, string calldata)`: Change the name and symbol of the NFT.
- `VaultHandlerV7.updateTokenUri(uint256, string memory)`: Update the token URI. 
- `VaultHandlerV7.changePayment(address)`: Change the address of the payment.
- `VaultHandlerV7.changeRecipient(address)`: Change the recipient address.
- `VaultHandlerV7.changeNft(address)`: Change the NFT address.
- `VaultHandlerV7.changePrice(uint256)`: Change the price.
- `VaultHandlerV7.addChainId(uint)`: Add a chain id. 
- `VaultHandlerV7.removeChainId(uint)`: Remove a chain id. 
- `VaultHandlerV7.transferFromChain(address _to, uint chainId, uint256 amount)`: Transfer tokens from chain. 

Any compromise to the `owner` account may allow the hacker to take advantage of this and manipulate the project.

When users add a pre-transfer record by calling the function `VaultHandlerV7.addPreTransfer()`, the owner of the NFT token is not checked. It might cause the issue that a non-owner user adds a pre-transfer record for an NFT token, and the owner of the NFT token is not allowed to adds the "real" pre-transfer record because it already exists.

Lack of Check for Token Owner

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. For example,
- `Bridged.transferToChain(uint chainId, uint256 amount)`: Contract on `paymentAddress` triggers `allowance()`, `transferFrom()` and `mint()` whose implementations are unknown. 
- `Bridged._transferFromChain(address _to, uint chainId, uint256 amount)`: Contract on `paymentAddress` triggers `mint()` whose implementation is unknown.

These contracts may trigger customized external calls and thus allow reentrancy attacks.

Please note that, because the `Bridged._transferFromChain()` is `internal`, it would not be at risk of reentrancy attacks if the public functions calling them are already protected.

Potential Reentrancy Attack in Contract `Bridged`

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. For example,
- `VaultHandlerV7.buyWithPaymentOnly()`
- `VaultHandlerV7.buyWithSignature()`
- `VaultHandlerV7.addPreMint()`
- `VaultHandlerV7.transferWithCode()`
- `VaultHandlerV7.transferFromChain()`

Although the contract `VaultHandlerV7` inherits the contract `ReentrancyGuard`, it does use the modifier `nonReentrant` to protect from reentrance. These functions trigger some methods from the contracts built on `paymentAddress` or `nftAddress`, which may in turn trigger customized external calls and thus allow reentrancy attacks.

Potential Reentrancy Attack in Contract `VaultHandlerV7`

The library `SafeMath` implemented from line 119 - 260 only works for `uint256`. If it is used for `uint8`, then it will be auto-casted as `uint256`. So it might not revert if the overflow of `uint8` occurs. 

SafeMath Only Works Properly for `uint256`

In the function `VaultHandlerV7.changePrice()`, the calculation of `price` has the risk of integer overflow.
```solidity=676
    function changePrice(uint256 _price) public isOwner {
        uint decimals = BasicERC20(paymentAddress).decimals();
        price = _price * 10 ** decimals;
    }
```

Potential Integer Overflow

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 contracts, `Bridged` and `VaultHandlerV7`, it uses `uint` to declare 256-bit unsigned integers, while it also uses `uint256` elsewhere. Although, `uint` is an alias for `uint256` and both represent the same underlying integer allocation. It is advisable that the complete form `uint256` should be used instead of the alias `uint` for clean coding practices. 

 Usage of `uint` Alias Instead of `uint256`

The functions that affect the status of sensitive variables should emit events for better tracking contract status. For example,
- `Bridged._addChainId()`
- `Bridged._removeChainId()`

Lack of Event Emitting in Contract `Bridged`

The function that is declared `payable` can receive ether. The functions `VaultHandlerV7.buyWithPaymentOnly()`, `VaultHandlerV7.buyWithSignature()` and `VaultHandlerV7transferWithCode()` are not designed for receiving ether, so it is redundant to have the `payable` modifier.

Redundant Modifier `payable`

Literals with many digits are difficult to read and review.

Too Many Digits

The linked state variables are never used in the codebase.

Unused State Variable

The functions that affect the status of sensitive variables should emit events for better tracking contract status. For example,
- `VaultHandlerV7.addPreMint()`
- `VaultHandlerV7.deletePreMint()`
- `VaultHandlerV7.toggleShouldBurn()`
- `VaultHandlerV7.addWitness()`
- `VaultHandlerV7.removeWitness()`
- `VaultHandlerV7.transferWithCode()`
- `VaultHandlerV7.addPreTransfer()`
- `VaultHandlerV7.deletePreTransfer()`
- `VaultHandlerV7.changeMetadataBaseUri()`
- `VaultHandlerV7.changePayment()`
- `VaultHandlerV7.changeRecipient()`
- `VaultHandlerV7.changeNft()`
- `VaultHandlerV7.changePrice()`
- `VaultHandlerV7.addChainId()`
- `VaultHandlerV7.removeChainId()`

Lack of Event Emitting in Contract `VaultHandlerV7`

According to the [Solidity style guide](https://docs.soliditylang.org/en/v0.6.0/style-guide.html#order-of-layout), the event declarations should follow the variable declarations and should be followed by function declarations.

In the contract `VaultHandlerV7`, the constructor declaration should be at the beginning of the function declaration. 

The internal functions `VaultHandlerV7.stringToBytes32()` and `VaultHandlerV7.bytes32ToStr()` are never used within the contract.

Unused `internal` Functions

In the contract `VaultHandlerV7`, the function `toggleShouldBurn()`, which decides if the ERC20 tokens are burned or not, can be called by anyone.
```solidity=538
    function toggleShouldBurn() public {
        shouldBurn = !shouldBurn;
    }
```

Lack of Access Control on `toggleShouldBurn()`

In the constructor of the contract `Bridged`, `chainBalances` are assigned with the initial value `200000000000000000`. We would like to check with the Emblem team what would be the reason of this initialization.

Initialization of Chain Balances

The function `VaultHandlerV7.buyWithPaymentOnly()` allows users to purchase an NFT token with information stored in `preMints[msg.sender][_tokenId]`. However, the existence of `preMints[msg.sender][_tokenId]` is not checked, which means users can occupy token IDs without `preMints` by purchasing NFT tokens with the IDs and empty payloads.
```solidity=464
        PreMint memory preMint = preMints[msg.sender][_tokenId];
        require(preMint.preImage == sha256(abi.encodePacked(image)), 'Payload does not match'); // Payload should match
```

Emblem

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

36

0

13

0

23