Potential Loss of User Funds Due to Lack of Overpayment Refund in Individual Claim Functions

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The individual claim functions (claim and its internal calls to signatureClaim and merkleClaim) in the PhiFactory contract do not properly handle overpayments, potentially leading to users losing excess ETH. When users send more ETH than the required mint fee, the excess amount remains trapped in the contract without a mechanism for refund. This issue could result in cumulative loss of funds for users who consistently overpay slightly.
In contrast, the batchClaim function does accurately refund excess fees, creating an inconsistency in the contract's behavior.

Proof of Concept

In the claim function, only the exact mintFee is passed to internal claim functions:

uint256 mintFee = getArtMintFee(artId, quantity_);
this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs);

The signatureClaim function receives only this exact mintFee:

function signatureClaim(
    bytes calldata signature_,
    bytes calldata encodeData_,
    MintArgs calldata mintArgs_
)
    external
    payable
    whenNotPaused
{
    // ...
    _processClaim(artId_, minter_, ref_, verifier_, mintArgs_.quantity, data_, mintArgs_.imageURI, msg.value);
    // ...
}

The _processClaim function's refund mechanism never triggers for individual claims:

uint256 mintFee = getArtMintFee(artId_, quantity_);
if ((etherValue_ - mintFee) > 0) {
    _msgSender().safeTransferETH(etherValue_ - mintFee);
}

In contrast, the batchClaim function correctly handles overpayments by passing individual ETH values and also checking it matches msg.value

if (msg.value != totalEthValue) revert InvalidEthValue();

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the claim function to pass the full msg.value to internal claim functions:

Assessed type

Token-Transfer

[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