Refunds sent to incorrect addresses in certain cases

mediumCode4rena

Lines of code

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

Vulnerability details

Vulnerability Details:

The _processClaim internal function is responsible for calling the claimFromFactory function in the art contract, transferring the protocol fees to the protocolFeeDestination, and handling any applicable refunds.

solidity📋
    function _processClaim(
        ...
    )
        private
    {
        PhiArt storage art = arts[artId_];

        // Handle refund
        uint256 mintFee = getArtMintFee(artId_, quantity_);
        if ((etherValue_ - mintFee) > 0) {
            _msgSender().safeTransferETH(etherValue_ - mintFee);
        }
        protocolFeeDestination.safeTransferETH(mintProtocolFee * quantity_);

        (bool success_,) = art.artAddress.call{ value: mintFee - mintProtocolFee * quantity_ }(
            abi.encodeWithSignature(
                "claimFromFactory(uint256,address,address,address,uint256,bytes32,string)",
                artId_,
                minter_,
                ref_,
                verifier_,
                quantity_,
                data_,
                imageURI_
            )
        );

        if (!success_) revert ClaimFailed();
    }

When a user sends excess funds (msg.value) over the required mintFee, the difference is intended to be refunded to the user. This works as expected when the signatureClaim or merkleClaim functions are called directly in the PhiFactory contract by the user since msg.sender is the correct address (User).

However, in some cases refunds are sent to the wrong address.

Case 1: Claim function and batchClaim function

In the batchClaim function, multiple claims are processed by calling the claim function with this (the current contract) as the caller, resulting in the msg.sender being the PhiFactory contract rather than the actual user.

solidity📋
    function batchClaim(bytes[] calldata encodeDatas_, uint256[] calldata ethValue_) external payable {
        if (encodeDatas_.length != ethValue_.length) revert ArrayLengthMismatch();

        // calc claim fee
        uint256 totalEthValue;
        for (uint256 i = 0; i < encodeDatas_.length; i++) {
            totalEthValue = totalEthValue + ethValue_[i];
        }
        if (msg.value != totalEthValue) revert InvalidEthValue();

        for (uint256 i = 0; i < encodeDatas_.length; i++) {
            this.claim{ value: ethValue_[i] }(encodeDatas_[i]);
        }
    }

Similarly, the claim function also uses this when calling either the signatureClaim or merkleClaim functions, further propagating the incorrect msg.sender value. Consequently, any refunds are sent to the PhiFactory contract instead of the actual user, causing an incorrect refund.

Case 2: signatureClaim function and merkleClaim function in the Claimable contract

The signatureClaim and merkleClaim functions are also callable through the Claimable contract, which is part of the PhiNFT1155 contract. However, like in the previous case, the msg.sender will be incorrect being the PhiNFT1155 contract itself, rather than the actual user who initiated the call. This results in refunds being directed to the PhiNFT1155 contract rather than the user. In this case, funds are also not able to be pulled out by the owner, effectively locking them in the contract.

Impact:

• Incorrect handling of msg.sender results in refunds being sent to contracts (PhiFactory or PhiNFT1155) rather than the intended users.
• While the loss of funds due to overpayment is more of a user error, the primary concern here is that the intended logic for refunding users is incorrect which was specifically mentioned by the protocol to look out for.
  • 2.a. Function Reliability: Ensure the contract behaves consistently under various conditions.
  • 2.b. Function Correctness: Ensure the smart contract logic correctly implements the intended functionality without errors.
  Reference

Proof Of Concept

solidity📋
    function test_claim_1155_refund() public {
        // SET UP
        vm.warp(START_TIME + 1);
        uint256 artId = 1;
        address artAddress = phiFactory.getArtAddress(artId);
        vm.warp(START_TIME + 2);
        bytes memory data = _createSigSignData(true);
        bytes memory payload = abi.encodePacked(abi.encodeWithSignature("signatureClaim()"), data);
        // SET UP
        // Check 1155 contract ETH balance before
        assertEq(address(artAddress).balance, 0 ether);
        // send mint fee plus extra 0.1 ETH
        vm.startPrank(participant, participant);
        (bool success,) = artAddress.call{ value: phiFactory.getArtMintFee(artId, 1) + 0.1 ether }(payload);
        require(success, "1155 artAddress.call failed");

        // Check 1155 contract ETH balance, refund sent here instead
        assertEq(address(artAddress).balance, 0.1 ether);
    }

Tools Used:

• Manual analysis

Recommendation:

Modify the affected functions (batchClaim, claim, signatureClaim, and merkleClaim) to pass the original user address as a parameter. This way, the function can handle refunds correctly by transferring excess funds to the correct user.

Assessed type

Other