When PhiFactory.claim function is called with a msg.value that is more than total mint fee, the difference between such msg.value and total mint fee cannot be refunded to caller of such function

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/3a817c9dedca53ea27ff3e7988f8389086935b8b/src/PhiFactory.sol#L723-L758
https://github.com/code-423n4/2024-08-phi/blob/3a817c9dedca53ea27ff3e7988f8389086935b8b/src/PhiFactory.sol#L264-L304

Vulnerability details

Impact

As a part of the refund mechanisms throughout the protocol, the PhiFactory._processClaim function, which is eventually called by the PhiFactory.claim function, would execute uint256 mintFee = getArtMintFee(artId_, quantity_) and if ((etherValue_ - mintFee) > 0) { _msgSender().safeTransferETH(etherValue_ - mintFee); } for handling a refund. Also, https://docs.philand.xyz/explore-phi/developers/architecture#example-flow-for-paid-mint-with-referral states that a normal workflow would include that User calls ``claim`` function with sufficient ETH to cover the total mint fee. Therefore, the caller of the PhiFactory.claim function should expect a refund of the difference between the msg.value and total mint fee when calling such function with a msg.value that is more than the total mint fee.

https://github.com/code-423n4/2024-08-phi/blob/3a817c9dedca53ea27ff3e7988f8389086935b8b/src/PhiFactory.sol#L723-L758

    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_);

        (bool success_,) = art.artAddress.call{ value: mintFee - mintProtocolFee * quantity_ }(
            abi.encodeWithSignature(
                "claimFromFactory(uint256,address,address,address,uint256,bytes32,string)",
                artId_,
                minter_,
                ref_,
                verifier_,
                quantity_,
                data_,
                imageURI_
            )
        );

        if (!success_) revert ClaimFailed();
    }

However, the PhiFactory.claim function executes this.merkleClaim{ value: mintFee }(proof, claimData, mintArgs, leafPart_) if art.verificationType is MERKLE or this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs) if art.verificationType is SIGNATURE, where the mintFee is the total mint fee that is returned by getArtMintFee(artId, quantity_). Since only the mintFee is sent when calling the PhiFactory.merkleClaim or PhiFactory.signatureClaim function, the difference between the msg.value and total mint fee would remain in the PhiFactory contract and not be refunded to the caller of the PhiFactory.claim function when the PhiFactory.claim function is called with such msg.value that is more than the total mint fee. As a result, such caller of the PhiFactory.claim function loses such ETH difference that should be refunded to him.

https://github.com/code-423n4/2024-08-phi/blob/3a817c9dedca53ea27ff3e7988f8389086935b8b/src/PhiFactory.sol#L264-L304

    function claim(bytes calldata encodeData_) external payable {
        bytes memory encodedata_ = LibZip.cdDecompress(encodeData_);
        (uint256 artId) = abi.decode(encodedata_, (uint256));
        PhiArt storage art = arts[artId];
        uint256 tokenId = IPhiNFT1155Ownable(art.artAddress).getTokenIdFromFactoryArtId(artId);
        if (art.verificationType.eq("MERKLE")) {
            (
                ,
                address minter_,
                bytes32[] memory proof,
                address ref_,
                uint256 quantity_,
                bytes32 leafPart_,
                string memory imageURI_
            ) = abi.decode(encodedata_, (uint256, address, bytes32[], address, uint256, bytes32, string));

            bytes memory claimData = abi.encode(minter_, ref_, artId);
            MintArgs memory mintArgs = MintArgs({ tokenId: tokenId, quantity: quantity_, imageURI: imageURI_ });
@>          uint256 mintFee = getArtMintFee(artId, quantity_);
@>          this.merkleClaim{ value: mintFee }(proof, claimData, mintArgs, leafPart_);
        } else if (art.verificationType.eq("SIGNATURE")) {
            (
                ,
                address minter_,
                address ref_,
                address verifier_,
                uint256 expiresIn_,
                uint256 quantity_,
                bytes32 data_,
                string memory imageURI_,
                bytes memory signature_
            ) = abi.decode(encodedata_, (uint256, address, address, address, uint256, uint256, bytes32, string, bytes));

            bytes memory claimData = abi.encode(expiresIn_, minter_, ref_, verifier_, artId, block.chainid, data_);
            MintArgs memory mintArgs = MintArgs({ tokenId: tokenId, quantity: quantity_, imageURI: imageURI_ });
@>          uint256 mintFee = getArtMintFee(artId, quantity_);
@>          this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs);
        } else {
            revert InvalidVerificationType();
        }
    }

Proof of Concept

Please add the following test in test\PhiFactory.t.sol. This test will pass to demonstrate the described scenario.

    function test_phiFactoryClaimFunctionCannotIssueRefund() public {
        _createArt(ART_ID_URL_STRING);

        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)
        );

        bytes32[] memory proof = new bytes32[](2);
        proof[0] = 0x0927f012522ebd33191e00fe62c11db25288016345e12e6b63709bb618d777d4;
        proof[1] = 0xdd05ddd79adc5569806124d3c5d8151b75bc81032a0ea21d4cd74fd964947bf5;
        address to = 0x1111111111111111111111111111111111111111;
        bytes32 value = 0x0000000000000000000000000000000003c2f7086aed236c807a1b5000000000;
        uint256 artId = 2;
        bytes memory data = abi.encode(artId, to, proof, referrer, uint256(2), value, IMAGE_URL2);

        bytes memory dataCompressed = LibZip.cdCompress(data);

        vm.deal(participant, 2 ether);

        vm.startPrank(participant);

        // phiFactory contract does not hold any ETH at this moment
        assertEq(address(phiFactory).balance, 0);

        // participant sends a msg.value that is more than total mint fee when calling claim function
        phiFactory.claim{ value: 2 ether }(dataCompressed);

        // difference between participant's msg.value and total mint fee remains in phiFactory contract
        assertGt(address(phiFactory).balance, 1.9 ether);

        // participant, who calls claim function, cannot receive any refund at all
        assertEq(participant.balance, 0);

        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The PhiFactory.claim function can be updated to send the difference between the msg.value and total mint fee to the function caller when such function is called with a msg.value that is more than the total mint fee.

Assessed type

Other

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as not a duplicate

[c4-judge]

fatherGoose1 marked the issue as duplicate of #109