Refunds are not correctly handled and would be locked in PhiNFT1155.sol contract instances and PhiFactory

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/abstract/Claimable.sol#L22

Vulnerability details

Impact

All refunds for signature/merkle claim calls occurring through the PhiNFT1155.sol contract instances are not sent to the msg.sender but would be permanently locked in the PhiNFT1155.sol contract instances themselves.

The same issue is also present in the PhiFactory.sol contract. When claims are made using the claim() function, the excess would be locked in the PhiFactory contract.

Proof of Concept

1. The PhiNFT1155.sol contracts inherit the Claimable.sol contracts, which have the signatureClaim() and merkleClaim() functions. Let's say users use these functions to claim their tokens. These functions call the signatureClaim() and merkleClaim() functions on the phiFactory contract.

File: Claimable.sol
23:     function signatureClaim() external payable {
24:         ...
40:         ...
42: 
43:         IPhiFactory phiFactoryContract = getPhiFactoryContract();
44:         IPhiFactory.MintArgs memory mintArgs_ = IPhiFactory.MintArgs(tokenId_, quantity_, imageURI_);
45:         phiFactoryContract.signatureClaim{ value: msg.value }(signature, claimData_, mintArgs_);
46:     }
47: 
48:     /// @notice Processes a merkle claim.
49:     function merkleClaim() external payable {
50:         ...
64: 
65:         IPhiFactory.MintArgs memory mintArgs = IPhiFactory.MintArgs(tokenId, quantity, imageURI);
66:         phiFactory.merkleClaim{ value: msg.value }(proof, claimData, mintArgs, leafPart);
67:     }

2. These functions signatureClaim() and merkleClaim() call the internal functions _validateAndUpdateClaimState() and _processClaim(). in the functions, we observe the following:

• In _validateAndUpdateClaimState(), we pass the condition since msg.sender is the artAddress
• In _processClaim(), the refund is sent to the msg.sender. But we just saw in the above point that the msg.sender is the artAddress.
• Due to this, the refunds are sent to the PhiNFT1155.sol contracts, where there is no logic further to refund the actual msg.sender.

File: PhiFactory.sol
717:     function _validateAndUpdateClaimState(uint256 artId_, address minter_, uint256 quantity_) private {
718:         PhiArt storage art = arts[artId_];
719: 
720:         // Common validations
721:         if (tx.origin != _msgSender() && msg.sender != art.artAddress && msg.sender != address(this)) {
722:             revert TxOriginMismatch();
723:         }
724:         ...
736:     }


File: PhiFactory.sol
738:     function _processClaim(
739:         uint256 artId_,
740:         address minter_,
741:         address ref_,
742:         address verifier_,
743:         uint256 quantity_,
744:         bytes32 data_,
745:         string memory imageURI_,
746:         uint256 etherValue_
747:     )
748:         private
749:     {
750:         PhiArt storage art = arts[artId_];
751: 
752:         // Handle refund
753:         uint256 mintFee = getArtMintFee(artId_, quantity_);
754:         
755:         if ((etherValue_ - mintFee) > 0) {
756:             _msgSender().safeTransferETH(etherValue_ - mintFee);
757:         }
758:         ...
774:     }

3. Similarly when users claim using the claim() function in the PhiFactory contract, the signatureClaim() and merkleClaim() functions are called using the this keyword. This makes a call to the contract itself i.e. it becomes the msg.sender. This is proved by the validation function _validateAndUpdateClaimState() in step 2 where it checks the msg.sender to be address(this).

File: PhiFactory.sol
269:     function claim(bytes calldata encodeData_) external payable {
270:             ...
290:             this.merkleClaim{ value: mintFee }(proof, claimData, mintArgs, leafPart_);
291:         } else if (art.verificationType.eq("SIGNATURE")) {
292:             ...
308:             this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs);
309:         } else {
310:             revert InvalidVerificationType();
311:         }
312:     }

4. Now we know that mintFee amount of tokens are sent forward to the merkleClaim() and signatureClaim() functions as part of a new context. In the new context, the msg.value is the mintFee. But in the original context, the remaining tokens i.e. msg.value - mintFee are not refunded. For example, if the msg.value = 1e18 as an example and mintFee = 1e15, 1e18 - 1e15 remain in the phiFactory contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider refunding msg.value - mintFee through the claim() function before making a call using the this keyword. For the phiNFT1155.sol contracts, check pre balance and post balance of the contract and refund the difference to the user.

Assessed type

Error

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[mcgrathcoutinho]

Hey @fatherGoose1, this issue describes #109 in point 3 and 4. Is it possible to dup this to #109 since it clearly mentions the issue surrounding the claim() function and provides a mitigation for it. To note, it also mentions the bug #11 in points 1 and 2 and provides a mitigation for it as well. Technically both issues can be resolved by sending mintFee only and refunding the excess on the spot using msg.value - mintFee which is why I submitted them as one issue. I honestly don't think they should be split into two separate issues but if you still stand with the decision on #109, is there anyway this issue could be made into two submissions with dups to both #109 and #11 or atleast dupped to #109.

Thank you!

[c4-judge]

fatherGoose1 marked the issue as not a duplicate

[c4-judge]

fatherGoose1 marked the issue as duplicate of #109

[fatherGoose1]

I duped this one to #109 and created an extra issue for you and duped to #11: #381

You clearly describe both issues.