M-03.md

AddressRegistry can associate same CID to different addresses at the same time

The AddressRegistry contract can associate a CID NFT to an account address. As stated in the contest, the CID NFT can be transferred out of the account that registered it. However, once transferred it can be registered again while keeping the previous registration.

Impact

The same CID NFT can be registered under multiple accounts at the same time.

This happens because the registration process doesn't check if the CID NFT has been previously registered in order to blank the previous association. This may lead to having the same CID NFT registered for multiple accounts at the same time, as the CID NFT can be registered, then transferred, then registered again, and this process can be repeated any number of times.

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

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);
}

PoC

The following test demonstrates the issue:

contract Note is ERC20 {
    constructor() ERC20("Note", "NOTE", 18) {}

    function mint(address account, uint256 amount) external {
        _mint(account, amount);
    }
}

contract AuditTest is DSTest {
    Vm internal immutable vm = Vm(HEVM_ADDRESS);

    string internal constant BASE_URI = "tbd://base_uri/";

    address feeWallet;
    address alice;
    address bob;
    address attacker;

    Note note;
    SubprotocolRegistry subprotocolRegistry;
    CidNFT cidNFT;
    SubprotocolNFT sub1;
    SubprotocolNFT sub2;
    AddressRegistry addressRegistry;

    function setUp() public {
        feeWallet = vm.addr(0x1);
        alice = vm.addr(0x2);
        bob = vm.addr(0x3);
        attacker = vm.addr(0x4);

        note = new Note();
        subprotocolRegistry = new SubprotocolRegistry(address(note), feeWallet);
        cidNFT = new CidNFT(
            "MockCidNFT",
            "MCNFT",
            BASE_URI,
            feeWallet,
            address(note),
            address(subprotocolRegistry)
        );
        sub1 = new SubprotocolNFT();
        sub2 = new SubprotocolNFT();
        addressRegistry = new AddressRegistry(address(cidNFT));
    }

    function test_AddressRegistry_DuplicateRegistration() public {
        vm.startPrank(alice);

        // Mints CidNFT
        cidNFT.mint(new bytes[](0));
        uint256 cidNFTID = 1;
        assertEq(cidNFT.ownerOf(cidNFTID), alice);

        // Alice registers the CID
        addressRegistry.register(cidNFTID);
        assertEq(addressRegistry.getCID(alice), cidNFTID);

        // Moves token to Bob
        cidNFT.transferFrom(alice, bob, cidNFTID);

        vm.stopPrank();

        vm.startPrank(bob);

        // Alice registers the same CID
        addressRegistry.register(cidNFTID);

        // Alice and Bob have the same CID
        assertEq(addressRegistry.getCID(alice), cidNFTID);
        assertEq(addressRegistry.getCID(bob), cidNFTID);

        vm.stopPrank();
    }
}

Recommendation

Keep an association of which CID NFTs have been already registered to blank any previous registration when the CID NFT is registered again:

  contract AddressRegistry {
      ...
+     mapping(uint256 => address) private registeredCidNFTs;
      ...
      
      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);
          
+         address previousAccount = registeredCidNFTs[_cidNFTID];
+         if (previousAccount != address(0)) {
+             delete cidNFTs[previousAccount];
+         }    
              
          cidNFTs[msg.sender] = _cidNFTID;
+         registeredCidNFTs[_cidNFTID] = msg.sender;
          emit CIDNFTAdded(msg.sender, _cidNFTID);
      }
  }