Using the `this` keyword to pass `mintFee` ETh value in the `claim(...)` function causes loss of excess ETH

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L283
https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L300

Vulnerability details

Impact

The ETH refund in _processClaim(...) function becomes useless because of the this keyword used to pass ETH in the claim(...) function.
This causes users to lose excess ETH.

Proof of Concept

Excess ETh used in calling the claim(...) function will not be refunded.

It is clear that the ETh refund is implemented in the _processClaim(...) function which is called by the merkleClaim(...) and signatureClaim(...) functions that are called in the this claim(...) function.

However, the issue is that using the this keyword to pass ETh value to the merkleClaim(...) and signatureClaim(...) functions changes the context and also the msg.value in both the merkleClaim(...) and signatureClaim(...) to a new one which is the mintFee. This means that the original msg.value passed by the user to the claim(...) function is not the same to the one passed to this.merkleClaim{ value: mintFee }(...). As we can see the mintFee is now the msg.value in the merkleClaim function. So excess ETH amount is not passed is not passed down to merkleClaim function for the _processClaim(...) function to do the excess ETH refund. This causes the user to lose excess ETH.

File: PhiFactory.sol
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_);//FINDING: the this keyword causes loss of excess ETH
        } 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);//FINDING: the this keyword causes loss of excess ETH
        } else {
            revert InvalidVerificationType();
        }
    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the this keyword since the functions are all in the PhiFactory.sol like this

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_);
++          merkleClaim(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);
++          signatureClaim(signature_, claimData, mintArgs);
        } else {
            revert InvalidVerificationType();
        }
    }

Assessed type

ETH-Transfer