Claim Function Fails to Refund Excess ETH Correctly

[c4-bot-7]

Lines of code

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

Vulnerability details

Vulnerability Details:

The claim function allows users to claim an art reward, calling either the merkleClaim or signatureClaim function based on the chosen verification process. These functions internally call the _processClaim function to handle fees, mint the token, and refund any excess ETH to the user.

    function claim(bytes calldata encodeData_) external payable {
        bytes memory encodedata_ = LibZip.cdDecompress(encodeData_);
        (uint256 artId) = abi.decode(encodedata_, (uint256)); 
        PhiArt storage art = arts[artId];
        uint256 tokenId = IPhiNFT1155Ownable(art.artAddress).getTokenIdFromFactoryArtId(artId);
        if (art.verificationType.eq("MERKLE")) {
            ...
            uint256 mintFee = getArtMintFee(artId, quantity_);
            this.merkleClaim{ value: mintFee }(proof, claimData, mintArgs, leafPart_);
        } else if (art.verificationType.eq("SIGNATURE")) {
            ...
            uint256 mintFee = getArtMintFee(artId, quantity_);
            this.signatureClaim{ value: mintFee }(signature_, claimData, mintArgs);
        } else {
            revert InvalidVerificationType();
        }
    }

The processClaim function handles the refund by checking if the etherValue (msg.value) sent is greater than the mintFee. If it is, the difference is refunded to the user.

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

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

However, in the claim function, the exact mintFee is passed as msg.value when calling this.signatureClaim or this.merkleClaim. This means that even if a user sends excess ETH, the excess funds will not reach the refund process and will not execute as intended, as the extra funds will remain in the PhiFactory contract instead of being returned to the user.

Impact:

Users are not refunded their excess ETH, as the smart contract does not implement the intended logic as mentioned here. This could accumulate over multiple transactions, resulting in significant amounts of ETH being retained by the contract rather than refunded to users.

Proof Of Concept

    function testClaimFuncRefundExtra() public {
        // SET UP
        _createArt(ART_ID_URL_STRING);
        uint256 artId = 1;
        bytes32 advanced_data = bytes32("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(1, participant, referrer, verifier, expiresIn, uint256(1), advanced_data, IMAGE_URL, signature);
        bytes memory dataCompressed = LibZip.cdCompress(data);
        uint256 totalMintFee = phiFactory.getArtMintFee(1, 1);
        // SET UP
        // Check contract ETH balance
        assertEq(address(phiFactory).balance, 0 ether);
        //Check User ETH balance
        uint256 participantBalBefore = address(participant).balance;

        // send mint fee plus extra 0.1 ETH
        vm.startPrank(participant, participant);
        phiFactory.claim{ value: totalMintFee + 0.1e18 }(dataCompressed);

        // Check contract ETH balance after, kept extra funds (0.1 ETH)
        assertEq(address(phiFactory).balance, 0.1 ether);
        //Check User ETH balance after, did not get a refund
        assertEq(address(participant).balance, participantBalBefore - totalMintFee - 0.1e18);
    }

Tools Used:

• Manual analysis

Recommendation:

Modify the claim function to correctly handle excess ETH by ensuring that the _processClaim function can properly detect and refund any excess amount to the user.

Assessed type

Other

[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