Unseen - Audit

The `ProxyRegistry` contract plays an important role in the `TheGenerates` contract. Specifically, in the `MarketsPreapproved` contract (which is inherited by `TheGenerates` contract), the `isApprovedForAll()` function returns `true` if the `operator` is a proxy for the user. Subsequently, the `isAuthorized` modifier defined in the `ERC4907A` contract would pass for the user's `tokenId`, which means that the proxy can control the NFT corresponding to the `tokenId`. The proxy is recorded in the `ProxyRegistry` contract, specifically the `proxies` mapping. The intended behavior is that a user can only set its own proxy, and not any other user's proxy, so a user can control the NFT either directly or via its proxy. However, the `transferAccessTo()` function in the `ProxyRegistry` contract is capable of updating the `proxies[to]` mapping, and the access control in the `ProxyRegistry` and `AuthenticatedProxy` contracts is insufficient. This enables user Bob to change the `proxies[Alice]` to his own proxy, and subsequently takes control of any `tokenId` that Alice owns via Bob's proxy. Note that in the `AuthenticatedProxy` contract, the `transferProxyOwnership()` function enables the proxy owner to transfer the ownership to a new owner, and update the mapping in the registry. However, the `AuthenticatedProxy` contract also contains a `proxy()` function and a `proxyAssert()` function that can also call the `transferAccessTo()` function of the Registry, but does NOT change the `owner` variable. This allows Bob to retain ownership of his proxy, while updating the `proxies[Alice]` value in the `ProxyRegistry` contract. A proof of concept test case is added to the `exchange.registry.spec.ts` file to demonstrate the above. This vulnerability impacts all other contracts that utilize the registry and proxy contracts, including `TheGenerates` and `UnseenExchange`. For `TheGenerates`, a malicious user could effectively take control of another user's NFT. For `UnseenExchange`, the ability to manipulate the `proxy` address in line 356 of the `ExchangeCore` contract allows a malicious user to brick legitimate atomic match transactions.

The `init` function is supposed to be called by the contract owner only one time, as the comment in line 116 indicates. However, the check in line 120 does not prevent the function from being called more than once. As a result, the owner can change the `tokenId` and add new minters at will. This represents a significant risk to the token holders, as a new minter can mint a large amount of tokens and also arbitrarily burn tokens from other addresses. ```solidity=113 /** * @notice Initialize the bridge parmas. * * This can only be called by the contract owner and one time. */ function init(bytes32 tokenId_, address tokenManager) external onlyOwner { if (tokenId_ == bytes32(0)) revert TokenIdZero(); if (tokenId_ == tokenId) revert TokenIdIsAlreadySet(); tokenId = tokenId_; _addMinter(tokenManager); } ```

The `validateOrderAuthorization()` function is intended to "validate if maker signed the order hash" (comment in line 173). However, in line 459 and 471, the `executor` address is passed in to the function instead. If the `maker` and `executor` are different addresses, this represents the inconsistency between the intended design and the actual behavior of the code.

According to EIP 4906 (https://eips.ethereum.org/EIPS/eip-4906), The `supportsInterface` method MUST return `true` when called with `0x49064906`. The `VestingLockup` contract inherits `IERC4906` but the `supportsInterface` method is not updated to conform to the EIP 4906 standard.

When the owner creates a new schedule either via `createSchedule()` or `createMultiSchedules()`, there is no check that the `params.recipient` can handle the received ERC721. If `params.recipient` is a smart contract that cannot handle the ERC721, the corresponding vesting schedule could be unwithdrawable.

Functions with names starting with `_` should be declared as `private`/`internal`.

The `ecrecover()` function can return the zero address (0x0000000000000000000000000000000000000000) if the signature was invalid or the message was malformed. When the smart contract does not check the output of `ecrecover()` and assumes it is always a valid address, vulnerability may arise.

It is important to emit events for sensitive actions, particularly those that can be executed by centralized roles or administrators. This ensures transparency and enables tracking of critical changes, which is essential for security and trust in the system. Missing event logs can indeed result in a lack of visibility and potential information loss.

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.

The function `setDefaultRoyalty` receives the `feeNumerator` is used for royalty payment. In the marketplaces which support EIP-2981, `salePrice * ( feeNumerator/10000 `) will be sent to the royalty recipient. But there's no upper limit for `feeNumerator` when setting the default royalty and it can be as high as 100%.