Kephi Audit

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, ```solidity=90 function mint( string calldata _tokenURI, bytes calldata signature ) external payable { _verifySigner(_tokenURI, signature); require(!hasTokenWithURI[_tokenURI], "ERC721Main: URI already exists"); require(msg.value == IFees(factory).getFee(), "Wrong mint fees"); payable(IFees(factory).getReceiver()).transfer(msg.value); uint256 tokenId = _lastMintedId++; _safeMint(_msgSender(), tokenId); hasTokenWithURI[_tokenURI] = true; _setTokenURI(tokenId, _tokenURI); setApprovalForAll(IExchangeProvider(factory).exchange(), true); ``` The `mint()` function requires `hasTokenWithURI[_tokenURI]` is `false` (i.e., the token to be minted does not exist). In L98 of `ERC721Main.sol`, it initiates an external call `transfer()`, and the states related to the minted token (f.e., `hasTokenWithURI[_tokenURI]` value) will be updated only after the external call on L98. In this case, if `IFees(factory).getReceiver()` is a malicious contract with a fallback function to reentrance, it could bypass the check on L95 by recursively invoking the `mint()` to set multiple ERC721 tokens to one URI.

OpenZeppelin's latest implementation of [`ERC721Enumerable`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Enumerable.sol#L72)'s `_beforeTokenTransfer()` contains an invocation of `ERC721._beforeTokenTransfer(from, to, tokenId)`: ```solidity function _beforeTokenTransfer( address from, address to, uint256 tokenId ) internal virtual override { super._beforeTokenTransfer(from, to, tokenId); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); } else if (from != to) { _removeTokenFromOwnerEnumeration(from, tokenId); } if (to == address(0)) { _removeTokenFromAllTokensEnumeration(tokenId); } else if (to != from) { _addTokenToOwnerEnumeration(to, tokenId); } } ``` Additionally, OpenZeppelin's latest implementation of [`ERC721Enumerable`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Enumerable.sol#L30)'s `supportsInterface()` contains an invocation of `ERC721.supportsInterface(interfaceId)`: ```solidity function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC721) returns (bool) { return interfaceId == type(IERC721Enumerable).interfaceId || super.supportsInterface(interfaceId); } ``` Therefore, calling `ERC721._beforeTokenTransfer(from, to, tokenId);` on L54 and `ERC721.supportsInterface(interfaceId)` on L75 are unnecessary.

`public` functions that are never called by the contract could be declared as `external`. `external` functions are more efficient than `public` functions. For example, - `ERC1155Main.uri()` - `ERC1155URIStorage.tokenURI()`

The return values of `transferFrom()` is not properly handled. For example, ```solidity=104 IERC20(tokenToSell.tokenAddress).transferFrom() ``` ```solidity=111 IERC20(tokenToSell.tokenAddress).transferFrom() ``` ```solidity=181 IERC20(tokenToSell.tokenAddress).transferFrom() ``` ```solidity=188 IERC20(tokenToSell.tokenAddress).transferFrom() ``` `transferFrom()` is not void-return functions per `IERC20` interface. Ignoring the return values of the functions might cause some unexpected exceptions, especially if the called functions do not revert automatically on failure.

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 different compiler versions. 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.

`feeAmounts` is an arbitrary input parameter for functions `makeExchangeERC1155()` and `makeExchangeERC721()`. Without upper bound restrictions, `SIGNER_ROLE` is able to push any values into `feeAmounts`, thus affecting the tokens the seller can receive since `tokenToSeller` is calculated as: ```solidity=102 tokenToSeller = tokenToSeller - feeAmounts[i]; ``` In an extreme case, setting `feeAmounts[i]` equal to `tokenToSeller` will result in NFT sellers receiving 0 tokens for the exchange transaction.

Variable `exchange` is initialized but not used in the contract.

The following variables could be declared as `constant` since these state variables are never modified. - `ERC721Main.SIGNER_ROLE` - `Exchange.SIGNER_ROLE` - `FactoryErc1155.SIGNER_ROLE` - `FactoryErc721.SIGNER_ROLE` - `ERC1155Main.SIGNER_ROLE`