SubprotocolRegistry accepts empty string as protocol name

[code423n4]

Lines of code

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

Vulnerability details

Impact

The input sanitization statements in SubprotocolRegistry.sol's register() function are:

function register(
        bool _ordered,
        bool _primary,
        bool _active,
        address _nftAddress,
        string calldata _name,
        uint96 _fee
    ) external {
        // ...
        if (!(_ordered || _primary || _active)) revert NoTypeSpecified(_name);
        SubprotocolData memory subprotocolData = subprotocols[_name];
        if (subprotocolData.owner != address(0)) revert SubprotocolAlreadyExists(_name, subprotocolData.owner);
        // ...
        if (!ERC721(_nftAddress).supportsInterface(type(CidSubprotocolNFT).interfaceId))
            revert NotASubprotocolNFT(_nftAddress);
        // ...
    }

The subprotocol registry does not check for _name, and accepts an empty string "" in the protocol name field. This is an issue since in practically all manual contract calling tools (remix, etherscan etc), empty string is the default value for a string field. In remix, it does even raise an error if a function expecting a string is called without writing anything in the parameter dialog box.

A malicious user can register the empty string "" subprotocol and specify non-zero fees. The user can register a bogus contract as the _nftAddress which passes the supportsInterface check and always returns true in safeTransferFrom calls, allowing anyone to call add(). Any user/developer unfortunate enough to make an add() call in cidNFT contract with a missing subprotocol name will end up paying an arbitrary fee in note tokens to the malicious user.

The impact of this issue is higher than a user simply putting in the wrong subprotocol name due to the following reasons:

1. If a user mistypes a subprotocol name (say a typo, or wrong capitalizations), it is likely the add() call will be reverted since a subprotocol probably doesn't exist with that name.
2. If a user types the name of the wrong protocol, the add() call is still likely to be reverted if the user doesn't hold the necessary nft address's necessary tokenId.
3. "" is the default value in a lot of tools. So if a particular tool fails to parse user input correctly, it might call the contract with "", making the chance of this error happening quite likely.

Since the impact is high (loss of note tokens), and the circumstances of it occurring are believable (missed input during contract call), this is classified as high severity.

Proof of Concept

The following test can be added to CidNFT.t.sol:

function testRegisterBlankName() public {
        address user = user1;
        note.mint(user2, 10000 * 1e18);
        (uint256 tokenId, uint256 subId, uint256 key) = prepareAddOne(
            user2,
            user2
        );

        vm.prank(user1);
        // Register blank name with a fee
        string memory name = "";
        subprotocolRegistry.register(
            true,
            true,
            true,
            address(sub1),
            name,
            1 ether
        );
        vm.stopPrank();

        vm.startPrank(user2);
        note.approve(address(cidNFT), type(uint256).max);
        uint256 initBal = note.balanceOf(user2);
        // User calls with blank field
        cidNFT.add(tokenId, "", key, subId, CidNFT.AssociationType.ORDERED);
        vm.stopPrank();
        uint256 finalBal = note.balanceOf(user2);

        assert(finalBal == initBal - 1 ether);
    }

This shows user1 can register empty string, and user2, when calling, loses note balance. In this POC the user2 is minted an nft, but the attacker user1 can simply create a bogus ERC721 contract which passes the supportsInterface check:

if (
            !ERC721(_nftAddress).supportsInterface(
                type(CidSubprotocolNFT).interfaceId
            )
        ) revert NotASubprotocolNFT(_nftAddress);

and always returns true in its safetransferFrom function, allowing the add() call to go through.

Tools Used

Foundry, Remix

Recommended Mitigation Steps

Add a simple sanity check during registration in SubprotocolRegistry.sol:

require(bytes(_name).length > 0);

[berndartmueller]

Known issue by the sponsor (https://github.com/code-423n4/2023-01-canto-identity#automated-findings--publicly-known-issues)

String confusions: Subprotocols are identified by a string and there are multiple ways to encode the same human-readable string. It is the responsibility of the developer that integrates with CID to ensure that he queries the correct string.

Even though it's a valid point, it requires the user to make a mistake (it's not a systematic issue - regardless of how the likelihood is inflated in this submission). Additionally, the severity of the issue is very much overinflated.

I will for now leave this submission open for the sponsor review, but I'm inclined to close this issue as "Out of scope".

[c4-sponsor]

OpenCoreCH marked the issue as sponsor disputed

[OpenCoreCH]

Agree that this also belongs to string confusions which were out of scope. I think it's important to mention that in practice, a user will rarely have a reason to type this string directly (unless he really wants to and knows what he is doing). Usually, there will be a frontend for a subprotocol that prepares the necessary call for the user. So this should be considered a developer interface, not a user-facing interface (which is also the reason that we do not perform any string sanitization).

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Out of scope