Subprotocol NFT can be registered again under a different name

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-canto-identity/blob/main/src/SubprotocolRegistry.sol#L79-L101

Vulnerability details

The SubprotocolRegistry contract allows subprotocol owners to register their subprotocol NFT under a name.

Despite frontrunning issues described in the contest, the same subprotocol NFT can be registered again under a different name, as there is no check to verify if a subprotocol NFT has been already registed.

Impact

A bad actor can re-register an already registered subprotocol NFT. Even though this requires the registered name to be different, it is possible to register the same subprotocol NFT contract to trick others into thinking this is the proper registration for the subprotocol.

The register function present in the SubprotocolRegistry doesn't check that the _nftAddress has been previously registered as a subprotocol, and allows for a bad actor to register a subprotocol under the same _nftAddress as previously registered subprotocol.

https://github.com/code-423n4/2023-01-canto-identity/blob/main/src/SubprotocolRegistry.sol#L79-L101

function register(
    bool _ordered,
    bool _primary,
    bool _active,
    address _nftAddress,
    string calldata _name,
    uint96 _fee
) external {
    SafeTransferLib.safeTransferFrom(note, msg.sender, cidFeeWallet, REGISTER_FEE);
    if (!(_ordered || _primary || _active)) revert NoTypeSpecified(_name);
    SubprotocolData memory subprotocolData = subprotocols[_name];
    if (subprotocolData.owner != address(0)) revert SubprotocolAlreadyExists(_name, subprotocolData.owner);
    subprotocolData.owner = msg.sender;
    subprotocolData.fee = _fee;
    if (!ERC721(_nftAddress).supportsInterface(type(CidSubprotocolNFT).interfaceId))
        revert NotASubprotocolNFT(_nftAddress);
    subprotocolData.nftAddress = _nftAddress;
    subprotocolData.ordered = _ordered;
    subprotocolData.primary = _primary;
    subprotocolData.active = _active;
    subprotocols[_name] = subprotocolData;
    emit SubprotocolRegistered(msg.sender, _name, _nftAddress, _ordered, _primary, _active, _fee);
}

PoC

In the following test, an attacker registers an already registered subprotocol created by Alice.

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_SubprotocolRegistry_ReRegisterSubprotocolNFT() public {
        note.mint(alice, 100 * (10**18));
        note.mint(attacker, 100 * (10**18));

        // Setup
        vm.startPrank(alice);

        // Alice registers subprotocol
        note.approve(address(subprotocolRegistry), type(uint256).max);
        subprotocolRegistry.register(
            true,
            true,
            true,
            address(sub1),
            "sub1",
            0
        );

        vm.stopPrank();

        // Attack
        vm.startPrank(attacker);

        // Attacker registers same subprotocol NFT under a different name
        note.approve(address(subprotocolRegistry), type(uint256).max);
        subprotocolRegistry.register(
            true,
            true,
            true,
            address(sub1),
            "sub1-duplicate",
            0
        );

        vm.stopPrank();
    }
}

Recommendation

Keep track of registered subprotocol NFTs and check if it was already registered during the call to register:

  contract SubprotocolRegistry {
      ...
+     mapping(address => bool) private registered;
      ...

      function register(
          bool _ordered,
          bool _primary,
          bool _active,
          address _nftAddress,
          string calldata _name,
          uint96 _fee
      ) external {
          SafeTransferLib.safeTransferFrom(note, msg.sender, cidFeeWallet, REGISTER_FEE);
          if (!(_ordered || _primary || _active)) revert NoTypeSpecified(_name);
          SubprotocolData memory subprotocolData = subprotocols[_name];
          if (subprotocolData.owner != address(0)) revert SubprotocolAlreadyExists(_name, subprotocolData.owner);
          subprotocolData.owner = msg.sender;
          subprotocolData.fee = _fee;
+         if (registered[nftAddress])
+             revert SubprotocolNFTAlreadyRegistered(_nftAddress);
          if (!ERC721(_nftAddress).supportsInterface(type(CidSubprotocolNFT).interfaceId))
              revert NotASubprotocolNFT(_nftAddress);
          subprotocolData.nftAddress = _nftAddress;
          subprotocolData.ordered = _ordered;
          subprotocolData.primary = _primary;
          subprotocolData.active = _active;
          subprotocols[_name] = subprotocolData;
+         registered[nftAddress] = true;
          emit SubprotocolRegistered(msg.sender, _name, _nftAddress, _ordered, _primary, _active, _fee);
      }
  }

[c4-sponsor]

OpenCoreCH marked the issue as disagree with severity

[OpenCoreCH]

By design. If we would only allow one registration, we would need to permission the registration to the owner (as frontrunning could then be a huge problem), see #185 for some thoughts on why we do not do this.

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-b