Fee recipient addresses can't be changed in the protocol

[code423n4]

Lines of code

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

Vulnerability details

The CID protocol contains fees at two different levels, one is defined at the subprotocol level by the owner of the registered subprotocol and the other at the CID protocol level in the CidNFT and SubprotocolRegistry contracts.

In all cases, the address that collects the fees is immutable and can't be changed:

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

address public immutable cidFeeWallet;

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

address public immutable cidFeeWallet;

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

struct SubprotocolData {
    /// @notice Owner (registrant) of the subprotocol
    address owner;
    /// @notice Optional cost in NOTE to add an NFT
    /// @dev Maximum value is (2^96 - 1) / 10^18 =~ 80 billion. Zero for no fee
    uint96 fee;
    address nftAddress;
    bool ordered;
    bool primary;
    bool active;
}

Impact

If any of these addresses becomes unavailable or gets compromised, fees will still be sent to those addresses and funds will be lost, as these can't be updated.

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

if (subprotocolFee != 0) {
    uint256 cidFee = (subprotocolFee * CID_FEE_BPS) / 10_000;
    SafeTransferLib.safeTransferFrom(note, msg.sender, cidFeeWallet, cidFee);
    SafeTransferLib.safeTransferFrom(note, msg.sender, subprotocolOwner, subprotocolFee - cidFee);
}

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

SafeTransferLib.safeTransferFrom(note, msg.sender, cidFeeWallet, REGISTER_FEE);

Recommendation

Allow protocol owners to update the cidFeeWallet in the CidNFT and SubprotocolRegistry contract. Allow subprotocol owners to define and update a fee recipient address in their registered subprotocols.

[c4-judge]

berndartmueller marked the issue as primary issue

[c4-sponsor]

OpenCoreCH marked the issue as disagree with severity

[OpenCoreCH]

Might be something that we consider, but cidFeeWallet will be a multi-sig in practice and CID currently has no owner. So we would need to introduce an owner and if that is compromised, funds are also lost.

Design-related improvement suggestion, so I would say QA

[berndartmueller]

I agree with the sponsor on the downgrade to QA (Low)

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-b