Unrestricted usage of `soulBounded` parameter breaks `PhiNFT1155` compliance with ERC-1155, and allows to create honey pots

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L215-L257
https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/art/PhiNFT1155.sol#L317

Vulnerability details

Impact

Contract PhiFactory allows the artist at any time to prohibit transfers of issued NFT tokens via setting the soulBounded parameter of the PhiNFT1155 contract. This breaks PhiNFT1155 compatibility with the ERC-1155 standard, and allows the artists to create honey pots from their NFTs.

Summary

Contract PhiFactory allows the artist at any time to set soulBounded parameter to true via updateArtSettings function. When soulBounded is set to true, this effectively disallows any NFT transfers of PhiNFT1155, as safeTransferFrom contains the following fragment:

if (from_ != address(0) && soulBounded(id_)) revert TokenNotTransferable();

The specification of ERC-1155 says the following about safeTransferFrom function:

    @notice Transfers `_value` amount of an `_id` from the `_from` address to the `_to` address specified (with safety call).
    @dev Caller must be approved to manage the tokens being transferred out of the `_from` account (see "Approval" section of the standard).
    MUST revert if `_to` is the zero address.
    MUST revert if balance of holder for token `_id` is lower than the `_value` sent.
    MUST revert on any other error.
    MUST emit the `TransferSingle` event to reflect the balance change (see "Safe Transfer Rules" section of the standard).
    After the above conditions are met, this function MUST check if `_to` is a smart contract (e.g. code size > 0). If so, it MUST call `onERC1155Received` on `_to` and act appropriately (see "Safe Transfer Rules" section of the standard).        

What's implicit in the specification is that safeTransferFrom must succeed when none of the "MUST revert" conditions are met. Notice that soulBounded being set to true is not an encountered error; it's an additional condition, which can be set at will at any time; thus "MUST revert on any other error" clause doesn't apply. Also, this additional mechanism is different the pausing/unpausing of the contract, which is intended to be used only in case of uncovers security bugs to prevent loss of funds. The soulBounded can be set to true by an artist at any time. This means that PhiNFT1155 is not ERC-1155 compliant; the condition mentioned explicitly in the README.

Moreover, the existence and unrestricted usage of soulBounded parameter of PhiNFT1155 contract makes it a perfect example of honey pot:

• An artist first creates the contract with soulBounded set to false; thus NFTs operate as any other NFTs, and can be traded on NFT exchanges.
• Issued NFTs gather popularity, and users invest a lot of money into them.
• The artist sets soulBounded to false; as a result, the PhiNFT1155 contract stops to behave as a normal ERC-1155 contract: no NFTs can change hands.
• The artist can now extract a ransom from the users holding NFTs for unblocking them.

Tools Used

Manual review

Recommended Mitigation Steps

Either remove soulBounded parameter completely, or allow to change it only in one direction: from true to false.

Assessed type

Other