XRGB-Smart contract audit

The ERC404 contract is a sophisticated blend of ERC20 and ERC721 standards, introducing a unique approach to handle tokens with mixed characteristics. However, a review of the contract reveals an oversight in the implementation of the `transferFrom()` function, particularly concerning whitelist verification during ERC721 transfers. In the `transferFrom()` function, the contract distinguishes between ERC20 and ERC721 transfers based on the `amountOrId` parameter. For `amountOrId` values that are less than or equal to the `minted` variable, the contract performs an ERC721 transfer. Otherwise, it handles an ERC20 transfer. The issue occurs within the ERC721 transfer mechanism, as the contract fails to verify whether the recipient is whitelisted prior to executing the transfer. In contrast, the ERC20 transfer mechanism includes this verification. This lack of whitelist verification can lead to unintended consequences, particularly in scenarios where the contract logic assumes certain addresses (like those of smart contracts designed for liquidity pools or trading platforms) should be prevented from directly receiving ERC721 tokens due to potential compatibility issues. ```solidity if (amountOrId <= minted) { if (from != _ownerOf[amountOrId]) { revert InvalidSender(); } if (to == address(0)) { revert InvalidRecipient(); } // Authorization checks are performed, but no whitelist verification if ( msg.sender != from && !isApprovedForAll[from][msg.sender] && msg.sender != getApproved[amountOrId] ) { revert Unauthorized(); } balanceOf[from] -= units; unchecked { balanceOf[to] += units; } _ownerOf[amountOrId] = to; delete getApproved[amountOrId]; // Transfer logic for ERC721 tokens does not include whitelist checks ... } ```

Based on the contract design of `ERC404`, if `ERC721` token owners are included in the whitelist, they are allowed to transfer their `ERC20` balance but retain their `ERC721` balance. Consequently, these owners can duplicate the number of `ERC721` tokens they own. To illustrate, let's assume a token owner possesses `t` `ERC721` tokens and `t*units` `ERC20` tokens. Once the owner's account, referred to as `accA`, is whitelisted, they can use the `transfer()` function to transfer their `t*units` `ERC20` tokens to another account, `accB`. It's important to note that `accA` still maintains its `ERC721` balance, which is `t`. Later on, if `accA` is removed from the whitelist, they can transfer the `ERC20` balance from `accB` back to `accA`. As a result of this transfer, `accA` will have `t*units` `ERC20` tokens and a total of `2*t` `ERC721` tokens, including `t` newly minted tokens. ```solidity function setWhitelist( string memory ticker, address target, bool state ) public onlyOwner { if (_ticker[ticker] == address(0x0)) { revert Errors.InvalidTicker(); } BRC404(_ticker[ticker]).setWhitelist(target, state); } ```

The BRC404 contract, an extension of the ERC404 contract, introduces an issue within its token burn mechanism. This issue stems from the interaction between the `_burnBRC404()` function and its reliance on the `_transferERC20WithERC721()` method for implementing the token burn logic. Specifically, the approach to token burning is misaligned with the conventional expectations of how token burning should affect the total supply of tokens, especially in the context of ERC20 tokens, and mismanages ERC721 token handling. The `_burnBRC404()` function attempts to burn tokens by internally calling `_transferERC20WithERC721()`, passing the zero address (`address(0)`) as the destination for the tokens being burned. The internal function designed to facilitate the transfer and, by extension, the burning of ERC20 tokens does not decrease the `totalSupply` of the token when tokens are transferred to the zero address. This oversight leads to a discrepancy where burned tokens are not accurately reflected in the total supply, undermining the principle of token burning which intends to reduce the available supply of tokens. ```solidity function _burnBRC404(address from, uint256 amount) internal { _transferERC20WithERC721(from, address(0), amount); } ``` The `_transferERC20WithERC721()` function, as called above, is expected to handle both ERC20 and ERC721 token transfers. For ERC20 tokens, the function does not decrease the `totalSupply` when transferring to the zero address. This behavior diverges from the standard practice of burning tokens, where the `totalSupply` should be reduced to reflect the removal of tokens from circulation. ```solidity function _transferERC20( address from_, address to_, uint256 value_ ) internal virtual { if (from_ == address(0)) { totalSupply += value_; } else { balanceOf[from_] -= value_; } unchecked { balanceOf[to_] += value_; } emit ERC20Events.Transfer(from_, to_, value_); } ```

