c4-bot-7 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/main/src/PhiFactory.sol#L617 https://github.com/code-423n4/2024-08-phi/blob/main/src/PhiFactory.sol#L365

Vulnerability details

Impact

The credMerkleRoot[credChainId][credId] can be overwritten, potentially erasing the previous credMerkleRoot for art creators with the same credChainId and credId. If those former creators utilized the MERKLE type, they would be unable to claim.

Proof of Concept

Art creators utilize the createArt function in the PhiFactory contract to generate new Art. Based on the credChainId and credId provided, either a new PhiNFT1155 contract is created, or an existing contract is reused. During the retrieval of the art contract in the createERC1155Internal function, new art data is initialized through the _initializePhiArt function, where credMerkleRoot[credChainId][credId] is set up.

    function _initializePhiArt(PhiArt storage art, ERC1155Data memory createData_) private {
        (
            uint256 credId,
            address credCreator,
            string memory verificationType,
            uint256 credChainId,
            bytes32 merkleRootHash
        ) = abi.decode(createData_.credData, (uint256, address, string, uint256, bytes32));

        ...

>       credMerkleRoot[credChainId][credId] = merkleRootHash;
    }

Here, we face an issue for art creators using the MERKLE type. The credMerkleRoot[credChainId][credId] might already be initialized by a previous creator with the same credChainId and credId. Without a check for an existing value, it is overwritten with the new merkleRootHash regardless of whether it matches. If overwritten with a different hash, it poses a problem for the previous creator, as credMerkleRoot[credChainId][credId] is crucial for verifying the merkleProof during NFT claims in the merkleClaim function.

    function merkleClaim(
        bytes32[] calldata proof_,
        bytes calldata encodeData_,
        MintArgs calldata mintArgs_,
        bytes32 leafPart_
    )
        external
        payable
        whenNotPaused
    {
        (address minter_, address ref_, uint256 artId_) = abi.decode(encodeData_, (address, address, uint256));
        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();
        }
        ...
    }

Without its original root hash, the function will revert with an InvalidMerkleProof error.

Here's a PoC to prove the claiming fails. Full script can be found here.

    function test_claimMerkleFailDueToRootOverwrite() public {
        // create art 1
        //  credId = 1
        //  credChainId = 31_337
        //  rootHash = 0xe70e719557c28ce2f2f3545d64c633728d70fbcfe6ae3db5fa01420573e0f34b
        bytes32 expectedRoot = 0xe70e719557c28ce2f2f3545d64c633728d70fbcfe6ae3db5fa01420573e0f34b;
        bytes memory credData = abi.encode(1, owner, "MERKLE", 31_337, expectedRoot);
        bytes memory signCreateData = abi.encode(expiresIn, ART_ID2_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)
        );

        // create art 2
        //  credId = 1
        //  credChainId = 31_337
        //  rootHash = 0x0000000000000000000000000000000003c2f7086aed236c807a1b5000000000 <- any different proof (just an example, this should be a correct calculated hash)
        expectedRoot = 0x0000000000000000000000000000000003c2f7086aed236c807a1b5000000000;
        credData = abi.encode(1, owner, "MERKLE", 31_337, expectedRoot);
        signCreateData = abi.encode(expiresIn, ART_ID2_URL_STRING, credData);
        createMsgHash = keccak256(signCreateData);
        createDigest = ECDSA.toEthSignedMessageHash(createMsgHash);
        (cv, cr, 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)
        );

        // try claiming art 1
        // this will revert with InvalidMerkleProof
        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, referrer, uint256(2), value, IMAGE_URL2);

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

        vm.startPrank(participant);
        vm.expectRevert(IPhiFactory.InvalidMerkleProof.selector);
        phiFactory.claim{ value: totalMintFee }(dataCompressed);
    }

Run the test by

forge test --mt test_claimMerkleFailDueToRootOverwrite -vvv

Tools Used

Manual Review

Recommended Mitigation Steps

Verify if credMerkleRoot[credChainId][credId] already exists and matches the current value if previously set.

    function _initializePhiArt(PhiArt storage art, ERC1155Data memory createData_) private {
        (
            uint256 credId,
            address credCreator,
            string memory verificationType,
            uint256 credChainId,
            bytes32 merkleRootHash
        ) = abi.decode(createData_.credData, (uint256, address, string, uint256, bytes32));

        ...

+       if(credMerkleRoot[credChainId][credId] != bytes32(0) && credMerkleRoot[credChainId][credId] != merkleRootHash){
+          revert InvalidMerkleRootHash();
+       }
        credMerkleRoot[credChainId][credId] = merkleRootHash;
    }

Assessed type

Error

fatherGoose1 commented 2 months ago

All data is signed by phiSignerAddress

c4-judge commented 2 months ago

fatherGoose1 marked the issue as unsatisfactory: Invalid

KupiaSecAdmin commented 2 months ago

@fatherGoose1 Above all, the createArt function is accessible to anyone wishing to create their own art pieces. While all data is indeed signed by the phiSignerAddress, individuals can potentially attach their own signatures and merkle proofs to go through the art creation process - which results in above issue. This vulnerability was likely demonstrated in the previous proof of concept (PoC). Malicious actors can exploit this to invalidate the minting capability of existing artworks by overwriting the merkle proof.

This poses a risk of financial loss to artists who have paid art creation fees and invested in their artworks. I strongly recommend that the protocol be designed to prevent such actions, safeguarding users' rights to claim and protecting them from potential losses, provided it's feasible.

Thank you for reconsidering this matter!

fatherGoose1 commented 2 months ago

But the phiSignerAddress would need to sign twice, with cred datas:

credData = abi.encode(1, owner, "MERKLE", 31_337, expectedRoot1); AND credData = abi.encode(1, owner, "MERKLE", 31_337, expectedRoot2);

I'm unsure what you mean by individuals can potentially attach their own signatures and merkle proofs to go through the art creation process. They can only attach what the PhiSigner signs. Could you please clarify?

KupiaSecAdmin commented 1 month ago

Sure,

Individuals (not particularly an attacker) may use same credId and credChainId to create their own arts. Since this request comes legitimate, they can have their own expectedRoot signed with which they intend to verify claims.

For those NFT contracts, a check is in place to re-use already created contract in createERC1155Internal, but _initializePhiArt doesn't check such conditions.

fatherGoose1 commented 1 month ago

This would be unlikely to occur, but I agree it should be checked. QA

c4-judge commented 1 month ago

fatherGoose1 changed the severity to QA (Quality Assurance)