Petaverse security record

Documentation suggests that clones can only be transferred by the Cloner role, however, clones can also be transferred by Custodians if they are owned by custodial accounts, or if clones are owned by the contract they can be transferred by Minters. Additionally, accounts holding cloned pets are free to transfer them as they wish. By design, cloned pets are intended to only be held by the same account that holds the original pet, just on a terminal chain. However, with the ability to transfer these clones, a situation could arise where separate accounts are holding both the original and cloned pet. Also, recipients of cloned pets could be surprised to have their NFT moved by the team at any point.

The function `setRoyaltyInfo()` does not verify the `royaltyReceiver` to not be the `0x0 address`, and the `royaltyNumerator` to be within a defined bound. If the royaltyNumerator is the same as the `ROYALTY_DENOMINATOR`, then the royalty will be the entire sale price. If it is greater than the `ROYALTY_DENOMINATOR` then the royalty will be more than the sale price itself.

Documentation suggests that NFTs should only be transferred, approved, or burned on the origin chain where they were minted. However, there is no smart-contract level enforcement of this. This means NFTs can be transferred, regardless of what chain they are on.

In `Pet.mintTo()`#L222, the custodial status of the to address is updated after the minting of the token, violating the Checks-Effects-Interactions coding pattern. The coding pattern is meant to mitigate any chance of other contracts manipulating the state of the blockchain in unexpected and possibly malicious ways before control is returned to the original contract. While no critical vulnerabilities were identified as a result of this violation, it is still best to adhere to the pattern to mitigate any potential issues.

`Pet.bulkTransferFromCustodials()` is restricted to only being called by an `Custodian` However, the call to `setCustodial()` inside of this function is limited to only addresses with DEFAULT_ADMIN_ROLE. An address with CUSTODIAN_ROLE cannot bulk transfer Pets if they do not additionally hold the DEFAULT_ADMIN_ROLE.

`Voucher.mintTo()`, a voucher can be minted in 2 ways: <ol> <li> The _to address must match the to address on the signed VoucherToken data.</li> <li> As long as the _to data doesnt equal the 0x0 address and _voucher.to is 0x0, then the voucher token can be minted to any arbitrary _to address</li> </ol> The second scenario allows for a front-runner to simulate a `mintTo()` transaction for a voucher token with an unassigned address and see that they can receive this token if they provide their own address in the _to the parameter.

Currently, Voucher only allows the assignment of the value for the address of the Pet proxy contract once, during construction. The inability of the team to modify the address used for the Pet contract presents a single point of failure. If there were an instance where this proxy address was compromised, or assigned an incorrect address, Voucher would have to be redeployed as this contract would be unable to function as intended.

The function `mintTo()` can write the same value as the custodial mapping because it maps an address to a boolean. Consider the case where the address is already marked as custodial, there is no need to do the same write operation again. Given that the `_tos` array contains duplicate addresses, the same goes for the function `multiMintTo()`; except that it writes the same value for the duration of the loop. Although it is only one bit, these write operations can cost more gas than needed.

The contracts can approve an address to transfer tokens on their behalf without setting a limit on how many tokens may be transferred via `ERC721.setApprovalForAll()`. If the approved address becomes hacked or is intentionally malicious, it may transfer out all of the approved tokens.

In the Pet.sol file a `_burn()` function is defined which overrides the burn function of all other contracts it inherits from. However, it is an internal function that is never used in any part of the code.