Canto Identity Protocol contest - libratus's results

Lines of code

https://github.com/code-423n4/2023-01-canto-identity/blob/main/src/AddressRegistry.sol#L42-L49

Vulnerability details

Impact

In the contest details it is mentioned that identity can be transferable even if it's currently assigned to address in AddressRegistry. However, I would assume that if another address registers it, the identity should be removed from the previous owner. This is currently not the case.

One of the main properties of identity is that it is unique. However, current logic allows to register identity for many addresses by calling AddressRegistry.register(id) and then transferring the identity NFT to the next address.

If there is a requirement to assign identity to multiple user's addresses, I would advise to explore other options, like using a sub-protocol.

Proof of Concept

This test case results in the same identity being registered for two addresses

diff --git a/src/test/AddressRegistry.t.sol b/src/test/AddressRegistry.t.sol
index 0f4214e..110afb1 100644
--- a/src/test/AddressRegistry.t.sol
+++ b/src/test/AddressRegistry.t.sol
@@ -25,6 +25,28 @@ contract AddressRegistryTest is DSTest {
         addressRegistry = new AddressRegistry(address(cidNFT));
     }
 
+    function testRegistryMultipleAddresses() public {
+        uint256 nftIdOne = 1;
+        address address1 = users[0];
+        address address2 = users[1];
+
+        vm.startPrank(address1);
+        bytes[] memory addList;
+        cidNFT.mint(addList);
+
+        addressRegistry.register(nftIdOne);
+
+        // transfer the identity to another address
+        cidNFT.safeTransferFrom(address1, address2, nftIdOne);
+
+        vm.stopPrank();
+        vm.startPrank(address2);
+        addressRegistry.register(nftIdOne);
+
+        assertEq(addressRegistry.getCID(address1), nftIdOne);
+        assertEq(addressRegistry.getCID(address2), nftIdOne);
+    }
+
     function testRegisterNFTCallerNotOwner() public {

Tools Used

Manual Review

Recommended Mitigation Steps

Track whether identity is registered for an address. Then during registration, make sure to remove it from the previous owner.

diff --git a/src/AddressRegistry.sol b/src/AddressRegistry.sol
index 82bdd5e..87b2905 100644
--- a/src/AddressRegistry.sol
+++ b/src/AddressRegistry.sol
@@ -37,6 +37,8 @@ contract AddressRegistry {
         cidNFT = _cidNFT;
     }
 
+    mapping(uint256 => address) private cidNFTtoAddress;
+
     /// @notice Register a CID NFT to the address of the caller. NFT has to be owned by the caller
     /// @dev Will overwrite existing registration if any exists
     function register(uint256 _cidNFTID) external {
@@ -44,7 +46,13 @@ contract AddressRegistry {
             // We only guarantee that a CID NFT is owned by the user at the time of registration
             // ownerOf reverts if non-existing ID is provided
             revert NFTNotOwnedByUser(_cidNFTID, msg.sender);
+
+        address prevOwner = cidNFTtoAddress[_cidNFTID];
+        if (prevOwner != address(0))
+            delete cidNFTs[prevOwner];
+
         cidNFTs[msg.sender] = _cidNFTID;
+        cidNFTtoAddress[_cidNFTID] = msg.sender;
         emit CIDNFTAdded(msg.sender, _cidNFTID);
     }
 
@@ -53,6 +61,7 @@ contract AddressRegistry {
         uint256 cidNFTID = cidNFTs[msg.sender];
         if (cidNFTID == 0) revert NoCIDNFTRegisteredForUser(msg.sender);
         delete cidNFTs[msg.sender];
+        delete cidNFTtoAddress[cidNFTID];
         emit CIDNFTRemoved(msg.sender, cidNFTID);
     }

L-01 Make CidNFT external functions non-reentrant

It is currently possible to reenter remove function as well as mint and add (because remove can be called from them). This is because remove() uses ERC721.safeTransferFrom which can invoke the callback on the receiver side.

While there were no possible attacks spotted at the moment, even minor code changes can introduce issues related to reentrancy.

Recommendation is to apply reentrancy guard modifiers to these functions.

NC-01 Active subprotocols are a subset of Ordered. Their value can be reconsidered

In order to simplify the contract it may be viable to remove Active subprotocol type. Ordered can be used instead as it's also capable of holding multiple entries. The only downside is that clients would have to manage keys when adding/removing elements themselves, possibly relying on emitted events.

NC-02 Return id when minting an identity

A little QoL change could be to return the id of minted identity from mint function like the following:

diff --git a/src/CidNFT.sol b/src/CidNFT.sol
index d6a23db..79bf848 100644
--- a/src/CidNFT.sol
+++ b/src/CidNFT.sol
@@ -144,8 +144,9 @@ contract CidNFT is ERC721, ERC721TokenReceiver {
     /// @dev An address can mint multiple CID NFTs, but it can only set one as associated with it in the AddressRegistry
     /// @param _addList An optional list of encoded parameters for add to add subprotocol NFTs directly after minting.
     /// The parameters should not include the function selector itself, the function select for add is always prepended.
-    function mint(bytes[] calldata _addList) external {
-        _mint(msg.sender, ++numMinted); // We do not use _safeMint here on purpose. If a contract calls this method, he expects to get an NFT back
+    function mint(bytes[] calldata _addList) external returns (uint256) {
+        uint256 id = ++numMinted;
+        _mint(msg.sender, id); // We do not use _safeMint here on purpose. If a contract calls this method, he expects to get an NFT back
         bytes4 addSelector = this.add.selector;
         for (uint256 i = 0; i < _addList.length; ++i) {
             (
@@ -154,6 +155,7 @@ contract CidNFT is ERC721, ERC721TokenReceiver {
             ) = address(this).delegatecall(abi.encodePacked(addSelector, _addList[i]));
             if (!success) revert AddCallAfterMintingFailed(i);
         }
+        return id;
     }

NC-03 Code can be simplified when adding subprotocol entires of type Active

There is no need to check whether there are any existing entries of a subprotocol. The following condition can be removed:

diff --git a/src/CidNFT.sol b/src/CidNFT.sol
index d6a23db..79bf848 100644
--- a/src/CidNFT.sol
+++ b/src/CidNFT.sol
@@ -212,18 +214,11 @@ contract CidNFT is ERC721, ERC721TokenReceiver {
             if (!subprotocolData.active) revert AssociationTypeNotSupportedForSubprotocol(_type, _subprotocolName);
             IndexedArray storage activeData = cidData[_cidNFTID][_subprotocolName].active;
             uint256 lengthBeforeAddition = activeData.values.length;
-            if (lengthBeforeAddition == 0) {
-                uint256[] memory nftIDsToAdd = new uint256[](1);
-                nftIDsToAdd[0] = _nftIDToAdd;
-                activeData.values = nftIDsToAdd;
-                activeData.positions[_nftIDToAdd] = 1; // Array index + 1
-            } else {
-                // Check for duplicates
-                if (activeData.positions[_nftIDToAdd] != 0)
-                    revert ActiveArrayAlreadyContainsID(_cidNFTID, _subprotocolName, _nftIDToAdd);
-                activeData.values.push(_nftIDToAdd);
-                activeData.positions[_nftIDToAdd] = lengthBeforeAddition + 1;
-            }
+
+            if (activeData.positions[_nftIDToAdd] != 0)
+                revert ActiveArrayAlreadyContainsID(_cidNFTID, _subprotocolName, _nftIDToAdd);
+            activeData.values.push(_nftIDToAdd);
+            activeData.positions[_nftIDToAdd] = lengthBeforeAddition + 1;
             emit ActiveDataAdded(_cidNFTID, _subprotocolName, _nftIDToAdd, lengthBeforeAddition);
         }
     }

NC-04 Events have obsolete values

When removing subprotocol entries, parameter _nftIDToRemove only matters if working with type Active. However, it is included in the events for Ordered and Primary types.

https://github.com/code-423n4/2023-01-canto-identity/blob/main/src/CidNFT.sol#L265

https://github.com/code-423n4/2023-01-canto-identity/blob/main/src/CidNFT.sol#L271

It is pointless to include _nftIDToRemove in these events