code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

TLDR; contracts without ERC721TokenReceiver can add to the CidNFT but not remove.

If an address is a contract without onERC721Received, they can receive NFTs using standard transferFrom calls. This allows them to then call the CidNFT contract and successfully add the subprotocol NFT. safeTransferFrom is used in the add function, however that performs checks on the NFT recipient only - so it does not make any assertions about the msg.sender on add.

Now, the contract which added a subprotocol NFT can never call remove. When a remove is attempted, safeTransferFrom requires that the recipient implements onERC721Received if it is a contract address - without that, the transfer will revert. This may cause the subprotocol NFT to be stuck in the CidNFT contract.

If the type is primary then they can not change their association because this will attempt a remove first. For ordered and active, they can only add new associations.

Risk: This seems to fall under the definition of high "Assets can be ~~stolen/~~lost~~/compromised~~ directly" since once the subprotocol NFT has been successfully added in this scenario, they may not be able to recover that asset. However this is mitigated by the fact that remove is callable cidNFTOwner approved addresses as well, assuming that address is an EOA or a contract with that capability then approvals could be leveraged to allow an alternate caller to remove on their behalf.

Proof of Concept

Create a mock contract that interacts with the cidNFT contract:

contract MockNonReceiver {
    CidNFT public cidNFT;

    constructor(CidNFT _cidNFT, ERC721 sub1) {
        cidNFT = _cidNFT;
        // Approve transfers to CidNFT to enable add
        sub1.setApprovalForAll(address(_cidNFT), true);
    }

    function add(
        uint256 _cidNFTID,
        string calldata _subprotocolName,
        uint256 _key,
        uint256 _nftIDToAdd,
        CidNFT.AssociationType _type
    ) external {
        cidNFT.add(_cidNFTID, _subprotocolName, _key, _nftIDToAdd, _type);
    }

    function remove(
        uint256 _cidNFTID,
        string calldata _subprotocolName,
        uint256 _key,
        uint256 _nftIDToRemove,
        CidNFT.AssociationType _type
    ) external {
        cidNFT.remove(_cidNFTID, _subprotocolName, _key, _nftIDToRemove, _type);
    }
}

Then in CidNFT.t.sol add the following test, modeled after testAddRemoveByOwner: ( // @audit comments are used to highlight where this new test differs from the original)

function testAddRemoveByNonReceivableContract() public {
    (uint256 tokenId, uint256 sub1Id, uint256 key1) = prepareAddOne(address(this), address(this)); 

    // @audit Create test contract and transfer mint tokens into it
    MockNonReceiver mock = new MockNonReceiver(cidNFT, sub1);
    cidNFT.transferFrom(address(this), address(mock), tokenId); 
    sub1.transferFrom(address(this), address(mock), sub1Id);

    // add by owner
    vm.expectEmit(true, true, true, true);
    emit OrderedDataAdded(tokenId, "sub1", key1, sub1Id);
    mock.add(tokenId, "sub1", key1, sub1Id, CidNFT.AssociationType.ORDERED); // @audit changed contract called to the mock

    // confirm data
    assertEq(cidNFT.getOrderedData(tokenId, "sub1", key1), sub1Id);

    // remove by owner
    vm.expectEmit(true, true, true, true);
    emit OrderedDataRemoved(tokenId, "sub1", key1, sub1Id);
    // @audit This line fails with `EvmError: Revert`!
    mock.remove(tokenId, "sub1", key1, sub1Id, CidNFT.AssociationType.ORDERED); // @audit changed contract called to the mock

    // @audit If you apply the rec below then all tests pass, including this new one.
}

Tools Used

forge test

Recommended Mitigation Steps

Don't use safeTransferFrom in remove, instead use the standard transferFrom. In this scenario the msg.sender was previously in possession of the NFT in order to add it in the first place. It may be safe to assume that they could handle having it sent right back to them - and likely a better outcome than having the NFT become trapped like it is currently. Not using safeTransferFrom would also save a little gas on the remove scenario.

This is similar to the assumption made in the mint function -- "If a contract calls this method, he expects to get an NFT back" == if a contract calls the remove method, they expect to get an NFT back.

However this could lead to assets getting stuck in the case of a non-receivable approved caller removing subprotocol NFTs. So a safer alt may be to keep safeTransferFrom in the remove function but add a param allowing the caller to specify the NFT recipient address.

OpenCoreCH commented 1 year ago

Valid point, I'm not sure about the severity. In my opinion, this is ultimately a user/integrator error, because someone calls this function and does not adhere to the specifications properly. We can do our best to avoid these errors, but as the warden mentions, there is not really a perfect solution. Using transferFrom may lead to situations where the NFT is trapped within the receiver and I am not really sure if that is better. This issue here is usually recoverable (you can transfer the CID NFT to a different address that can receive NFTs, at least if the contract that currently owns it has a functionality to transfer it), whereas a trapped subprotocol NFT is not recoverable. A configurable receiver address could potentially help, but what if that address cannot receive NFTs? Of course that would also be a user/integrator error, but if someone does not adhere to the specs properly with the current design, he also might not do with this updated design and just pass e.g. address(this) as the NFT receiver.

Ultimately, I'm leaning towards QA because the issue is often recoverable (by transferring), caused by a user error, and not completely avoidable.