The function `transferFrom()` in the contract `ERC404` is designed to implement the experimental `ERC-404` token standard on the blockchain. `ERC-404` was designed to merge the characteristics of fungible (`ERC-20`) and non-fungible tokens (`ERC-721`) into a hybrid form, allowing for both unique and divisible asset representations on the blockchain. However, there is no return value in the function `transferFrom()`. ```solidity function transferFrom( address from, address to, uint256 amountOrId ) public virtual { ... } ``` Based on the official `ERC20` standard, it should have a return value in boolean. i.e. `function transferFrom(address _from, address _to, uint256 _value) public returns (bool success)`. Missing return value may result in the inability to integrate with other projects. i.e lending/exchange protocol.

According to the standard, the `tokenURI` method must be reverted if a non-existent `tokenId` is passed. However, in the contract `BRC404`, this point was ignored. What leads to a [violation of the EIP721 spec](https://eips.ethereum.org/EIPS/eip-721#specification).

The function `approve()` is designed for owners to approve authority to spenders. Authorize both `ERC20` and `ERC721` token types simultaneously. However, both of these two authorities emit the same event which cause ambiguity. The third parameter value represent token id when approving authority for `ERC721` token or token amount when approving authority for `ERC20` token. ```solidity function approve( address spender, uint256 amountOrId ) public virtual returns (bool) { if (amountOrId <= minted && amountOrId > 0) { address owner = _ownerOf[amountOrId]; if (msg.sender != owner && !isApprovedForAll[owner][msg.sender]) { revert Unauthorized(); } getApproved[amountOrId] = spender; emit Approval(owner, spender, amountOrId); } else { allowance[msg.sender][spender] = amountOrId; emit Approval(msg.sender, spender, amountOrId); } return true; } ```

The function `_mintERC20()` lacks of validation for max supply of `ERC20` tokens. ```solidity function _mintERC20(address to_, uint256 value_) internal virtual { /// You cannot mint to the zero address (you can't mint and immediately burn in the same transfer). if (to_ == address(0)) { revert InvalidRecipient(); } _transferERC20WithERC721(address(0), to_, value_); } ```

Addresses are not validated before assignment or external calls, potentially allowing the use of zero addresses and leading to unexpected behavior or vulnerabilities. For example, transferring tokens to a zero address can result in a permanent loss of those tokens. **BRC404Factory**: ```solidity function withdraw(address to) external onlyOwner {...} ``` - `to` is not zero-checked before being used. --- **ERC404**: ```solidity function _setWhitelist(address target, bool state) internal {...} ``` - `target` is not zero-checked before being used. ```solidity function approve( address spender, uint256 amountOrId ) public virtual returns (bool) {...} ``` - `spender` is not zero-checked before being used. ```solidity function setApprovalForAll(address operator, bool approved) public virtual {...} ``` - `operator` is not zero-checked before being used. ```solidity function _transfer( address from, address to, uint256 amount ) internal returns (bool) {...} ``` - `from` and `to` are not zero-checked before being used. ```solidity function _mintBRC404(address to, uint256 amount) internal { if (totalSupply + amount > maxSupply) { revert Errors.ExceedMaxSupply(); } _transferERC20WithERC721(address(0), to, amount); } ``` - `to` is not zero-checked before being used.

The state variable `minted` represents the total number of `ERC721` tokens that have been minted. Additionally, `ERC721` tokens can be burned, and the corresponding token IDs will be returned to the bank `_storedERC721Ids`, where they can be reused when minting new tokens. However, the function `erc721TotalSupply()` only returns the count of tokens minted thus far and does not account for the tokens that have been burned. ```solidity function erc721TotalSupply() public view virtual returns (uint256) { return minted; } ```

In the `DOMAIN_SEPARATOR()` function to calculate the domain separator for the EIP712 standard, it will be checked if the current blockchain id is similar to the cached one (i.e., `_INITIAL_DOMAIN_SEPARATOR` calculated in the constructor). This check is to ensure the domain separator should be recalculated after a hard fork and prevent potential replay attacks due to it. ```solidity=419 /// @notice Returns domain initial domain separator, or recomputes if chain id is not equal to initial chain id function DOMAIN_SEPARATOR() public view virtual returns (bytes32) { return block.chainid == _INITIAL_CHAIN_ID ? _INITIAL_DOMAIN_SEPARATOR : _computeDomainSeparator(); } ``` However, it will have a compatibility issue if the contract is used as an implementation contract of a proxy, namely the contract is used in `delegatecall`, where the `address(this)` should be the proxy contract. In this case, the domain separator should be recalculated. As a reference, Openzeppelin fix this issue in the [PR #2852](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2852).