Users will lose funds when they call claim() and send bigger msg.value than required

[howlbot-integration]

Lines of code

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

Vulnerability details

When users call the claim() function in Cred.sol, they are required to pay a mintFee, which is determined by the artId and quantity through the getArtMintFee() function:

return IPhiRewards(phiRewardsAddress).computeMintReward(quantity_, arts[artId_].mintFee)
+ quantity_ * mintProtocolFee;
}

This function first calculates the reward value using computeMintReward(), and then adds quantity_ * mintProtocolFee to determine the total mintFee. This total is the amount users must pay when executing the claim() function.

The calculated mintFee is passed as msg.value to the merkleClaim() function, or to signatureClaim() depending on art.verificationType. Both of these functions ultimately call _processClaim() with msg.value as one of its parameters. _processClaim() attempts to handle any surplus by recalculating the mintFee. The issue arises because claim() initially accepts msg.value, but only passes the mintFee to merkleClaim() or signatureClaim(). As a result, the following check in processClaim() will always result in zero since msg.value and mintFee are the same:

if ((etherValue - mintFee) > 0) {
msgSender().safeTransferETH(etherValue - mintFee);
} 

Impact

Users will be not able to get refund amount if mintFee is less than msg.value that was sent. Mark as high, because anytime when msg.value is not exact same as mintFee, users will lose the funds from the difference between msg.value and mintFee.

Proof of Concept

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L264-L305
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

Consider the following scenario:

1. Alice calls claim() with msg.value set to 100.
2. The claim() function calculates mintFee as 40.
3. claim() passes the mintFee (40) to merkleClaim().
4. merkleClaim() processes the 40 and calls _processClaim() with 40 as the final parameter.
5. _processClaim() tries to handle any surplus by recalculating mintFee, but the comparison between mintFee and msg.value results in zero.

As a result, Alice loses the surplus 60.
Keep in mind that the same scenario is possible not only with merkleClaim(), but also with signatureClaim().

Tools Used

Manual review

Recommended Mitigation Steps

Implement a check within claim() to refund any redundant amount that exceeds the calculated mintFee.

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