Canto Identity Protocol contest - adriro's results

Lines of code

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

Vulnerability details

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

Low issues

Follow "Checks Effects Interactions"

See https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html

• https://github.com/code-423n4/2023-01-canto-identity/blob/main/src/SubprotocolRegistry.sol#L79
  In SubprotocolRegistry.register external call to note transfer comes first.

delegatecall in CidNFT.mint can be replaced by internal calls

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

Prefer an internal call over the use of delegatecall as the add function is present in the same contract.

Non-critical issues

Non-library/interface files should use fixed compiler versions, not floating ones

Pin fixed Solidity versions in pragma specifications.

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

CidNFT reimplements onERC721Received

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

The contract is reimplementing the onERC721Received callback as this is already provided by the inherited ERC721TokenReceiver mixin from solmate.