Canto Identity Protocol contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

The _cidNFTID will be registered to incorrect owner in case of NFT transfer. In some cases this can lead to multiple registration for same _cidNFTID

Proof of Concept

1. Lets say _cidNFTID 1 is owned by User A
2. User A registers this _cidNFTID 1 using register function

function register(uint256 _cidNFTID) external {
        if (ERC721(cidNFT).ownerOf(_cidNFTID) != msg.sender)
            // 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);
        cidNFTs[msg.sender] = _cidNFTID;
        emit CIDNFTAdded(msg.sender, _cidNFTID);
    }

3. Post some time User A transfer this _cidNFTID to User B. This is allowed since cidNFT contract implements ERC721 contract
4. Since cidNFT does not contain any overridden implementation of transfer so default transfer from ERC721 contract is made
5. Post transfer the registration still point to old owner. Even if new owner registers, there will be multiple users for same _cidNFTID

Recommended Mitigation Steps

Registration of _cidNFTID should be removed if _cidNFTID is transferred