Canto Identity Protocol contest - d3e4's results

Beneficiaries are fixed

The recipient of subprotocol registration fees, SubprotocolRegistry.cidFeeWallet, and of CID fees, CidNFT.cidFeeWallet, are currently only set in the constructor and cannot be changed. This could result in forever missing out on the intended revenue if these accounts ever were to be lost or become compromised. Similarly, the owner of the subprotocol, SubprotocolRegistry.SubprotocolData.owner, which receives fees when adding an NFT, currently also cannot be changed. This inflexibility also prevents the protocol and subprotocols from being transferred (e.g. sold), which may be especially relevant for subprotocols. Consider implementing a back-up administrator who can change the cidFeeWallet, and similarly for changing the owner of a subprotocol.

Removed NFT is not sent back to owner

CidNFT.remove always sends the NFT to msg.sender. When granting approval to another address for one's CID NFT, this may have the unintended consequence that when this approved address is adding an NFT to a subprotocol, such that CidNFT returns the preexisting NFT registered in its place, this preexisting NFT is transferred to the approved address making the call rather than its owner. The approved address may not be able to handle or transfer the received NFT. The owner may thus inadvertently burn one of his NFTs simply by having another address add an NFT. Consider always sending the NFT back to its owner, or letting the recipient be chosen.

Association type flexibility may be moot

A subprotocol can be registered with any combination of association types. This suggests that all of the supported association types can be used in parallel. However, an NFT can only be added to one specific association type; it cannot be transferred to another association type within the subprotocol, and it cannot be listed under more than one association type. This means that even if a subprotocol has support for multiple association types, it acts effectively as several copies of one subprotocol, identical up to association type. Unless the intention is specifically to offer a discount for registering the same subprotocol with more than one association type, it would be logically more transparent to explicitly allow only one association type per protocol. But even if the discount is intended, it seems users would then be incentivized to preemptively always register their subprotocols with all association types enabled, yet again rendering this intended flexibility moot.

Registration fee in $NOTE is fixed

The fee for registering a subprotocol is fixed at 100 $NOTE. The effective and desired value of this amount may change unpredictably and significantly in the future, rendering the contract either unprofitable or too expensive to use. Since a resetting of this amount doesn't have any retrospective effect on users, implementing a setter of REGISTER_FEE would only serve to safeguard the contract against said eventualities.

A CID NFT can be registered to an address which does not own the CID NFT

Since a user can transfer his CID NFT after having registered it in the AddressRegistry, and this registration is not thereby removed, multiple users can end up being registered to the same CID NFT, without owning it. This needlessly obfuscates the meaning of the registration and muddles the notion of CID NFTs as on-chain identities, and might lead to third-party misunderstandings, e.g. if it ever were to be construed as indicating ownership of the CID NFT. Furthermore, automatically removing the registration offers an opportunity for gas savings: consider incorporating the address registry in the CidNFT contract such that whenever a CID NFT is transferred its previous owner's entry is deleted from the cidNFTs-mapping.

Comment typo

It seems "the value will not be unset" should be "the value will be unset" or equivalent.