XRGB security record

In the contract `BRC404` the role `_factory` has authority over the functions shown in the diagram below.  Any compromise to the **_factory** account may allow a hacker to take advantage of this authority and do the following: - burn any amount of `BRC404` token from any account. - mint any amount of `BRC404` token to any account. - add or remove any account to whitelist. - set base token uri for `BRC404` token. In the contract `BRC404Factory` the role `_owner` has authority over the functions shown in the diagram below.  Any compromise to the **_owner** account may allow a hacker to take advantage of this authority and do the following: - add or remove any account to whitelist. - set base token uri for `BRC404` token. - create `BRC404` token. - mint any amount of `BRC404` token to any account. - add support chain. - change the cross-chain fee. - withdraw native tokens from the `BRC404Factory` contract.

In the contract `BRC404Factory`, the role owner has the authority to update the token balance of an arbitrary account without sanity restriction. Any compromise to the owner account may allow a hacker to take advantage of this authority and manipulate users' balances. For example, The hacker could also update his/her balance to a large number, sell these tokens, and cause the token price to drop.

According to the logic of the function `transferFrom()`, if the value of the passed parameter `amountOrId` is less than the value of the state variable `minted`, the transfer operation will be considered as an `ERC721` transfer. A `ERC721` token and unit `ERC20` tokens will be transferred. ```solidity function transferFrom( address from, address to, uint256 amountOrId ) public virtual { if (amountOrId <= minted) { if (from != _ownerOf[amountOrId]) { revert InvalidSender(); } if (to == address(0)) { revert InvalidRecipient(); } 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]; // update _owned for sender uint256 updatedId = _owned[from][_owned[from].length - 1]; _owned[from][_ownedIndex[amountOrId]] = updatedId; // pop _owned[from].pop(); // update index for the moved id _ownedIndex[updatedId] = _ownedIndex[amountOrId]; // push token to to owned _owned[to].push(amountOrId); // update index for to owned _ownedIndex[amountOrId] = _owned[to].length - 1; emit Transfer(from, to, amountOrId); emit ERC20Transfer(from, to, units); } else { uint256 allowed = allowance[from][msg.sender]; if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amountOrId; _transfer(from, to, amountOrId); } } ``` However, if a transfer operation intends to send a small quantity of `ERC20` tokens, the amount will be treated as an `NFT ID`, which is unexpected. **[CertiK, 03/06/2024]** The team updated the codebase in the commit [e978ce7e804b2476d840e003784502bae77bcc82](https://github.com/XRGB/xrgb-contracts-v1/tree/e978ce7e804b2476d840e003784502bae77bcc82/contracts) with a new design. The function uses `_isValidTokenId` to check if the transfer is an ERC721 transfer or an ERC20 transfer. For example, in the `trasnferFrom()` function ```solidity function transferFrom( address from_, address to_, uint256 valueOrId_ ) public virtual returns (bool) { if (_isValidTokenId(valueOrId_)) { erc721TransferFrom(from_, to_, valueOrId_); } else { // Intention is to transfer as ERC-20 token (value). return erc20TransferFrom(from_, to_, valueOrId_); } return true; } ``` The `_isValidTokenId` checks if the id is smaller than `minted`, which is the biggest number of the token id. ```solidity function _isValidTokenId(uint256 id_) internal view returns (bool) { return id_ <= minted && id_ != type(uint256).max; } ``` Therefore, the amount that is smaller than the value of `minted` will be treated as an ERC721 transfer, using `erc721TransferFrom()` to transfer. However, since the sender may not hold the NFT with such an ID, the token transfers will fail.