PhiNFT1155 contracts continue sending fees/royalties to old protocol destination address

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/art/PhiNFT1155.sol#L122

Vulnerability details

Summary

Context: When a PhiNFT1155.sol contract instance is created from the PhiFactory contract, the initialize() function is called to setup it. One of the parameters passed along is the protocolFeeDestination address, which is used as the default royalty recipient (see here) and the address that will receive the artCreate fees (see here).

Issue: The issue is that if the protocolFeeDestinationAddress contract is changed in the PhiFactory contract by the owner using function setProtocolFeeDestination(), the existing PhiNFT1155.sol contract instances created would still be using the old protocol fee destination address stored in them since it is not being dynamically retrieved from the PhiFactory contract. The protocolFeeDestination address could be updated for several reasons such as in case it is compromised, becomes malicious or there has been a decision made to make the address non-privileged.

Impact: Due to this, the old protocol fee destination address would still be receiving all the royalties (i.e. 500 bps = 5%) from all PhiNFT1155.sol contract instances. It would also be receiving all the artCreateFee amount of tokens when art is created through the createArtFromFactory() function.

Proof of Concept

1. First we'll take a look at the protocolFeeDestination address. As we can see below, once the protocolFeeDestination address is set through the initialize() function on Line 122, there is no way to update it to the new address or no way to dynamically retrieve the latest address from the PhiFactory contract.

File: PhiNFT1155.sol
43:     address public protocolFeeDestination;

... ...

File: PhiNFT1155.sol
095:     function initialize(
096:         uint256 credChainId_,
097:         uint256 credId_,
098:         string memory verificationType_,
099:         address protocolFeeDestination_
100:     )
101:         external
102:         initializer
103:     {
104:         ...

109:         initializeRoyalties(protocolFeeDestination_);
110: 
111:         ...

120:         phiFactoryContract = IPhiFactory(payable(msg.sender));
121: 
122:         protocolFeeDestination = phiFactoryContract.protocolFeeDestination(); 
123:         ...
126:     }

2. Now let's say the owner of the PhiFactory contract calls the setProtocolFeeDestination() function to update the protocolFeeDestination address in case the address is compromised, malicious or there has been a decision made to make the address non-privileged.

File: PhiFactory.sol
426:     function setProtocolFeeDestination(address protocolFeeDestination_)
427:         external
428:         nonZeroAddress(protocolFeeDestination_)
429:         onlyOwner
430:     {
431:         protocolFeeDestination = protocolFeeDestination_;
432:         emit ProtocolFeeDestinationSet(protocolFeeDestination_);
433:     }

3. Now although the address has changed, the PhiNFT1155.sol contracts, which inherit the CreatorRoyaltiesControl.sol contract still return the config.royaltyRecipient as the old address when royaltyInfo() function is called (which is used by external marketplaces). This is because when initializeRoyalties() was called, it permanently set the royaltyRecipient equal to the old protocol fee destination address. Since royaltyRecipient does not dynamically retrieve the latest value through the factory, the royalties are paid to the old address.

File: CreatorRoyaltiesControl.sol
17:     function initializeRoyalties(address _royaltyRecipient) internal {
18:         if (_royaltyRecipient == address(0)) revert InvalidRoyaltyRecipient();
19:         if (initilaized) revert AlreadyInitialized();
20:         royaltyRecipient = _royaltyRecipient;
21:         initilaized = true;
22:     }

... ...

43:     function royaltyInfo(
44:         uint256 tokenId,
45:         uint256 salePrice
46:     )
47:         public
48:         view
49:         returns (address receiver, uint256 royaltyAmount)
50:     {
51:         RoyaltyConfiguration memory config = getRoyalties(tokenId);
52:         royaltyAmount = (config.royaltyBPS * salePrice) / ROYALTY_BPS_TO_PERCENT;
53:         receiver = config.royaltyRecipient;
54:     }

4. Secondly, when multiple art creations occur through functions _createNewNFTContract() and _useExistingNFTContract() in the PhiFactory contract, it calls the createArtFromFactory() function in the respective PhiNFT1155.sol contracts. Over here as we can see below, the artCreateFee is still sent to the old protocolFeeDestination address.

File: PhiNFT1155.sol
139:     function createArtFromFactory(uint256 artId_) external payable onlyPhiFactory whenNotPaused returns (uint256) {
140:         ...
143: 
144:         uint256 artFee = phiFactoryContract.artCreateFee();
145: 
146:         protocolFeeDestination.safeTransferETH(artFee);
147:         
             ...
159:     }

5. To note, if the old protocolFeeDestination address is a contract implementation that turns out to be malicious (which could be one of the reasons why it is being replaced), it can revert in its fallback function to DOS art creations from factory for a credential i.e. credId. This impact is unlikely to occur since in most cases the fee destination would be an EOA. But the impacts mentioned in step 3 and 4 would be troublesome since all the existing PhiNFT1155.sol contracts would still be sending all the royalties and art creation fees to it.

Tools Used

Manual Review Review

Recommended Mitigation Steps

To solve the issue in CreatorRoyaltiesControl, consider retrieving the default royalty recipient variable dynamically from the phiFactory contract. The same would also need to be applied to the protocolFeeDestination address currently stored in the PhiNFT1155.sol contract.

Using dynamic retrieval from the factory would solve the issue for all PhiNFT1155.sol contract instances that were created.

Assessed type

Error

[ZaK3939]

This is a good observation. I appreciate it.

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report

[mcgrathcoutinho]

Hi @fatherGoose1,

Could you please re-consider this issue and its dups for High-severity? It seems like the impact is rather suited as a high-severity bug since the protocol would be losing all the fees and royalties from all the PhiNFT1155.sol instances ever created as well as considering that the accumulated amount would become large over time.

Thank you for your time!

[bronzepickaxe]

Hi,

This has been correctly identified as a Medium severity issue. The preconditions are very unlikely - project changing their protocolFeeDestination address is extremely unlikely. Low likelihood + High impact = Medium severity.

Thanks!

[mcgrathcoutinho]

Hey @bronzepickaxe,

C4 has their own severity categorization rules when estimating risk. Please do not use different severity categorization rules here that are used outside of C4.

Acc to the High-severity definition, if assets can be lost or compromised, it is to be treated as a high-severity issue. 3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

My first comment was directed towards the judge, so please refrain from trying to negate my comments and let the judge make up their mind. Thank you!

[fatherGoose1]

I agree that this issue has high impact, but requires that the fee destination address be changed, which is rather unlikely in practice. Even in the case of a fee address update, the older fee address may still be functional and the desired location for the older fee accrual. Sticking with medium for these reasons.