Incorrect Fee Handling Prevents Protocol from Updating Fees

mediumCode4rena

Lines of code

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

Vulnerability details

Impact

The primary impact of this bug is that the protocol cannot change the mint and art creation fees as intended. This limitation has the following consequences:

The protocol is unable to set the correct mint fee of 0.00005 ETH per NFT minted, as documented in the specifications:

   Mint Fee
   This fee is applied when a Minter wants to mint a Cred NFT.
   Protocol will have a mint price/fee of 0.00005 ETH.

js📋
    function setProtocolFee(uint256 protocolFee_) external onlyOwner {
        if (protocolFee_ > 10_000) revert ProtocolFeeTooHigh(); // @audit <== This check is incorrect
        mintProtocolFee = protocolFee_;
        emit ProtocolFeeSet(protocolFee_);
    }

    function setArtCreatFee(uint256 artCreateFee_) external onlyOwner {
        if (artCreateFee_ > 10_000) revert ArtCreatFeeTooHigh(); // @audit <== This check is incorrect
        artCreateFee = artCreateFee_;
        emit ArtCreatFeeSet(artCreateFee_);
    }

This discrepancy between the intended fee (0.00005 ETH) and the maximum settable fee (10,000 wei) renders the fee adjustment mechanism ineffective.

The protocol loses the ability to adjust its revenue model in response to market conditions or strategic decisions, as both the mint fee and art creation fee are effectively locked at their initial values.

Below lines demonstrate that the fees are being applied as direct values rather than percentages, contrary to the setter functions' assumptions.

From src/PhiFactory.sol:

solidity📋
   function _processClaim(
        uint256 artId_,
        address minter_,
        address ref_,
        address verifier_,
        uint256 quantity_,
        bytes32 data_,
        string memory imageURI_,
        uint256 etherValue_
    )
        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_); // @audit

From src/art/PhiNFT1155.sol:

js📋
   function createArtFromFactory(uint256 artId_) external payable onlyPhiFactory whenNotPaused returns (uint256) {
        _artIdToTokenId[artId_] = tokenIdCounter;
        _tokenIdToArtId[tokenIdCounter] = artId_;

        uint256 artFee = phiFactoryContract.artCreateFee();

        protocolFeeDestination.safeTransferETH(artFee); // @audit

It's important to note that while the immediate financial impact may be limited if the initial fees were set correctly, the long-term implications of this inflexibility could be significant for the protocol's adaptability and financial management.

Proof of Concept

This POC demonstrates the inability to set the correct protocol and art creation fees as intended by the protocol.

1. Add the test to the TestPhiFactory contract in test/PhiFactory.t.sol.
2. Run forge test --mt test_ChangeProtocolFees.

js📋
function test_ChangeProtocolFees() public {
    assertEq(phiFactory.owner(), owner, "owner is correct");
    assertEq(phiFactory.mintProtocolFee(), PROTOCOL_FEE, "protocolFee is correct");

    vm.startPrank(owner);
    // maximum 10000 wei is allowed, but according to protocol documentations and deploy script it should be 0.00005 ether
    vm.expectRevert(IPhiFactory.ProtocolFeeTooHigh.selector);
    phiFactory.setProtocolFee(10001);

    vm.expectRevert(IPhiFactory.ProtocolFeeTooHigh.selector);
    phiFactory.setProtocolFee(0.00005 ether);

    vm.expectRevert(IPhiFactory.ArtCreatFeeTooHigh.selector);
    phiFactory.setArtCreatFee(10001);

    vm.expectRevert(IPhiFactory.ArtCreatFeeTooHigh.selector);
    phiFactory.setArtCreatFee(0.00005 ether);
    vm.stopPrank();
}

Tools Used

Manual

Recommended Mitigation Steps

diff📋
+   uint constant MAX_PROTOCOL_FEE = 0.00005 ether;
+   uint constant MAX_ART_CREATE_FEE = 0.00005 ether;

    function setProtocolFee(uint256 protocolFee_) external onlyOwner {
-       if (protocolFee_ > 10_000) revert ProtocolFeeTooHigh();
+       if (protocolFee_ > MAX_PROTOCOL_FEE) revert ProtocolFeeTooHigh();
        mintProtocolFee = protocolFee_;
        emit ProtocolFeeSet(protocolFee_);
    }

    function setArtCreatFee(uint256 artCreateFee_) external onlyOwner {
-       if (artCreateFee_ > 10_000) revert ArtCreatFeeTooHigh();
+       if (artCreateFee_ > MAX_ART_CREATE_FEE) revert ArtCreatFeeTooHigh();
        artCreateFee = artCreateFee_;
        emit ArtCreatFeeSet(artCreateFee_);
    }

Assessed type

Other