Canto Identity Protocol contest - gzeon's results

Lines of code

https://github.com/code-423n4/2023-01-canto-identity/blob/dff8e74c54471f5f3b84c217848234d474477d82/src/CidNFT.sol#L178-L182

Vulnerability details

Impact

CidNFT.mint(bytes[]) allow user to mint and add subprotocol NFTs directly after minting. The _addList args to the add call include the _cidNFTID param, which can change if there are other mint before the user's transaction. Additionally, CidNFT.add only check if the cid nft is owned by msg.sender, OR if the msg.sender is approved to to cidnft; Thus, an attacker can front run the mint and add call, approve the victim to his newly mint cid nft, let the victim tx to be executed and therefore assigning his subprotocol nft to the attacker's cid nft, and revoke the approval.

Unlike the DOS attack, this is high risk because asset can be stolen.

Proof of Concept

1. Bob want to mint cid NFT with id = 1, and add subprotocol DOGE as PRIMARY entry in a single call using CidNFT.mint(bytes[])
2. Alice frontrun bob to mint NFT with id = 1
3. Alice approve Bob to use the NFT
4. Bob's transaction in (1) is executed, but cid nft id = 2 is minted, however, the add call still have cidnftid = 1
5. Bob's subprotocol DOGE nft is added to Alice's NFT
6. Alice revoke Bob's approval to his cid NFT
7. Alice call remove and stole Bob's subprotocol NFT

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

    function mint(bytes[] calldata _addList) external {
        _mint(msg.sender, ++numMinted); // We do not use _safeMint here on purpose. If a contract calls this method, he expects to get an NFT back
        bytes4 addSelector = this.add.selector;
        for (uint256 i = 0; i < _addList.length; ++i) {
            (
                bool success, /*bytes memory result*/

            ) = address(this).delegatecall(abi.encodePacked(addSelector, _addList[i]));
            if (!success) revert AddCallAfterMintingFailed(i);
        }
    }

https://github.com/code-423n4/2023-01-canto-identity/blob/dff8e74c54471f5f3b84c217848234d474477d82/src/CidNFT.sol#L178-L182

        if (
            cidNFTOwner != msg.sender &&
            getApproved[_cidNFTID] != msg.sender &&
            !isApprovedForAll[cidNFTOwner][msg.sender]
        ) revert NotAuthorizedForCIDNFT(msg.sender, _cidNFTID, cidNFTOwner);

Recommended Mitigation Steps

Instead of a self delegate call, use a switch statement with abi.decode to support different type of entry, and call the internal function according with the newly generated cid nft id.

Lines of code

https://github.com/code-423n4/2023-01-canto-identity/blob/dff8e74c54471f5f3b84c217848234d474477d82/src/CidNFT.sol#L143-L157

Vulnerability details

Impact

CidNFT.mint(bytes[]) allow user to mint and add subprotocol NFTs directly after minting. The _addList args to the add call include the _cidNFTID param, which can change if there are other mint before the user's transaction.

Proof of Concept

An attacker can DOS a user's mint and add by front-running their mint by another mint, bumping the cid nft id.

https://github.com/code-423n4/2023-01-canto-identity/blob/dff8e74c54471f5f3b84c217848234d474477d82/src/CidNFT.sol#L143-L157

    /// @notice Mint a new CID NFT
    /// @dev An address can mint multiple CID NFTs, but it can only set one as associated with it in the AddressRegistry
    /// @param _addList An optional list of encoded parameters for add to add subprotocol NFTs directly after minting.
    /// The parameters should not include the function selector itself, the function select for add is always prepended.
    function mint(bytes[] calldata _addList) external {
        _mint(msg.sender, ++numMinted); // We do not use _safeMint here on purpose. If a contract calls this method, he expects to get an NFT back
        bytes4 addSelector = this.add.selector;
        for (uint256 i = 0; i < _addList.length; ++i) {
            (
                bool success, /*bytes memory result*/

            ) = address(this).delegatecall(abi.encodePacked(addSelector, _addList[i]));
            if (!success) revert AddCallAfterMintingFailed(i);
        }
    }

Recommended Mitigation Steps

Instead of a self delegate call, use a switch statement with abi.decode to support different type of entry, and call the internal function according with the newly generated cid nft id.