Incorrect msg.value handling in claim function prevents ETH refunds

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L264-L304
https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L327-L346
https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L352-L383
https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L723-L758

Vulnerability details

In the claim function, when a user wants to claim art and transfers ETH, the function internally calls either this.merkleClaim or this.signatureClaim, passing the mintFee as the value. However, because these calls use the this keyword, they are treated as external calls. This causes msg.value to reset, meaning that in the signatureClaim and _processClaim functions, msg.value will only equal the mintFee. As a result, when the refund is calculated in the _processClaim function, it is always zero, since etherValue_ always equals mintFee. Consequently, users are unable to receive refunds for any excess ETH sent with their claim.

Impact

This issue prevents users from receiving refunds for any ETH they send beyond the required mintFee. If a user mistakenly sends more ETH than required, they will lose the excess amount because the contract incorrectly calculates the refund as zero. This could lead to significant financial losses for users.

Proof of Concept

Add following logs into the PhiFactory

These two into the claim() function

...
uint256 mintFee = getArtMintFee(artId, quantity_);
console2.log("claim msg.value >>>>>>>>>>>>>>>>>>>>>>>>", msg.value);
console2.log("claim >>>>>>>>>>>>>>>>>>>>>>>>", mintFee);
this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs);
...

This one into the signatureClaim function

...
console2.log("signatureClaim >>>>>>>>>>>>>>>>>>>>>>>>", msg.value);
(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();
...

This one into the _processClaim function

...
PhiArt storage art = arts[artId_];
console2.log("_processClaim >>>>>>>>>>>>>>>>>>>>>>>>", msg.value);
// Handle refund
uint256 mintFee = getArtMintFee(artId_, quantity_);
if ((etherValue_ - mintFee) > 0) {
    _msgSender().safeTransferETH(etherValue_ - mintFee);
}
...

Add this code to the PhiFactory.t.sol file and run the following command:
forge test -vvv --match-test POC:

    function test_POC() 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);
        totalMintFee +=  1e8;
        console2.log(">>>>>>>>>>>>>>>>>>>>>>>>", totalMintFee);


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

Here logs for all functions in the transaction

Logs:
  >>>>>>>>>>>>>>>>>>>>>>>> 10450000100000000
  claim msg.value >>>>>>>>>>>>>>>>>>>>>>>> 10450000100000000
  claim >>>>>>>>>>>>>>>>>>>>>>>> 10450000000000000
  signatureClaim >>>>>>>>>>>>>>>>>>>>>>>> 10450000000000000
  _processClaim >>>>>>>>>>>>>>>>>>>>>>>> 10450000000000000

Tools Used

Manual Review
Forge

Recommended Mitigation Steps

To correct this issue, the internal function calls should avoid using the this keyword, ensuring that msg.value remains intact throughout the transaction. Alternatively, the mintFee should be passed and handled separately without altering the original msg.value. This will allow the contract to correctly calculate and refund any excess ETH sent by the user during the claim process.

Assessed type

ETH-Transfer

[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